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

have rootFinalizerIter implement sql.MutableRowIter #909

Merged
merged 9 commits into from
Nov 1, 2024
Merged

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Oct 31, 2024

We've moved rules out of analyzer into FinalizeIters so the applyRowUpdateAccumulator is applied afterwards.
However, this seems to be a special sql.RowIter, so we need to expose the child iterators through the sql.CustomRowIter interface to apply that rule

Copy link
Contributor

github-actions bot commented Oct 31, 2024

Main PR
Total 42090 42090
Successful 12877 12877
Failures 29213 29213
Partial Successes1 4893 4893
Main PR
Successful 30.5940% 30.5940%
Failures 69.4060% 69.4060%

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

Reading through the implementations again, it might be better if iterators destined for update accumulator wrapping exposed an interface, if only to make the code less brittle.

@jycor jycor changed the title manually call AddRowAccumulator for ContextRootFinalizer have rootFinalizerIter implement sql.CustomRowIter Nov 1, 2024
Copy link
Contributor

@max-hoffman max-hoffman left a comment

Choose a reason for hiding this comment

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

LGTM, couple comments on GMS side

@jycor jycor reopened this Nov 1, 2024
@jycor jycor changed the title have rootFinalizerIter implement sql.CustomRowIter have rootFinalizerIter implement sql.MutableRowIter Nov 1, 2024
@jycor jycor enabled auto-merge November 1, 2024 23:18
@jycor jycor merged commit db81318 into main Nov 1, 2024
13 checks passed
@jycor jycor deleted the james/finalize branch November 1, 2024 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants