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

[FLINK-7942] [table] Reduce aliasing in RexNodes #5019

Closed
wants to merge 2 commits into from

Conversation

twalthr
Copy link
Contributor

@twalthr twalthr commented Nov 15, 2017

What is the purpose of the change

This PR reduces the number of AS expressions in projects. This fixes the bug mentioned in FLINK-7942 with the FilterJoinRule and improves the plans. Many calc operations are not needed anymore. For multi-windows, the change caused some problems that are not trivial to fix, so they still use aliasing for resolving time attributes.

Brief change log

  • Remove explicit alias expression from RexNodes

Verifying this change

  • Added testFilterJoinRule in JoinTest.

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, Yarn/Mesos, ZooKeeper: no
  • The S3 file system connector: no

Documentation

  • Does this pull request introduce a new feature? no
  • If yes, how is the feature documented? not applicable

Copy link
Contributor

@fhueske fhueske left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @twalthr!

This change seems to result in a plan regression for over window aggregates because projections are no longer pushed down.

Thanks, Fabian

streamTableNode(0),
term("select", "a", "c", "proctime")
),
streamTableNode(0),
Copy link
Contributor

Choose a reason for hiding this comment

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

The new plan is less efficient, because a projection isn't pushed down anymore and a wider row needs to be materialized in the over window.

@twalthr
Copy link
Contributor Author

twalthr commented Nov 15, 2017

Thanks @fhueske. I simplified the changes a bit more. Now we use explicit aliases only for windows and the alias operator.

@fhueske
Copy link
Contributor

fhueske commented Nov 15, 2017

Looks good. +1 to merge

@asfgit asfgit closed this in b6a2dc3 Nov 15, 2017
asfgit pushed a commit that referenced this pull request Nov 15, 2017
glaksh100 pushed a commit to lyft/flink that referenced this pull request Jun 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants