[Coral-Trino] Migrate cast transformation from RelNode layer to SqlNode layer #491

KevinGe00 · 2024-02-19T00:51:49Z

What changes are proposed in this pull request, and why are they necessary?

The goal of this PR is to migrate the logic of visitCast from RelNode layer (in Calcite2TrinoUDFConverter.convertRel which takes a calcite rel and transforms it to trino compatible rel) to the SqlNode layer as part of Coral IR v2. Some important changes to make this happen:

New CastOperatorTransformer class which is a sqlNode transformer that handles Calcite to Trino cast operator transformation.
Since CastOperatorTransformer requires RelType derivation, which requires Calcite validation, which does not preserve laterals (known and chosen behavior), we had to override this behavior as some queries which had casts and lateral joins were having their laterals incorrectly dropped as an unwanted side effect of CastOperatorTransformer.
Calcite2TrinoUDFConverter deleted since there are no more transformations in the class left

How was this patch tested?

gradle clean build
regression tests for trino, avro and spark all passing

…ases

coral-trino/src/test/java/com/linkedin/coral/trino/rel2trino/HiveToTrinoConverterTest.java

wmoustafa

Looks like there is no direct relationship between this PR and LATERAL removal. Does it makes sense to add test cases for LATERAL in this PR or a separate PR?

Also, I recall we had Coral-side code that reintroduces the LATERA before linkedin/linkedin-calcite#98. Do you know why it does not show in the LHS version of this PR?

coral-common/src/main/java/com/linkedin/coral/common/utils/TypeDerivationUtil.java

wmoustafa · 2024-04-07T09:26:54Z

coral-trino/src/main/java/com/linkedin/coral/trino/rel2trino/RelToTrinoConverter.java

@@ -84,8 +83,7 @@ public RelToTrinoConverter(HiveMetastoreClient mscClient, Map<String, Boolean> c
   * @return SQL string
   */
  public String convert(RelNode relNode) {
-    RelNode rel = convertRel(relNode, configs);
-    SqlNode sqlNode = convertToSqlNode(rel);
+    SqlNode sqlNode = convertToSqlNode(relNode);


Why did you ignore the configs?

convertRel(relNode, configs) is a method from the now deleted Calcite2TrinoUDFConverter. I believe the logic of configs has been moved to CoralToTrinoSqlCallConverter

@aastha25 could you verify this change is safe?

This should be safe to remove.
Removing the Rel transformation layer, Calcite2TrinoUDFConverter, in Coral IR -> Trino SQL is a huge milestone. Thanks @KevinGe00 for getting us there :)

gradle/dependencies.gradle

...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/CastOperatorTransformer.java

coral-trino/src/test/java/com/linkedin/coral/trino/rel2trino/HiveToTrinoConverterTest.java

KevinGe00 · 2024-04-08T23:04:48Z

Looks like there is no direct relationship between this PR and LATERAL removal. Does it makes sense to add test cases for LATERAL in this PR or a separate PR?

The goal here is to have no test regressions caused by dropped LATERALs since we know that would be semantically incorrect. There's no need to add any additional tests in this PR because if the current tests pass, we know that the LATERAL was added back correctly by linkedin/linkedin-calcite#98. Therefore we are already testing LATERAL in Coral.

Also, I recall we had Coral-side code that reintroduces the LATERA before linkedin/linkedin-calcite#98. Do you know why it does not show in the LHS version of this PR?

We introduce LATERAL in RelToTrinoConverter.convertToSqlNode which happens before CastOperatorTransformer, where LATERAL is then dropped again due to type derivation.

wmoustafa · 2024-04-09T02:19:08Z

we know that the LATERAL was added back correctly by linkedin/linkedin-calcite#98

I was suggesting to add Coral-side tests for the same issue. Could be inspired by the failing regression integration tests.

Also, I recall we had Coral-side code that reintroduces the LATERAL before linkedin/linkedin-calcite#98

I was referring to code that @aastha25 introduced to manually reinstate LATERAL after validation. I think it was in TypeValidationUtil if I recall correctly. The reason I am asking is that now that we appropriately handle LATERAL in calcite, this reinstating code is no longer needed and should appear as deleted in the LHS of the PR diff.

KevinGe00 · 2024-04-09T02:44:54Z

I was suggesting to add Coral-side tests for the same issue. Could be inspired by the failing regression integration tests.

We already have Coral-side unit tests that capture this issue. As long as the test input query as a CAST operator and a LATERAL, the missing LATERAL post-validation issue will be tested.

to manually reinstate LATERAL after validation. I think it was in TypeValidationUtil if I recall correctly

I believe you're talking about the post processor @aastha25 wrote a long time ago to manually add the LATERAL back. This code was never commited/merged in the first place since the patch was a WIP so it wouldn't need to be deleted (deleted in the LHS of the PR diff).

wmoustafa

LGTM after minor Java Doc changes. Thanks for all the deep investigation down to the LATERAL issue in Calcite. Also Glad to see Calcite2TrinoUDFCconverter finally gone. Great work!

...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/CastOperatorTransformer.java

KevinGe00 · 2024-04-09T16:05:13Z

@wmoustafa Addressed the last 2 javadoc comments. Thanks for the thorough review :)

aastha25 · 2024-04-10T20:19:01Z

...-trino/src/main/java/com/linkedin/coral/trino/rel2trino/DataTypeDerivedSqlCallConverter.java

