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

Coral-Trino: Migrate function operator transformers defined in CalciteTrinoUDFMap from RelNode layer to SqlNode layer #349

Merged
merged 19 commits into from
Feb 8, 2023

Conversation

yiqiangin
Copy link
Contributor

@yiqiangin yiqiangin commented Jan 23, 2023

The code changes include:

  • Adding a class of OperatorBasedSqlCallTransformer which extends SqlCallTransformer and its condition is implemented based on the input operator name and the number of the operands
  • Adding the class of LinkedInOperatorBasedSqlCallTransformer which is a subclass of OperatorBasedSqlCallTransformer and transforms a Coral operator into a Trino operator on SqlNode layer for the LinkedIn specific functions
  • Adding a class of MapStructAccessOperatorTransformer which is an ad-hoc SqlCallTransformer which converts the map struct access operator "[]" defined from Calcite in a SqlIdentifier into a UDF operator of "element_at", e.g. from "col"["field"] to element_at("col", "field")
  • Removing the code of transforming standard UDF operators defined in CalciteTrinoUDFMap in the class of Calcite2TrinoUDFConverter on RelNode layer
  • Adding a class named CoralToTrinoSqlCallConverter which extends the class of SqlShuttle and initialize a class of SqlCallTransformers which containing a list of SqlCallTransformer to traverse the hierarchy of a SqlCall and converts the functions from Coral operator to Trino operator if it is required
  • Calling CoralToTrinoSqlCallConverter to convert function operators on SqlNode layer in the class of RelToTrinoConverter
  • Adding the class of ToDateOperatorTransformer as an Ad Hoc SqlCallTransformer as it needs to check the configuration parameter of AVOID_TRANSFORM_TO_DATE_UDF in its condition function
  • Some small changes on the names of date related UDF functions which should be recognized on SqlNode layer

To be noted that the migration from UDF operator transformers based on JSON infra to those implemented in Java code will be done in another several separated PRs

@wmoustafa
Copy link
Contributor

The class CalciteTrinoUDFOperatorTransformers seems complicated and I think it could use further simplification.

@wmoustafa
Copy link
Contributor

I think this PR migrates operators beyond ones leveraging JSON infra right? Might consider updating the title/description if necessary.

Copy link
Collaborator

@ljfgem ljfgem left a comment

Choose a reason for hiding this comment

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

Thanks @yiqiangin for this PR!

@wmoustafa
Copy link
Contributor

Ideally, everything is just a SqlCallTransformer or an extension of it.
Common transformation patterns (e.g., same condition based on signature) could leverage one extension together.
Similarly for Dali UDFs, either join forces with an existing extension, or create their own extension based on the condition/transformation pattern.

@yiqiangin yiqiangin changed the title Migrate standard UDF operator transformers based on JSON infra from RelNode layer to SqlNode layer Migrate existing UDF operator transformers from RelNode layer to SqlNode layer Jan 26, 2023
@yiqiangin yiqiangin changed the title Migrate existing UDF operator transformers from RelNode layer to SqlNode layer Migrate existing UDF operator transformers defined in CalciteTrinoUDFMap from RelNode layer to SqlNode layer Jan 26, 2023
Copy link
Collaborator

@ljfgem ljfgem left a comment

Choose a reason for hiding this comment

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

Thanks @yiqiangin, please run regression test for Trino again after addressing the comments.

@ljfgem
Copy link
Collaborator

ljfgem commented Jan 30, 2023

@yiqiangin: please resolve the conflicts.
@wmoustafa: do you want to take another look?

@wmoustafa
Copy link
Contributor

What is the objective of Calcite2TrinoUDFConverter class? I feel it can be refactored to a transformer or a set of transformers.

@yiqiangin
Copy link
Contributor Author

What is the objective of Calcite2TrinoUDFConverter class? I feel it can be refactored to a transformer or a set of transformers.

@wmoustafa From my understanding, Calcite2TrinoUDFConverter converts UDFs by both standard transformers and ad-hoc transformers (as described in @ljfgem design document ) on RelNode layer. All the standard transformers are defined in CalciteTrinoUDFMap. The scope of this story is to migrate these standard transformers from RelNode layer into SqlNode layer. The remaining ad-hoc converters in Calcite2TrinoUDFConverter are planned to be refactored in another work according to @ljfgem 's design. @ljfgem pls correct me if I am wrong.

Copy link
Collaborator

@ljfgem ljfgem left a comment

Choose a reason for hiding this comment

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

Thanks @yiqiangin, please rerun the regression test after addressing all the comments.

Copy link
Contributor

@aastha25 aastha25 left a comment

Choose a reason for hiding this comment

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

Thanks @yiqiangin for working on this PR!

@wmoustafa
Copy link
Contributor

I have simplified the APIs. You can take a look here.

@ljfgem ljfgem changed the title Migrate existing function operator transformers defined in CalciteTrinoUDFMap from RelNode layer to SqlNode layer Coral-Trino: Migrate function operator transformers defined in CalciteTrinoUDFMap from RelNode layer to SqlNode layer Feb 6, 2023
Copy link
Collaborator

@ljfgem ljfgem left a comment

Choose a reason for hiding this comment

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

I don't have other major concerns. Let's merge if no major concerns from @wmoustafa.

@wmoustafa
Copy link
Contributor

LGTM. Great work @yiqiangin! Thanks!

@yiqiangin yiqiangin merged commit e4d4db9 into master Feb 8, 2023
@yiqiangin yiqiangin deleted the yiqiangin/transformation-migration branch February 8, 2023 23:24
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.

4 participants