Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Queryable and Filterable typings #7

Merged
merged 35 commits into from
Jan 31, 2022
Merged

Queryable and Filterable typings #7

merged 35 commits into from
Jan 31, 2022

Conversation

jacoscaz
Copy link
Contributor

@gsvarovsky, @rubensworks and I have been bouncing ideas for a little while about a new RDF/JS specification aimed at standardizing the API of query engines willing to adopt the RDF/JS specs.

This PR aims to extend the conversation to the entire RDF/JS group, using what we have produced so far as a starting point for further discussion, changes and improvements.

On top of the work done on our respective projects, the proposed spec is also informed by the preliminary feedback we have received in #5. Thanks to all who pitched in!

Copy link
Member

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor comments and open questions

queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
@jacoscaz
Copy link
Contributor Author

jacoscaz commented Oct 20, 2021

Should we vote with the thumbs-up emoji on everything we agree with and comment on everything we don't?

Copy link

@gsvarovsky gsvarovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be useful to have a (no-op) example implementation and an example usage – particularly to clarify whether the typings really help with writing and reading code.

Happy to work something up if you agree.

queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
@jacoscaz
Copy link
Contributor Author

The conversation has been silent for a while, I assume there are no inbound comments and suggestions for the time being. I'll do a first round of edits based on the above.

queryable-spec.ts Outdated Show resolved Hide resolved
@jacoscaz jacoscaz changed the title DRAFT: Queryable Spec Typings DRAFT: Queryable and Filterable typings Nov 3, 2021
@rubensworks rubensworks mentioned this pull request Nov 4, 2021
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
…, unifies cost and order types across Filterable and Queryable, drops dependency on sparqljsalgebra
@jacoscaz
Copy link
Contributor Author

jacoscaz commented Nov 5, 2021

Another round of updates done based on all the suggestions in this thread. I'm resolving issues as I go but feel free to reopen if needed.

queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Show resolved Hide resolved
queryable-spec.ts Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
@jacoscaz
Copy link
Contributor Author

Should we merge this into master and let the conversation continue in dedicated issues / PRs or is it too soon to fragment like that?

@rubensworks rubensworks self-requested a review November 22, 2021 14:14
Copy link
Member

@rubensworks rubensworks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the long delay.
Just some minor things I noticed when going over everything in detail.

queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
queryable-spec.ts Outdated Show resolved Hide resolved
@rubensworks
Copy link
Member

However, I think that for the vast majority of programmers it'd be much easier to have a spec that reads like we get the result of a query by executing the query, which would map to something like engine.query(): Query and query.execute(): Result.

Won't the fact that we have two distinct things (the query string, and the query future) that are both named "query" be too confusing then?

Binding the naming scheme of the spec to a paradigm makes for a slightly higher barrier of entry for no clear benefit IMHO.

I actually like it when that happens 😄 Makes the code more universal, as it can be explained in terms of language-independent patterns that are known by many people, and are extensively documented.
(Promise in JS is also named like that for this reason)

I'm not married to "Future", definitely open to alternatives, but AFAICS it's the best name we have at the moment.

@jacoscaz
Copy link
Contributor Author

jacoscaz commented Dec 1, 2021

Won't the fact that we have two distinct things (the query string, and the query future) that are both named "query" be too confusing then?

Good point. If we ever switched to what I proposed, that argument would definitely need to be renamed to something like queryString.

I actually like it when that happens 😄 Makes the code more universal, as it can be explained in terms of language-independent patterns that are known by many people, and are extensively documented.
(Promise in JS is also named like that for this reason)

Aha, it's nice to disagree every now and then. Disagreement is where learning happens. My preference goes for naming schemes that try to stay close to how I would state something while discussing at a coffee machine. I think that makes for code that is easier to understand, although it might be harder to write or model initially. I don't think it's a purely personal thing but I am also not sure that it isn't. I interpret the idea of universality you mention as code that is more effective at describing how it does something than at describing what it does.

