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

More accurate definition of the Group operator #132

Merged
merged 2 commits into from
Dec 8, 2023
Merged

More accurate definition of the Group operator #132

merged 2 commits into from
Dec 8, 2023

Conversation

hartig
Copy link
Contributor

@hartig hartig commented Nov 14, 2023

... as discussed in #131.


Preview | Diff

@hartig hartig added the spec:editorial Minor issue or proposed change in the specification (markup, typo, informative text) label Nov 14, 2023
@hartig hartig linked an issue Nov 14, 2023 that may be closed by this pull request
Copy link
Member

@TallTed TallTed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small things

spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
spec/index.html Outdated Show resolved Hide resolved
Co-authored-by: Ted Thibodeau Jr <tthibodeau@openlinksw.com>
Copy link
Contributor

@afs afs left a 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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"projection" seems ambiguous here.

Suggested change
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)

Copy link
Contributor Author

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.

Copy link
Member

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[@TallTed] Please create a new issue for the part that's out of scope of this PR, so we don't lose track of it.

Done: PR #134

@hartig hartig merged commit 826c5d9 into main Dec 8, 2023
2 checks passed
@hartig hartig deleted the Issue131 branch December 8, 2023 14:54
hartig added a commit that referenced this pull request Dec 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
spec:editorial Minor issue or proposed change in the specification (markup, typo, informative text)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Definition of 'Group' operator insufficient
5 participants