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

Improvement to sentence about grouping and errors #134

Merged
merged 1 commit into from
Feb 22, 2024
Merged

Conversation

hartig
Copy link
Contributor

@hartig hartig commented Dec 12, 2023

This PR implements the change that @afs suggested in PR #132. See #132 (comment) for the original comment with this change suggestion.

I have to admit that, even with this change, I still don't understand what exactly the sentence is supposed to say. So, maybe we can improve the sentence even more. In particular, the following points still confuse me in this sentence.

  1. In the context of talking about "the result of ListEval," the sentence mentions "solutions containing error values." Assuming "solutions" here means solution mappings (as everywhere else in the spec), this part of the sentence doesn't make sense. Solutions cannot contain "error values." By definition!
  2. Related to the previous point, the result of ListEval is not a solution (solution mapping) but it is a list consisting of RDF terms and error values. So, perhaps that's what the word "solutions" was meant to refer to?
  3. Is this sentence the only place in the spec that is related to the mentioned removal "at the end of evaluating the group and any aggregation functions"? Or is this removal somehow captured somewhere else in the spec as well (explicitly or implicitly)? I couldn't find anything and, thus, I still don't know what exactly is supposed to be removed. @afs do you have a concrete example that demonstrates the removal mentioned in the sentence?

Preview | Diff

@hartig hartig added the needs discussion Proposed for discussion in an upcoming meeting label Jan 11, 2024
@afs
Copy link
Contributor

afs commented Jan 27, 2024

@afs do you have a concrete example that demonstrates the removal mentioned in the sentence?

IIRC

ListEval happens as grouping forms, collecting rows into the group-by-key.
GROUP (1/?x as ?y) may be an error.

count(*) is different in a way. It does not evaluate anything. It does count the "error group" and so a notion of error must be present at that point. "errors" aren't hidden after ListEval happens.

May be "projection time" is referring to the end of the group-aggregate operation. It is says "error tokens don't appear outside group-aggregate". While that occurs because of the SELECT line, it's not the project operation.

?y is unbound, ?B can be false, in

SELECT ?y (bound(?y) AS ?B)
....
GROUP (1/?x as ?y)

@afs afs added spec:editorial Minor change in the specification (markup, typo, informative text; class 1 or 2) and removed needs discussion Proposed for discussion in an upcoming meeting labels Feb 8, 2024
@hartig hartig merged commit 1e15e55 into main Feb 22, 2024
3 checks passed
@hartig hartig deleted the FollowUpOfPR132 branch February 22, 2024 13:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:editorial Minor change in the specification (markup, typo, informative text; class 1 or 2)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants