-
Notifications
You must be signed in to change notification settings - Fork 0
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
More accurate definition of the Group operator #132
Conversation
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.
Small things
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.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.
Overall, and beyond this PR, maybe the definition style needs to be rewritten to be more accessible to readers.
<p>ListEval((expr<sub>1</sub>, ..., expr<sub>n</sub>), μ) returns a list | ||
(e<sub>1</sub>, ..., e<sub>n</sub>), where e<sub>i</sub> = expr<sub>i</sub>(μ) or | ||
error.</p> | ||
<p>ListEval retains errors resulting from the evaluation of the list elements.</p> | ||
</div> | ||
<p>Note that, although the result of a ListEval can be an error, and errors may be used | ||
<p>Note that, although the result of <a href="#defn_ListEval">ListEval</a> | ||
may contain errors, and errors may be used | ||
to group, solutions containing error values are removed at projection time.</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.
"projection" seems ambiguous here.
to group, solutions containing error values are removed at projection time.</p> | |
to group, solutions containing error values are removed at the end of evaluating the group and any aggregation functions.</p> |
The errors do not appear because the overall grouping expression does not let errors out (it may be used within an expression) which is earlier than "project"
(treat this as an observation more than a necessary change)
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.
@afs Thanks for pointing this out! In fact, I think there are more issues with this sentence. In particular, it talks about "solutions containing error values" which is inaccurate because solutions (i.e., solution mappings) cannot contain error values. However, I considered fixing/improving this sentence as out of scope of this PR, whose purpose is to improve the definition of the Group operator. The only reason why I touched this sentence in this PR was to add the link to the definition of ListEval for which the PR introduces a linkable anchor ID to be used in the improved definition of the Group operator.
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.
out of scope of this PR
Please create a new issue for the part that's out of scope of this PR, so we don't lose track of it.
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.
... as discussed in #131.
Preview | Diff