EI-2742 - Introduce updated SparqlQuery RPC (for FKG) #36

vtermanis · 2024-02-26T14:27:28Z

⚠️ Do not merge until squashed & commit renamed (to reflect new request naming)
~~Local-scope SparqlQuery can be performed with FKGQuery local-scope~~ Same as before now
This will stay as a draft until EI-2743 has been implemented

vtermanis · 2024-02-26T15:41:27Z

proto/iotics/api/meta.proto

@@ -82,29 +80,6 @@ enum SparqlResultType {
  RDF_NTRIPLES = 5;
 }

-// SparqlQueryRequest describes a SPARQL query.
-message SparqlQueryRequest {


Note to reviewers: This is not re-used/modified since ExplorerQuery still uses it.

conbon · 2024-02-26T15:53:12Z

proto/iotics/api/meta.proto

+  // belongs. The result is returned as a sequence of chunks. Note that:
+  // - Result chunks MIGHT arrive out of order and it is the client's responsibility to re-assemble them.
+  // - This RPC is currently in alpha!
+  rpc FKGQuery(FKGQueryRequest) returns (stream FKGQueryResponse) {}


What do think of just keeping the SparqlQuery naming?
Seems like FKGQuery just muddies the water.
The doc string can indicate the enhanced abilities, but its still just sending a sparql query?

Sure - I hadn't realised we were happy with keeping exactly the same name 👍

smartrics · 2024-02-26T17:37:05Z

proto/iotics/api/meta.proto

+  // belongs. The result is returned as a sequence of chunks. Note that:
+  // - Result chunks MIGHT arrive out of order and it is the client's responsibility to re-assemble them.
+  // - This RPC is currently in alpha!
+  rpc FKGQuery(FKGQueryRequest) returns (stream FKGQueryResponse) {}


Do we need the FKG prefix in all the items?
Can't we assume this is "the" only API we'll ever need?
If we need specialisations, then the name of the specialisation can be added

Also does Query imply only the ability to read only?

Query is read-only (just like before SparqlQuery & SparqlUpdate) - but see above - I'll change it back to SparqlQuery

24d01b6
(Note that I'll change the commit message at the end. Probably easier to look at the whole diff rather than my fixup)

smartrics · 2024-02-26T17:39:12Z

proto/iotics/api/meta.proto

-  rpc SparqlQuery(SparqlQueryRequest) returns (stream SparqlQueryResponse) {}
+  // FKGQuery performs a SPARQL 1.1 query against the Federated Knowledge Graph of the Iotics network to which this host
+  // belongs. The result is returned as a sequence of chunks. Note that:
+  // - Result chunks MIGHT arrive out of order and it is the client's responsibility to re-assemble them.


why the out of order?
it all chunks are guaranteed to be produced, why can't they be reordered by the server?

Because we don't really want to buffer an arbitrary amount of data, in memory, on the server

smartrics · 2024-02-26T17:40:41Z

proto/iotics/api/meta.proto

+  Scope scope = 2;
+
+  // SPARQL query request payload
+  Payload payload = 3;


do we need the payload?

yes we need it to be consistent with everything else in the API

smartrics · 2024-02-26T17:41:03Z

proto/iotics/api/meta.proto

+
+    // The desired result content type. Note that choosing an invalid result type for the type of query will result in
+    // an error status reported in the response. (See SparqlResultType for valid content-query type combinations.)
+    SparqlResultType resultContentType = 1;


can you document the behaviour if resultContentType isn't specified

Sure - I'll add the default

24d01b6 (basically it'll be the default enum value since the field cannot be "empty")

smartrics · 2024-02-26T17:41:54Z

proto/iotics/api/meta.proto

+    bool last = 2;
+
+    // Content type of the result.
+    SparqlResultType contentType = 3;


do you need this? or will it be == to the valye in the request

Yes, it will be equal.
This is so that the receiver of responses doesn't need to know about the request necessarily. (We have the same approach in the original SparqlQuery and other fields such as - headers with client references)

smartrics · 2024-02-26T17:42:12Z

proto/iotics/api/meta.proto

+
+    // Query result chunk, encoded according to actualType.
+    // Note that:
+    // - The maximum size of each chunk is host-specific.


how does one find what the value is?

There is no way yet but we could (later) expose a host high-level conf page. It could be part of an admin API, etc. For now it is visible via the AWS console

OK - so it should be possible to document the current value here in the API?

We could hint at a default but I think "hard coding" it here in the docs means we cannot change it.

StephanieBracaloni · 2024-02-27T09:59:05Z

Local-scope SparqlQuery can be performed with FKGQuery local-scope

Local-scope SparqlQuery will be removed in the end

StephanieBracaloni · 2024-02-27T11:03:49Z

It looks good in principle. It is consistent with what we have in the API in general and the chunking approach is consistent with what we were doing for Sparql query (minus the multi-host reply).

I would vote for a more generic name like "SparqlQuery".
👍

vtermanis · 2024-02-27T15:44:04Z

Local-scope SparqlQuery will be removed in the end

If you mean the old SparqlQuery+local will be replaced - sure.
But if you mean there will be no way to query just the local host .. why wouldn't this be a useful feature? (Or am I missing the point here - we still have scope?)

proto/iotics/api/meta.proto

StephanieBracaloni · 2024-02-28T09:41:10Z

Local-scope SparqlQuery will be removed in the end

If you mean the old SparqlQuery+local will be replaced - sure. But if you mean there will be no way to query just the local host .. why wouldn't this be a useful feature? (Or am I missing the point here - we still have scope?)

This just means the existing SparqlQuery will be removed.

vtermanis · 2024-03-07T19:43:33Z

proto/iotics/api/meta.proto

+  message Payload {
+
+    // Position of a chunk in result for a given request. The first chunk has a sequence number of 0.
+    uint64 seqNum = 1;


Question (CC @StephanieBracaloni ): Do we want to keep this compatible in terms of field indexes by starting with 2 here? (hostId was 1 before)?
That way existing SPARQL requests made should still work and hostId would be ignored.

The field indexes do not matter (ie. it is ok if it starts from 2).
However, we don't want to/don't have to keep things compatible since we have announced a breaking change (even if only internal since sparql query is not used publicly yet) and the response format will also break the compatibility.

The field indexes do not matter

the response format will also break the compatibility

.. this is the only difference? (I don't understand what you mean but never mind.)

Anyway - if we're OK to break - let's keep start at index 1, as you would if it were brand new 👍

- Local-scope SparqlQuery can be performed with FKGQuery local-scope

vtermanis commented Feb 26, 2024

View reviewed changes

conbon reviewed Feb 26, 2024

View reviewed changes

smartrics reviewed Feb 26, 2024

View reviewed changes

vtermanis commented Feb 27, 2024

View reviewed changes

proto/iotics/api/meta.proto Outdated Show resolved Hide resolved

vtermanis requested review from StephanieBracaloni, conbon and smartrics February 27, 2024 16:01

vtermanis changed the title ~~EI-2742 - Introduce FKGQuery RPC, remove SparqlQuery~~ EI-2742 - Introduce updated SparqlQuery RPC (for FKG) Feb 27, 2024

StephanieBracaloni approved these changes Feb 28, 2024

View reviewed changes

vtermanis marked this pull request as ready for review March 7, 2024 19:23

vtermanis commented Mar 7, 2024

View reviewed changes

EI-2742 - Introduce FKGQuery RPC, remove SparqlQuery

ce1dd28

- Local-scope SparqlQuery can be performed with FKGQuery local-scope

vtermanis force-pushed the ei-2742 branch from 24cd4a2 to ce1dd28 Compare March 25, 2024 08:19

vtermanis merged commit 8c959fe into main Mar 25, 2024
1 check passed

vtermanis deleted the ei-2742 branch March 25, 2024 08:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EI-2742 - Introduce updated SparqlQuery RPC (for FKG) #36

EI-2742 - Introduce updated SparqlQuery RPC (for FKG) #36

vtermanis commented Feb 26, 2024 •

edited

Loading

vtermanis Feb 26, 2024

conbon Feb 26, 2024

vtermanis Feb 27, 2024

vtermanis Feb 27, 2024

smartrics Feb 26, 2024 •

edited

Loading

vtermanis Feb 27, 2024

vtermanis Feb 27, 2024

smartrics Feb 26, 2024

vtermanis Feb 27, 2024 •

edited

Loading

smartrics Feb 26, 2024

StephanieBracaloni Feb 27, 2024

smartrics Feb 26, 2024

vtermanis Feb 27, 2024

vtermanis Feb 27, 2024

smartrics Feb 26, 2024

vtermanis Feb 27, 2024 •

edited

Loading

smartrics Feb 26, 2024

StephanieBracaloni Feb 27, 2024

smartrics Feb 27, 2024

vtermanis Feb 27, 2024

vtermanis Feb 27, 2024

StephanieBracaloni commented Feb 27, 2024

StephanieBracaloni commented Feb 27, 2024

vtermanis commented Feb 27, 2024

StephanieBracaloni commented Feb 28, 2024

vtermanis Mar 7, 2024

StephanieBracaloni Mar 8, 2024

vtermanis Mar 8, 2024

EI-2742 - Introduce updated SparqlQuery RPC (for FKG) #36

EI-2742 - Introduce updated SparqlQuery RPC (for FKG) #36

Conversation

vtermanis commented Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smartrics Feb 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtermanis Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtermanis Feb 27, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StephanieBracaloni commented Feb 27, 2024

StephanieBracaloni commented Feb 27, 2024

vtermanis commented Feb 27, 2024

StephanieBracaloni commented Feb 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtermanis commented Feb 26, 2024 •

edited

Loading

smartrics Feb 26, 2024 •

edited

Loading

vtermanis Feb 27, 2024 •

edited

Loading

vtermanis Feb 27, 2024 •

edited

Loading