@@ -44,7 +45,7 @@ public DataTypeDerivedSqlCallConverter(HiveMetastoreClient mscClient, SqlNode to
    operatorTransformerList = SqlCallTransformers.of(new FromUtcTimestampOperatorTransformer(typeDerivationUtil),
        new GenericProjectTransformer(typeDerivationUtil), new NamedStructToCastTransformer(typeDerivationUtil),
        new ConcatOperatorTransformer(typeDerivationUtil), new SubstrOperatorTransformer(typeDerivationUtil),
-        new UnionSqlCallTransformer(typeDerivationUtil));
+        new CastOperatorTransformer(typeDerivationUtil), new UnionSqlCallTransformer(typeDerivationUtil));


is the ordering relevant here? Generally we add the new transformer at the end. But in this case, CastOperatorTransformer is appended before UnionSqlCallTransformer

Order shouldn't matter here since UnionSqlCallTransformer doesn't create any types of cast that CastOperatorTransformer would need to transform.

aastha25 · 2024-04-10T20:56:33Z

Could you also add in the PR description that this patch has been tested against the li-trino-hotfix branch?

rest LGTM.

ljfgem

Overall LGTM.
Thank you for removing the last RelNode layer transformation in Coral-Trino, this is a great milestone!

...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/CastOperatorTransformer.java

coral-trino/src/main/java/com/linkedin/coral/trino/rel2trino/RelToTrinoConverter.java

ljfgem

LGTM

aastha25 and others added 18 commits July 20, 2023 10:53

Initial commit for migrating Cast

16e7110

add type derivation changes

4acd5ba

type derivation enhance and test debug

bb8fffb

merge master

e540e4d

remove testing files

fb7a309

spotlessapply + comment out failing unit test

45da938

Merge branch 'master' into aastha25-movetrinocast

686e4e3

update unit tests

af01f97

fix regression by TypeDerivationUtil on casts with join on simple ali…

bdd7907

…ases

generalize case of casting with join on simple aliases

1fb5179

remove extraneous input to dummy select

a1f126f

temporary removal of type util postprocessor for i-test

d1bc9c4

revert

d553285

spotless checks

27aa188

update calcite version for linkedin-calcite linkedin#98/linkedin#99

0fcd7d6

remove Calcite2TrinoUDFConverter

83f2357

clean up tests

82bf185

unused imports

0884fac

KevinGe00 commented Feb 19, 2024

View reviewed changes

coral-trino/src/test/java/com/linkedin/coral/trino/rel2trino/HiveToTrinoConverterTest.java Show resolved Hide resolved

KevinGe00 mentioned this pull request Feb 19, 2024

Preserve LATERAL keyword during validation linkedin/linkedin-calcite#98

Merged

spotless

71172d9

wmoustafa reviewed Apr 7, 2024

View reviewed changes

KevinGe00 added 2 commits April 8, 2024 16:18

improve javadoc/documentation

dfaec55

fix javadoc

ccfdcfd

wmoustafa reviewed Apr 9, 2024

View reviewed changes

...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/CastOperatorTransformer.java Outdated Show resolved Hide resolved

...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/CastOperatorTransformer.java Show resolved Hide resolved

improve javadoc

4356109

grammar

2a2d1ea

aastha25 reviewed Apr 10, 2024

View reviewed changes

ljfgem reviewed Apr 10, 2024

View reviewed changes

refactor + clean up

9f2bade

ljfgem approved these changes Apr 11, 2024

View reviewed changes

KevinGe00 merged commit e09015e into linkedin:master Apr 11, 2024
1 check passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Coral-Trino] Migrate cast transformation from RelNode layer to SqlNode layer #491

[Coral-Trino] Migrate cast transformation from RelNode layer to SqlNode layer #491

KevinGe00 commented Feb 19, 2024

wmoustafa left a comment

wmoustafa Apr 7, 2024

KevinGe00 Apr 8, 2024

wmoustafa Apr 9, 2024

aastha25 Apr 10, 2024

KevinGe00 commented Apr 8, 2024

wmoustafa commented Apr 9, 2024

KevinGe00 commented Apr 9, 2024 •

edited

Loading

wmoustafa left a comment

KevinGe00 commented Apr 9, 2024

aastha25 Apr 10, 2024

KevinGe00 Apr 11, 2024

aastha25 commented Apr 10, 2024

ljfgem left a comment

ljfgem left a comment

[Coral-Trino] Migrate cast transformation from RelNode layer to SqlNode layer #491

[Coral-Trino] Migrate cast transformation from RelNode layer to SqlNode layer #491

Conversation

KevinGe00 commented Feb 19, 2024

What changes are proposed in this pull request, and why are they necessary?

How was this patch tested?

wmoustafa left a comment

Choose a reason for hiding this comment

wmoustafa Apr 7, 2024

Choose a reason for hiding this comment

KevinGe00 Apr 8, 2024

Choose a reason for hiding this comment

wmoustafa Apr 9, 2024

Choose a reason for hiding this comment

aastha25 Apr 10, 2024

Choose a reason for hiding this comment

KevinGe00 commented Apr 8, 2024

wmoustafa commented Apr 9, 2024

KevinGe00 commented Apr 9, 2024 • edited Loading

wmoustafa left a comment

Choose a reason for hiding this comment

KevinGe00 commented Apr 9, 2024

aastha25 Apr 10, 2024

Choose a reason for hiding this comment

KevinGe00 Apr 11, 2024

Choose a reason for hiding this comment

aastha25 commented Apr 10, 2024

ljfgem left a comment

Choose a reason for hiding this comment

ljfgem left a comment

Choose a reason for hiding this comment

KevinGe00 commented Apr 9, 2024 •

edited

Loading