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

Preserve LATERAL UNNEST during SqlNode validation #101

Merged
merged 4 commits into from
Nov 5, 2024

Conversation

KevinGe00
Copy link
Contributor

@KevinGe00 KevinGe00 commented Oct 29, 2024

Summary

This is a follow up to #98, where LATERALs were only preserved during validation if they appeared in the form ... LATERAL (SELECT ..... This PR induces the preservation behavior also in the .. LATERAL (UNNEST ... case, this is needed for Coral to produce correct translations on views (see CoralSqlNodeToSparkSqlNodeConverter for details).

Testing

  • new unit test to test that this only a preservation mechanism, we never add LATERALs that were never there to begin with
    • note that we cannot write a LATERAL UNNEST test case here in Calcite since it will not parse, see here. Instead we will use an acompanying Coral unit test that will generate the LATERAL UNNEST sqlnode case we need to test
  • new unit test in coral with these changes: Bump Calcite to preserve LATERALs in LATERAL UNNEST calls coral#542
  • mvn clean build (also runs all unit tests) ./mvnw clean install -Dgpg.skip -f core/pom.xml
  • clean build in coral with these changes ./gradlew clean build
  • correct translation with these changes for production view text which was previously incorrect
  • regression testing on all production views [pending]

Comment on lines -3141 to -3144
case LATERAL:
// Validate subquery that LATERAL precedes
validateQuery(((SqlCall) node).operand(0), scope, targetRowType);
break;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was incorrect, we want to recurse into same function (validateFrom) and not into validateQuery.

Comment on lines +2002 to +2004
case LATERAL:
convertFrom(bb, ((SqlCall) from).operand(0));
return;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is needed to work correctly with Coral's HiveSqlToRelConverter class which extends this class and overrides this function.

@aastha25
Copy link
Contributor

@KevinGe00 in general (1) lets add the commands used to build & test, especially how to i-test with Coral, in the PR description. it helps build confidence in the changes and it'll be useful for future work as well. (2) Please file the Coral PR and cross reference it here so that reviewers can follow testing done related to new unit test in coral with these changes

@KevinGe00 KevinGe00 changed the title Preserve lateral unnest during SqlNode validation Preserve LATERAL UNNEST during SqlNode validation Oct 30, 2024
@KevinGe00
Copy link
Contributor Author

@KevinGe00 in general (1) lets add the commands used to build & test, especially how to i-test with Coral, in the PR description. it helps build confidence in the changes and it'll be useful for future work as well. (2) Please file the Coral PR and cross reference it here so that reviewers can follow testing done related to new unit test in coral with these changes

i-testing with Coral can only be done after merging and publishing these changes, then using the newest Calcite version containing these changes in a Coral feature branch in the regression pipeline, same thing done for #98.

@aastha25
Copy link
Contributor

aastha25 commented Nov 5, 2024

Approving to unblock i-testing. Let's revisit the code changes of this PR during / after the i-testing.

@KevinGe00 KevinGe00 merged commit 1c8cf43 into linkedin:li-1.21.0 Nov 5, 2024
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.

2 participants