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

Replace ValuesNode::Expression with RowExpression #13079

Closed
wants to merge 2 commits into from

Conversation

SongChujun
Copy link
Member

Description

This is a port of the Expression-To-RowExpression Migration feature in Presto. The plan is to follow the steps in their original migration to divide and conquer each PlanNode and Symbol. This PR is the port of ValuesNode, the following PRs would port PlanNodes following this order:

  • ValuesNode
  • FilterNode
  • ProjectNode/ApplyNode
  • AggregationNode
  • WindowNode
  • JoinNode
  • SpatialJoinNode
  • Symbol

Is this change a fix, improvement, new feature, refactoring, or other?

This is a port of an improvement in Presto, which is part of the multiple IR cleaning effort.

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

This PR is a change to the core query engine. The future PR may need to change SPI.

How would you describe this change to a non-technical end user or system administrator?

This change is transparent to end user, they viable difference is that when trying to run explain plan command, the Expression part in the PlanNode would be changed to RowExpression, which includes type information of the expression so it is more self-contained. But the end user should not notice the difference when executing a query.

Related issues, pull requests, and links

Original issue in Presto: prestodb/presto#12546.

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

@cla-bot
Copy link

cla-bot bot commented Jul 4, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

@SongChujun SongChujun marked this pull request as ready for review July 4, 2022 16:13
@phd3 phd3 requested review from martint and kasiafi July 4, 2022 17:18
@cla-bot
Copy link

cla-bot bot commented Jul 6, 2022

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please submit the signed CLA to [email protected]. For more information, see https://github.com/trinodb/cla.

@martint
Copy link
Member

martint commented Jul 6, 2022

Thanks for submitting this PR, but unfortunately, this is not the way we want to approach this. The result is very error prone, with a mix of old and new expressions, special casing everywhere and chances for missing things.

Instead, we want to start by split the AST and IR expressions into two separate hierarchies and adjust the planner and TranslationMap to translate between them. Then, over time, we can simplify the IR hierarchy to remove all the non essential nodes that come from the SQL grammar, add types to the IR nodes, etc.

@martint martint closed this Jul 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants