-
Notifications
You must be signed in to change notification settings - Fork 2
Adds explicit definition of the algebraic syntax #227
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
Conversation
…he translation of PP expressions/patterns
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for pushing this!
spec/index.html
Outdated
<a href="#defn_absOrderBy" class="absOp">OrderBy</a>(|A|, |condition|) | ||
is an algebraic query expression if | ||
|A| is an algebraic query expression and | ||
|condition| is an ordering condition.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be nice to link here to a definition of "ordering condition"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I totally agree!!! Yet, there is no such definition anywhere in the spec; at least, I couldn't find any.
The existing parts related to the definition of ORDER BY have the same issue:
- Section 18.2.5.1 ORDER BY just mentions a "list of order comparators" without saying what exactly that is.
- In the "Definition: Evaluation of OrderBy" in Section 18.5.2 Evaluation Semantics it is simply called "condition" without any further clarification.
- The definition of the corresponding algebra operator also just says "condition" (and the fact that this definition simply states that "the [result] sequence satisfies the ordering condition" is also not particularly helpful).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created issue #229 to point out the lack of a definition of "ordering condition"
is an algebraic query expression if | ||
|A| is an algebraic query expression.</li> | ||
<li id="defn_absGroup"> | ||
<a href="#defn_absGroup" class="absOp">Group</a>(|exprlist|, |A|) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit puzzled by having Group
here because it can't be composed with any other algebraic query operator (Join, ...) except Aggregation.
Idea: merge AggregateJoin
, Aggregation
and Group
definitions in a single bullet point to make clear they are part of the same construction and not stand-alone and that constructions like Distinct(Group(...))
are not possible.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking the exact same thing and was planning to get back to this very idea in a follow-up PR. In the given PR, as a first step, I only wanted to make explicit what I see as the algebraic syntax used in the spec so far.
When looking at the corresponding definitions in Section 18.5.2 Evaluation Semantics, it seemed clear to me that "Group(exprlist, P)" and "Aggregation(exprlist, func, scalarvals, Grp)" have been considered as independent operators of the algebraic syntax: they have their own cases/definitions for the eval function (which is formally incorrect because, for these two cases, the result is of a completely different kind than for any other case of the eval function).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes! I always got puzzled by these separated definitions for Group
and Aggregation
. I agree with @afs, it might be nice to have a list of future step and link it here using respec issue
feature to make clear to the reader this is a work-in-progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have created issue #230 to point out that 'Group' and 'Aggregation' should not be presented as independent operators of the algebraic syntax, and I will push a commit with inline Notes that point to these GitHub issues using the respec issue
feature.
spec/index.html
Outdated
is an algebraic query expression.</li> | ||
<li> | ||
A sequence of <a href="#defn_sparqlSolutionMapping">solution mappings</a> | ||
is an algebraic query expression.</li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
None of the other definitions in this list makes use of the multiset vs sequence distinction. It's weird to have it only here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The different cases of this overall definition capture any possible type of sub-expression of the algebraic syntax that I have found in the translation algorithm of Section 18.2 Translation to the SPARQL Algebra.
A multiset of solution mappings becomes a sub-expression if the WHERE clause of the given query contains a VALUES clause: see the point "If the form is InlineData" in Section 18.2.2.6 Translate Graph Patterns.
A sequence of solution mappings becomes a sub-expression if the given query contains a trailing VALUES clause: see Section 18.2.4.3 VALUES.
I didn't want to overload the PR by also already including potential changes to the syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes perfect sense! Thank you for the detailed answer. I would add this to the list of possible follow ups.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added issue #231 to point out that we may want to change this part of the algebraic syntax.
If there are more steps to come, maybe have one issue for this work programme with tasks. |
Done: #228 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for opening the issues!
@afs same question for this PR; are you still planning to review it? Also, in case you haven't seen it, the issue with the work program that suggested me to create, is #228 @rubensworks What do you think of this PR? |
This was discussed during the #rdf-star meeting on 19 June 2025. View the transcriptw3c/sparql-query#221 and w3c/sparql-query#227<gb> Pull Request 221 Consider bnode graph names in evaluation of Graph (by hartig) <gb> Pull Request 227 Adds explicit definition of the algebraic syntax (by hartig) olaf: I have two PRs on sparql-query that have been opened for a long time AndyS: does not look controversial |
The Echidna step is failing because of HTML validation failure.
Line numbers are after file inclusion so don't align with the HTML 😞. The HTML error seems to be these |
Fixed now. |
<p id="defn_AlgebraicQueryExpression">An | ||
<a href="#defn_AlgebraicQueryExpression">algebraic query expression</a> | ||
is defined recursively as follows:</p> | ||
<ul> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Presentation comment:, not necessary for this PR:
This might be better as a table. All the <var>A</var> is an algebraic query expressions
etc can be written once then the presentation is more compact.
…ary representation.
This PR is the next step towards addressing the issue that the spec represents the operators of the algebraic syntax of SPARQL in exactly the same way as it represents the algebra functions used to define the evaluation semantics. See the previous PR for a description of the issue: #216
One aspect of the issue is that the algebraic syntax is not actually defined explicitly. Instead, the possible expressions of the algebraic syntax are introduced only implicitly as part of the translation procedure defined in Section 18.2 Translation to the SPARQL Algebra.
This PR focuses on that aspect by adding a new section that provides an explicit definition of the algebraic syntax, added directly before Section 18.2 Translation to the SPARQL Algebra. This definition includes
id
anchors for all operators of the algebraic syntax. These anchors can be used in the next step (future PR) to link all mentions of these operators to their definition, similar to how PR #216 linked the algebra operators to their definitions. In fact, as a second part of this PR, Sections 18.2.2.3 Translate Property Path Expressions and 18.2.2.4 Translate Property Path Patterns are already changed to use such links.I am leaving it to a future PR to do the same (i.e., using such links) everywhere else in Section 18.2 Translation to the SPARQL Algebra and also in Section 18.5.2 Evaluation Semantics. I chose not to include all these changes already in this PR to make it easier to review this step first.
(Section 18.3 after this PR)
Preview | Diff