Add queryable interfaces #30

rubensworks · 2022-02-03T11:50:25Z

This PR implements the latest state of the new query spec as defined in https://github.com/rdfjs/query-spec/

This PR should not be merged yet into master until the query spec is stable.
In the meantime, prereleases of this package are being published to npm for testing purposes.
At the time of writing 1.1.0-next.0 of @rdfjs/types is available on npm.

@jacoscaz I changed some small things while experimenting with these interfaces in Comunica. Nothing major was changed, just some things to simplify the implementation of them.
All interfaces (except for the filterable ones) have been implemented in Comunica, and have been confirmed to work as intended.

changeset-bot · 2022-02-03T11:50:28Z

🦋 Changeset detected

Latest commit: 09ad631

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@rdfjs/types	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

jacoscaz

Excellent work @rubensworks ! I'm really happy to hear that everything seems to work as intended. I have a few comments but only one of them is significant.

query/common.d.ts

jacoscaz

Looks good to me! I wouldn't merge, yet, but looks good.

rubensworks · 2022-02-04T09:23:03Z

No indeed, won't merge yet. Just published 1.1.0-next.1 for whoever is interested.

rubensworks · 2022-02-10T14:42:08Z

I've removed the filterable interfaces from this PR for now into #31, since no implementations of this interface exist yet, and it may take a longer time to stabilize.

This PR has been open for a week now.
Unless there are any objections, I'll wait another week (until February 17) to merge this.

rubensworks · 2022-02-10T14:44:38Z

Pinging some people who may want to have another look at this: @gsvarovsky @tpluscode

tpluscode

I realize I come late to the party, since I was not actively participating in this PR. Sorry if you might find my comment a little disruptive of your effort.

I envisioned that I would have an immediate use for it but I gotta share some observations. My biggest beef is with the Bindings interface. Please see the individual code comments

query/queryable.d.ts

tpluscode · 2022-02-10T15:29:56Z

query/queryable.d.ts

+> = unknown
+  & (SupportedResultType extends BindingsResultSupport ? {
+  queryBindings<QueryFormatType extends QueryFormatTypesAvailable>(
+    query: QueryFormatType,
+    context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,
+  ): Promise<ResultStream<Bindings>>;
+} : unknown)
+  & (SupportedResultType extends BooleanResultSupport ? {
+  queryBoolean<QueryFormatType extends QueryFormatTypesAvailable>(
+    query: QueryFormatType,
+    context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,
+  ): Promise<boolean>;
+} : unknown)
+  & (SupportedResultType extends QuadsResultSupport ? {
+  queryQuads<QueryFormatType extends QueryFormatTypesAvailable>(
+    query: QueryFormatType,
+    context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,
+  ): Promise<ResultStream<RDF.Quad>>;
+} : unknown)
+  & (SupportedResultType extends VoidResultSupport ? {
+  queryVoid<QueryFormatType extends QueryFormatTypesAvailable>(
+    query: QueryFormatType,
+    context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,
+  ): Promise<void>;
+} : unknown)


Anyone else finds the queryX methods names a little awkward?

If this is a SPARQL query interface, how about renaming them to their query form counterparts? For example as they are called in sparql-http-client

Suggested change

> = unknown

& (SupportedResultType extends BindingsResultSupport ? {

queryBindings<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<ResultStream<Bindings>>;

} : unknown)

& (SupportedResultType extends BooleanResultSupport ? {

queryBoolean<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<boolean>;

} : unknown)

& (SupportedResultType extends QuadsResultSupport ? {

queryQuads<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<ResultStream<RDF.Quad>>;

} : unknown)

& (SupportedResultType extends VoidResultSupport ? {

queryVoid<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<void>;

} : unknown)

> = unknown

& (SupportedResultType extends BindingsResultSupport ? {

select<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<ResultStream<Bindings>>;

} : unknown)

& (SupportedResultType extends BooleanResultSupport ? {

ask<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<boolean>;

} : unknown)

& (SupportedResultType extends QuadsResultSupport ? {

construct<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<ResultStream<RDF.Quad>>;

} : unknown)

& (SupportedResultType extends VoidResultSupport ? {

update<QueryFormatType extends QueryFormatTypesAvailable>(

query: QueryFormatType,

context?: QueryFormatType extends string ? QueryStringContextType : QueryAlgebraContextType,

): Promise<void>;

} : unknown)

There was also a long discussion about this one here: rdfjs/query-spec#7

Eventually, we reached a consensus about the current methods.
(I don't remember the exact argumentation, perhaps @jacoscaz or @gsvarovsky knows?)

I do remember the argumentation but it originated from the question of whether to have these type-specific methods at all. Now that we do have them in the spec, this looks like an interesting change to me. Or at least I cannot come up with a strong argument against this.

No strong opinion on this myself, I would be fine with this change. I think @gsvarovsky had some concerns about this.

I'm definitely in favour of more fluent method names. My only reservation is that these methods obtain a query object/proxy but don't execute it. But at this point that's probably splitting hairs.

If we all agree on having these methods return results directly, without an intermediate Query object, then we might also agree on changing their names as per @tpluscode's suggestion. Anyone against this?

I just remembered a (significant) reason to keep the query... methods: interface discovery through autocompletion.

Since the Queryable interface already exposes a query method, developers that don't need the API docs might start typing query... in their editor. Smart editors would then immediately also suggest queryBindings, queryQuads, ... This would help developers get directed to the simpler query methods from the SparqlQueryable interface if they are available.

If we would go for select, construct, etc, then we have to take into account the fact that construct might cause some confusion, since we don't have a 1-1 mapping with the SPARQL query clauses. Namely, construct would be used for both CONSTRUCT and DESCRIBE query clauses, which may not be obvious for newcomers.

Well, if were to go with renaming to select, construct, ... then we should also have a describe, which in the majority of cases would probably call construct internally. I guess on one side we would favor discovery by autocompletion, on the other side we would favor discovery by intuition. I can see the point in both approaches, happy to go with either.

Another (minor) argument in favor of query... is the alignment with the resultType field of Query.

All told, there seem to be more arguments in favor of keeping these methods as they are. I think we can resolve this and proceed with merging.

query/queryable.d.ts

query/common.d.ts

tpluscode

Closing thought: for a large PR with many new pieces, it would be wise to add some tests :)

tpluscode · 2022-02-10T16:52:52Z

.changeset/pre.json

@@ -0,0 +1,13 @@
+{


Aha! This will be the real last comment. Should the pre take be removed or are you intending another prerelease?

Yes, will remove.

rubensworks · 2022-02-10T17:26:32Z

Closing thought: for a large PR with many new pieces, it would be wise to add some tests :)

Right, will look into that.

query/queryable.d.ts

rubensworks · 2022-02-14T10:40:33Z

I just added some tests (I didn't aim for full coverage though, just the most critical parts for now)

…es provided as algebra objects in comments

…on a specific algebra implementation

Modularizes Queryable and SparqlQueryable interfaces

rubensworks · 2022-02-15T08:36:24Z

All issues should be resolved now. Unless there are any other comments, I'll merge this on Thursday (after fixing the changelog).

rubensworks added 6 commits January 26, 2022 09:30

Add initial draft interfaces for the query spec

b962f59

Move all mutability methods to Bindings

e30f83b

Tweak queryable interfaces based on implementation tests

e2d62fc

Add changelog for addition of queryable interfaces

bc7163e

Enter prerelease mode and version packages

5577666

Make GitHub CI also publish prereleases from feature/query branch

4e5bafb

rubensworks requested a review from jacoscaz February 3, 2022 11:50

rubensworks mentioned this pull request Feb 3, 2022

Update: experimental TS typings are available rdfjs/query-spec#10

Closed

jacoscaz reviewed Feb 3, 2022

View reviewed changes

query/common.d.ts Show resolved Hide resolved

query/common.d.ts Show resolved Hide resolved

query/common.d.ts Outdated Show resolved Hide resolved

rubensworks added 2 commits February 4, 2022 08:24

Allow supported metadata types to be overridden

71ee4ea

Add TODO to merge Stream types

1a2e38e

rubensworks requested a review from jacoscaz February 4, 2022 08:51

Release 1.1.0-next.1

60ff070

jacoscaz approved these changes Feb 4, 2022

View reviewed changes

Temporarily remove filterable typings

f64f002

rubensworks mentioned this pull request Feb 10, 2022

Add filterable interfaces #31

Open

tpluscode reviewed Feb 10, 2022

View reviewed changes

query/queryable.d.ts Outdated Show resolved Hide resolved

jacoscaz mentioned this pull request Feb 12, 2022

Modularizes Queryable and SparqlQueryable interfaces #32

Merged

rubensworks added 3 commits February 14, 2022 09:33

Also allow Bindings interaction via strings

d32e71c

Fix typo in SparqlQueryable tsdoc

2b8f571

Add tests for queryable interface

762a892

jacoscaz and others added 7 commits February 14, 2022 12:21

modularizes Queryable and SparqlQueryable interfaces

3b7ba40

mentions the difference between queries provided as strings and queri…

61f5a6a

…es provided as algebra objects in comments

adds support for multiple algebra types, drops conceptual dependency …

d7ba23c

…on a specific algebra implementation

updates test to use modularized interfaces

a666eab

adds tests for Algebra interfaces

186c2b7

drops redundant semicolons

d47c8b1

Merge pull request #32 from jacoscaz/feature/query-separate-interfaces

567ae66

Modularizes Queryable and SparqlQueryable interfaces

Exit prerelease mode

09ad631

rubensworks merged commit 1a557ee into master Feb 17, 2022

rubensworks deleted the feature/query branch February 17, 2022 10:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add queryable interfaces #30

Add queryable interfaces #30

rubensworks commented Feb 3, 2022

changeset-bot bot commented Feb 3, 2022 •

edited

Loading

jacoscaz left a comment

jacoscaz left a comment

rubensworks commented Feb 4, 2022

rubensworks commented Feb 10, 2022

rubensworks commented Feb 10, 2022

tpluscode left a comment

tpluscode Feb 10, 2022

rubensworks Feb 10, 2022

jacoscaz Feb 10, 2022

rubensworks Feb 10, 2022

gsvarovsky Feb 10, 2022

jacoscaz Feb 13, 2022

rubensworks Feb 14, 2022

jacoscaz Feb 14, 2022

rubensworks Feb 14, 2022

jacoscaz Feb 17, 2022

tpluscode left a comment

tpluscode Feb 10, 2022

rubensworks Feb 10, 2022

rubensworks commented Feb 10, 2022

rubensworks commented Feb 14, 2022

rubensworks commented Feb 15, 2022

Add queryable interfaces #30

Add queryable interfaces #30

Conversation

rubensworks commented Feb 3, 2022

changeset-bot bot commented Feb 3, 2022 • edited Loading

🦋 Changeset detected

jacoscaz left a comment

Choose a reason for hiding this comment

jacoscaz left a comment

Choose a reason for hiding this comment

rubensworks commented Feb 4, 2022

rubensworks commented Feb 10, 2022

rubensworks commented Feb 10, 2022

tpluscode left a 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

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

tpluscode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubensworks commented Feb 10, 2022

rubensworks commented Feb 14, 2022

rubensworks commented Feb 15, 2022

changeset-bot bot commented Feb 3, 2022 •

edited

Loading