-
Notifications
You must be signed in to change notification settings - Fork 3
Add missing comparison operators for triple terms #289
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
base: main
Are you sure you want to change the base?
Conversation
hartig
left a comment
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.
In addition to the suggestions in my review, I propose to remove the sentence about triple terms that is after the operator mapping table in Sec.17.3.
|
Aside from bringing A smaller issue (if we ignore the above) is that the signature of |
Correct, for subjects and predicates, it can only return 0 (since equality is defined for IRIs and bnodes) or error. This change also offers a baseline for systems that offer operator extensions for defining < and > for IRIs and blank nodes. |
How would this comparison work? The definition says: The
Those tests aren't approved yet, just proposed, so while we can clean them up no "retraction" would be necessary. |
If A and B are equal (e.g. when evaluating
So I do see how "the next step" could be interpreted differently, so wording should be improved there. |
|
Apologies. I misread your example, and didn't notice that subject and object were the same. However, this feels like a very small special case that could just as easily be handled by My opinion is that we should not include If we did include the operator support for triple terms, I think we would need to also include similar definitions for IRIs and blank nodes for consistency (which to be clear I think would be a bad idea). |
Co-authored-by: Olaf Hartig <[email protected]>
Co-authored-by: Olaf Hartig <[email protected]>
I understand your concern. The lack of default comparison support for IRIs and bnodes can introduce confusion indeed (it already has in the tests).
I would also not add support for bnodes indeed. However, I don't see immediate issues with adding IRI support, so I wouldn't be against that. |
|
+1 to @kasei I have a hard time to find a usecase for this extension of |
|
Chiming in from the side: All I can say about |
|
I also do not have a strong opinion, but I also think that adding support for iri comparison makes sense. If support for IRIs would be added, I think adding support for triple terms also makes sense. But like the majority here I also think adding, triple term comparison without defining IRI comparison is really confusing (to reason on what an engine would return). |
|
@hartig —
There are no section numbers here. Can you please change the above quoted snippet to identify that location in some way which is visible in the HTML/ReSpec source? |
@TallTed , @rubensworks has already implemented my suggestion, see commit 189d62e |
|
As discussed during the last SPARQL task force meeting, this PR has been updated to include comparison for IRIs and blank nodes. |
| <code>xsd:strings</code>, <code>xsd:booleans</code> and <code>xsd:dateTimes</code>. Pairs of | ||
| <code>xsd:strings</code>, <code>xsd:booleans</code>, <code>xsd:dateTimes</code>, and | ||
| <a data-cite="RDF12-CONCEPTS#dfn-triple-term">Triple Terms</a>. Pairs of | ||
| IRIs are ordered by comparing them as literals with datatype <code>xsd:string</code>.</p> |
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 think we need to say something about case-sensitive ordering being the norm in some environments, and case-insensitive ordering in others, as well as how to override both norms, assuming that is possible anywhere if not everywhere. I know we've said in some place(s) that lexical ordering is to be code point order, which is effectively case-sensitive, but I don't think this will suffice for all use cases. So far as I knowI believe code point order will also prove problematic when trying to compare, for instance, ä, as a single character and as a composite character (i.e., a combination of a and ¨), and in making all the as (uppercase, lowercase, with diacriticals, etc.) appear as a group and be in a predictable order within that group.
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.
"Function and operators" has fn:collation-key and https://www.w3.org/TR/xpath-functions-31/#choosing-a-collation to handle this.
(I don't see anything new to SPARQL 1.2 here so I think it's not for this PR.)
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
Co-authored-by: Ted Thibodeau Jr <[email protected]>
|
@rubensworks
Ignore what I said, I just read the spec changes and my assumption based on your message were wrong. |
I agree this is not for the operator mapping table - otherwise it would allow the use in any expression, not limited to It is probably worth introducing a piece of terminology for this. Having a name means we can have a (short) section about it, define the function, and pass the function into the order processs. (It is sort-of there, not by name, in "15.1 ORDER BY.) "pairwise comparsion" vs "ordering comparison" -- feels a bit unnatural and clunky to repeat it over-and-over again. "comparison" vs "collation" -- "order collation"? |
|
@afs Just to make sure I understand your comment correctly:
15.1 ORDER BY explicitly refers to the "<" operator, which is extended towards triple terms within this PR.
Are you referring to the ORDER BY section here, or the new section for |
While #194 introduced sameValue and sameTerm support for triple terms, but
<,<=,>, and>=were missing, so this PR adds them.It also adds ORDER BY support for triple terms.
The same approach as the RDF-star spec was followed here, with some small tweaks.
Thanks to the keen eye of @jitsedesmet for noticing this was missing!
Preview | Diff