EDIT: not worthy of another comment but relevant to the discussion is the fact that a naming scheme that matches the paradigm (i.e. that expresses both the what and the how) makes it harder to support both promises and callbacks while keeping the same interfaces. To me that's an instinctive red flag about violating some sort of orthogonality principle.

Coming back to earth, I'm ok with keeping the naming as-is if that's where we stand collectively. Thank you for the lovely exchange!

@gsvarovsky
Copy link

Won't the fact that we have two distinct things (the query string, and the query future) that are both named "query" be too confusing then?

No.

Per my previous comment, it's natural to me that "query" means both the query string/algebra, and the query future. The engine.xxx call is a reification of an abstract query to an executable one.

I don't like the latter being called "result", because it definitely isn't one.

Personally I prefer to keep the naming short, so while I don't have a problem with the explicit referencing of the "future" pattern, I don't think it's necessary.

@rubensworks
Copy link
Member

I don't like the latter being called "result", because it definitely isn't one.

Indeed, it's a "future to a query result".

Personally I prefer to keep the naming short, so while I don't have a problem with the explicit referencing of the "future" pattern, I don't think it's necessary.

Me too, as long as it is sufficiently descriptive.

So the options seem to be:

  1. QueryResultFutureBindings
  2. QueryBindings
  3. QueryResultBindings (this is what we have atm)

My personal preference is option 1, even though it is longer. But I'm open to option 2 if the consensus lands there. But I'm definitely against option 3.

@jacoscaz
Copy link
Contributor Author

jacoscaz commented Dec 2, 2021

All right, seems we have a consensus on option 2. QueryBindings!

@jacoscaz
Copy link
Contributor Author

jacoscaz commented Dec 8, 2021

There'a long conversation happening within the comments to #7 (comment) that has led us to realize the following issue:

const query = await engine.query('QUERY STR', { queryFormat: { language: 'someUnsupportedLanguage' } });
await query.isSupported(); // false
query.type; // ?

Basically, engine.query() may resolve to a query which the engine cannot parse, thus making it impossible to determine its result type, thus making it impossible to produce the return value of engine.query() itself unless we added a special type to the list of allowed result types.

We opted to drop isSupported() and to make the promise returned by engine.query() reject in case of unsupported queries.

Personal note: as an engine might go through multiple attempts at iteratively simplifying queries until it gets to a query that is supported by the underlying store (particularly in the case of Filterable), perhaps it'd be worth resolving to null rather than rejecting with an error, the idea being that an unsupported query might not be such an exceptional use case but rather a common occurrence.

@jacoscaz
Copy link
Contributor Author

jacoscaz commented Dec 10, 2021

@rubensworks @gsvarovsky there are no pending / unaddressed comments ATM! The only thing I have not addressed in the spec, although we have commented upon it but then forgotten about it, is where to put limit and offset or start and length. I think these ought to go both into

  1. an opts object for high-level methods (queryBindings(), ...), which could possibly be used to request a specific order known a-priori
  2. the QueryExecuteOptions interface

Objections?

Other than the above, I think we should merge this and then continue the conversation in dedicated issues.

queryable-spec.ts Outdated Show resolved Hide resolved
@rubensworks
Copy link
Member

rubensworks commented Dec 10, 2021

where to put limit and offset or start and length. I think these ought to go both into

We already have them in FilterableSource is seems, which should be sufficient I think?
The queryable interface should not have them I think, since offset/limit can be passed via SPARQL queries (or the algebra).

Other than the above, I think we should merge this and then continue the conversation in dedicated issues.

Sounds good!

@jacoscaz
Copy link
Contributor Author

We already have them in FilterableSource is seems, which should be sufficient I think?
The queryable interface should not have them I think, since offset/limit can be passed via SPARQL queries (or the algebra).

Haha, I am an idiot. +1

queryable-spec.ts Outdated Show resolved Hide resolved
@jacoscaz jacoscaz changed the title DRAFT: Queryable and Filterable typings Queryable and Filterable typings Jan 31, 2022
@jacoscaz jacoscaz merged commit 72133ee into rdfjs:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants