-
Notifications
You must be signed in to change notification settings - Fork 2
Adds links to algebraic syntax symbols within the translation section #245
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
…s now linked to its definition and ii) variables are marked up
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 this tedious work
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.
Looks really good. A few comments and suggestions added (with most being issues perhaps orthogonal to this PR).
<blockquote> | ||
<p>If the form is <a href="#rInlineData">InlineData</a></p> | ||
</blockquote> | ||
<pre class="code nohighlightBlock">The result is a multiset of solution mappings 'data'.</pre> | ||
<pre class="code nohighlight">The result is a multiset <var>data</var> of solution mappings.</pre> | ||
<div id="data-block"> | ||
<blockquote> | ||
<i>data</i> is formed by forming a solution mapping from the variable in the | ||
<var>data</var> is formed by forming a solution mapping from the variable in the | ||
corresponding position in list of variables (or single variable), omitting a binding | ||
if the <a href="#rDataBlockValue">DataBlockValue</a> | ||
is the word <code>UNDEF</code>. |
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.
This markup results in visually confusing nesting by the use of blockquotes, while in the presence of other blockquotes that do not imply nesting. "The result is a multiset" seems to be nested under "If the form is InlineData", but then "data is formed by forming…" is visually styled in the same manner.
> If the form is InlineData
The result is a multiset data of solution mappings.
> data is formed by forming a solution mapping…
I'm not sure how to easily fix this, but I'll also call out that later (in section 18.3.5.1 and elsewhere), the blockquotes are used in reverse, with the nesting language being non-quoted, and the translation procedure quoted:
If the query string has an ORDER BY clause
> M := OrderBy(M, list of order comparators)
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 that both of these are issues.
The first one (having the explanation of how data is formed as a blockquote) can easily fixed by removing the blockquote and including the explanation within the line of "pseudocode". Additionally, I think the explanation itself also needs to be improved quite a bit; it is not really clear what it says. Yet, I didn't want to overload this PR with things like these.
As for the revered use of blockquotes between Sections 18.3.2.6 and 18.3.5, I noticed that as well. Also, there are other related sections that don't use such blockquotes at all. Generally, there are quite a number of different styles throughout Section 18. Maybe someone has the time to make this consistent ;-)
Co-authored-by: Gregory Todd Williams <greg@evilfunhouse.com>
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.
tweaks for clarity, sometimes of the html source, sometimes of the eventual rendering
@TallTed, I agree with every single one of your suggestions. I resisted to make such changes myself when working on this PR. I wanted to make it as easy as possible to see that the PR does not actually change anything in the definitions, other than adding markup. For this reason, I deliberately tried to keep the original layout of the HTML source code unchanged (e.g., not adding line breaks even if they would have made the resulting HTML code nicer to look at). In this spirit, I will keep your suggestions uncommitted for the moment, until @afs had a chance to look at the PR, and then I will apply them afterwards. |
@afs are you still planning to take a look at this PR? |
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.
Maybe add an entry in the "changes" section? Mainly due to the scale of the changes but also see the question about why the name change.
<h3>Translation to the SPARQL Algebra</h3> | ||
<section id="translation"> | ||
<span id="sparqlQuery"><!-- obsolete id --></span> | ||
<h3>Translation to the Algebraic Syntax</h3> |
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 wondering why the name change to remove "SPARQL Algebra" at least as the section name (and to some extent in 18.2 previously).
The TOC becomes vague. "Translation to the Algebraic Syntax" - which "the"?
in 18,.2 the text is
resembles the SPARQL algebra
"resembles"
I don't understand the distinction being made.
(For the bulk of the changes, they look OK - it's the header / first main use that woudl seem to be the place to say "SPARQL" because it is specific to SPARQL.)
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 TOC becomes vague. "Translation to the Algebraic Syntax" - which "the"?
The algebraic syntax that, since PR #227, is explicitly defined in Section 18.2 now. I think the TOC is quite clear with this now:
18.2 Algebraic Syntax
18.3 Translation to the Algebraic Syntax
I am wondering why the name change to remove "SPARQL Algebra" at least as the section name (and to some extent in 18.2 previously).
Because the translation defined in this section does not directly translate to the SPARQL algebra. Instead, it translates into a language/syntax consisting of symbols that resemble the operators of the SPARQL algebra.
in 18,.2 the text is
resembles the SPARQL algebra
"resembles"
I don't understand the distinction being made.
Please take a look again at the following earlier reply from me to a similar comment from you: #227 (comment)
<span id="sparqlQuery"><!-- obsolete id --></span> | ||
<h3>Translation to the Algebraic Syntax</h3> | ||
<p>This section defines the process of converting graph patterns and solution modifiers in a | ||
SPARQL query string into a SPARQL algebra expression. The process described converts one |
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.
There are other uses of "SPARQL algebra expression"
e.g. https://www.w3.org/TR/sparql12-query/#func-filter-exists
Again - why not include SPARQL in the name?
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.
There are other uses of "SPARQL algebra expression"
e.g. https://www.w3.org/TR/sparql12-query/#func-filter-exists
As a general response, my aim for each of the PRs in this series of PRs related to #228 is to contain the changes of the PR just to a particular part of the whole document in order to make it easier to review each such PR (rather than introducing changes all over the place). In this sense, I will do a follow-up PR related to uses of "SPARQL algebra expression" outside of Section 18.
Again - why not include SPARQL in the name?
Would you prefer to say "SPARQL algebraic syntax" or "algebraic SPARQL syntax", instead of simply saying "algebraic syntax"?
I personally don't see a need for that because the whole document is about SPARQL, but if you think it is better to explicitly include "SPARQL" in the name of the algebraic syntax, then I can create a change.
Done: e10aa60 |
@afs are you okay with my responses to your review comments/questions? ... and with merging this PR? |
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.
We'll need to do whole document review for a sense of consistency - which isn't possible in this PR - so approved and noting #245 (comment)
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
This is the next step related to #228
Given the new option to directly link to the symbols of the algebraic syntax (as added by the previous PR, #227), this PR applies this option in Section 18.3 Translation to the SPARQL Algebra now. That is, every mention of such a symbol in this section is now linked to the definition of that symbol.
Additionally, the PR adds
<var>..</var>
markup within that section and uses relevant terminology related to the algebra and the algebraic syntax.Preview | Diff