Skip to content

[FLINK-36662][table-planner] Port CALCITE-6317 to fix the incorrectly constant pullup when group keys in aggregate are NULL#25612

Merged
lincoln-lil merged 2 commits intoapache:masterfrom
lincoln-lil:FLINK-36662
Nov 7, 2024
Merged

[FLINK-36662][table-planner] Port CALCITE-6317 to fix the incorrectly constant pullup when group keys in aggregate are NULL#25612
lincoln-lil merged 2 commits intoapache:masterfrom
lincoln-lil:FLINK-36662

Conversation

@lincoln-lil
Copy link
Contributor

What is the purpose of the change

This pr port the bugfix of CALCITE-6317 to fix the incorrectly constant pullup when group keys in aggregate are NULL
Before upgraded corresponding calcite version(1.37), we can have the fix that copy the related RelMdPredicates to flink and remove it after calcite version upgrading done.

Brief change log

Copy the RelMdPredicates based on 1.32.0 and add the fix of CALCITE-6317

Verifying this change

Add new case for GroupingSetsTest

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API, i.e., is any changed class annotated with @public(Evolving): (no)
  • The serializers: (no )
  • The runtime per-record code paths (performance sensitive): (no)
  • Anything that affects deployment or recovery: JobManager (and its components), Checkpointing, Kubernetes/Yarn, ZooKeeper: (no)
  • The S3 file system connector: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

… constant pullup when group keys in aggregate are NULL
@flinkbot
Copy link
Collaborator

flinkbot commented Nov 5, 2024

CI report:

Bot commands The @flinkbot bot supports the following commands:
  • @flinkbot run azure re-run the last Azure build

/**
* Utility to infer Predicates that are applicable above a RelNode.
*
* <p>The class was copied over based on calcite-1.32.0 and added the bugfix of CALCITE-6317.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please mark the changed lines with

// ------ FLINK MODIFICATION BEGIN
...
// ------ FLINK MODIFICATION END

and put numbers of lines in class comment as it is done for other Calcite's classes

it will significantly help during upgrade (before we reach 1.37)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@snuyanzin thank you for reviewing this so quickly! (My original plan was to use the night hours (utc+8) to do a full assessment of the impact of the existing tests)
I've now added a description of the scope for the changes.

Copy link
Contributor

@snuyanzin snuyanzin left a comment

Choose a reason for hiding this comment

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

LGTM
thanks for addressing feedback

@lincoln-lil
Copy link
Contributor Author

LGTM thanks for addressing feedback

@snuyanzin Thank you! Merging...

@lincoln-lil lincoln-lil merged commit 1069279 into apache:master Nov 7, 2024
@lincoln-lil lincoln-lil deleted the FLINK-36662 branch November 7, 2024 10:35
xaniasd pushed a commit to xaniasd/flink that referenced this pull request Jan 13, 2025
… constant pullup when group keys in aggregate are NULL

This closes apache#25612
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants