-
Notifications
You must be signed in to change notification settings - Fork 13.8k
[BP-2.1][FLINK-38400][table] Fix FILTER condition loss in STDDEV/VAR decomposition by AggregateReduceFunctionsRule #27046
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
Conversation
@lincoln-lil it seems you can not just cherry-pick it since in master there is Calcite 1.36 and in 2.1 Calcite 1.34, |
@snuyanzin Thank you for helping this! Since it's just a backport and we won't bump up calcite version for release-2.1, we can simply squash the commits into a single commit, WDYT? |
yes, +1 for squashing, however I usually do this after approvals |
* to simpler forms. This rule is copied to fix the correctness issue in Flink before upgrading to | ||
* the corresponding Calcite version. Flink modifications: | ||
* | ||
* <p>Lines 555 ~ 565 to fix CALCITE-7192. |
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've checked this file locally with a reformatted one from calcite-1.34.0, all the delta changes looks correct.
Sure, I'll ask someone help the review first. |
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.
LGTM overall, just one minor suggestion.
import org.junit.jupiter.api.BeforeEach; | ||
import org.junit.jupiter.api.Test; | ||
|
||
/** Test for {@link AggregateReduceFunctionsRule}. */ |
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.
Import this rule.
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.
Thanks for the tips! But I can’t find a strict rule that requires adding import statements for classes referenced only in comments, just to avoid red underlines in certain IDEs. Moreover, in the existing *RuleTest classes, we’ve consistently followed the convention of not adding such imports (see flink-table/flink-table-planner/src/test/java/org/apache/flink/table/planner/plan/rules/logical/*RuleTest.java).
Besides that, there’s nothing else that needs to be changed so far, so I’m planning not to make this modification. Does that sound okay to you?
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.
Thanks for the feedback, no more comments from me.
Still, I’d prefer fully qualified names in this case.
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 checked the existing RuleTest
classes — they don’t need explicit imports because they share the same package as the Flink extended rules, so the IDE doesn’t flag any errors.
In this case, however, the packages are different, so I’ll follow your suggestion and make the change.
@flinkbot run azure |
@lincoln-lil fyi: rerun will not help here |
…nsRule and format
…ition by AggregateReduceFunctionsRule
a416bae
to
cdda0aa
Compare
This is a backport pr for release-2.1 branch.