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-Spark: Migrate some operator transformers from RelNode layer to SqlNode layer #351

Merged
merged 5 commits into from
Feb 10, 2023

Conversation

ljfgem
Copy link
Collaborator

@ljfgem ljfgem commented Feb 3, 2023

Changes:

  1. Migrate convertDaliUDF, convertBuiltInUDF and fallbackToHiveUdf from RelNode layer in IRRelToSparkRelTransformer to SqlNode layer. We will migrate other remaining transformers and remove IRRelToSparkRelTransformer afterwards.
  2. Override aliasContext in CoralRelToSqlNodeConverter to convert RelNode like f(x).y.z to SqlNode which meets our expectation, check its comment for details.
  3. Enhance SparkSqlRewriter to avoid casting to any type containing ROW given Spark doesn't allow that.

Tests:

  1. Existing unit tests which already covered tests for these changes, no regression
  2. Regression test on all the production views, no regression
  3. Querying several complex production views on gateway, no regression

Copy link
Contributor

@wmoustafa wmoustafa left a comment

Choose a reason for hiding this comment

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

A few things:
1- Does it change the engine facing APIs?
2- Does it make sense to merge #349 first?
3- Could you see if you could reuse any of the transformers introduced in #349?

@ljfgem
Copy link
Collaborator Author

ljfgem commented Feb 6, 2023

@wmoustafa:
1- Does it change the engine facing APIs?
No for this PR, given the 3 migrated transformers in this PR don't need type derivation.

2- Does it make sense to merge #349 first?
Don't think this PR depends on #349, I can introduce SourceOperatorMatchSqlCallTransformer and OperatorRenameSqlCallTransformer in this PR for cardinality -> size transformation.
Besides, I think it makes more sense to merge this PR first, see my comment here. If we merge this PR first, #349 doesn't need this part.

3- Could you see if you could reuse any of the transformers introduced in #349?
Two small transformers SourceOperatorMatchSqlCallTransformer and OperatorRenameSqlCallTransformer can be reused for cardinality -> size transformation.

@ljfgem ljfgem force-pushed the coral_spark_transformation_migration branch from 104a68c to 394979a Compare February 6, 2023 23:33
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 for the PR @ljfgem

@ljfgem ljfgem force-pushed the coral_spark_transformation_migration branch from cc26200 to 2c388fe Compare February 9, 2023 16:20
@ljfgem
Copy link
Collaborator Author

ljfgem commented Feb 9, 2023

@aastha25 Thanks for your review! I have addressed all the comments and replied.
@wmoustafa PTAL, thanks!

@ljfgem ljfgem merged commit cdbdd4e into linkedin:master Feb 10, 2023
@ljfgem ljfgem deleted the coral_spark_transformation_migration branch February 10, 2023 18:58
@linkedin linkedin deleted a comment from wmoustafa Feb 11, 2023
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