-
Notifications
You must be signed in to change notification settings - Fork 187
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
Integrate CoralRelToSqlNodeConverter in CoralRelNode to trino SQL translation path. #315
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @aastha25 for this PR!
// Drop the AS operator from the rightSqlNode if it exists and append the LATERAL operator on the inner SqlNode. | ||
if (rightSqlNode instanceof SqlCall && ((SqlCall) rightSqlNode).getOperator().kind == SqlKind.AS) { | ||
lateralNode = SqlStdOperatorTable.LATERAL.createCall(POS, (SqlNode) ((SqlCall) rightSqlNode).operand(0)); | ||
} else { | ||
lateralNode = SqlStdOperatorTable.LATERAL.createCall(POS, rightSqlNode); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some context here (i.e. why we need to drop the AS operator from the rightSqlNode if it exists and append the LATERAL operator on the inner SqlNode), preferably with an example?
} | ||
|
||
// Update unnest operand for trino engine to expand the unnest operand to a single column | ||
private static SqlCall getTransformedUnnestSqlCall(SqlCall sqlCall) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like the SUPPORT_LEGACY_UNNEST_ARRAY_OF_STRUCT
is not used, which may cause regression for LI internal use? See #158 for more details.
/** | ||
* Temporary method to enable translations via CoralSqlNodeToTrinoSqlNodeConverter | ||
*/ | ||
public String convertDash(RelNode relNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we give this method a more descriptive name? Not following what convertDash
means.
case SELECT: | ||
return getTransformedSqlSelectSqlCall(sqlCall); | ||
case JOIN: | ||
return getTransformedJoinSqlCall(sqlCall); | ||
case AS: | ||
return getTransformedAsSqlCall(sqlCall); | ||
case UNNEST: | ||
return getTransformedUnnestSqlCall(sqlCall); | ||
case EQUALS: | ||
return getTransformedEqualsOperatorSqlCall(sqlCall); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add more descriptive comments for all the transformation methods (i.e. why we need this transformation), preferably with examples? It's kind of hard to understand these methods just by reading code.
* @param relNode relation algebra | ||
* @return CoralSqlNode representation for input | ||
*/ | ||
public SqlNode convertToCoralSqlNode(RelNode relNode) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this explicit method given it's simple and only called in convertDash
.
SqlCharStringLiteral transformArgsLiteral = | ||
SqlLiteral.createCharString(String.format("%s, x -> ROW(x)", fieldName), POS); | ||
|
||
// Generate expected recordType required for transformatioin |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: transformation
@Test(enabled = false, | ||
description = "pending migration to hive tables and corresponding queries to use standardised CoralSqlNode and CoralRelNode representations in the translation path") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like some of the tests pass if they are enabled, why do we need to migrate them?
ed92de9
to
fd5c41a
Compare
70202eb
to
00e4861
Compare
00e4861
to
68990bb
Compare
5fd9c18
to
3948cef
Compare
Update the CoralRelNode -> Trino SQL translation path to use
CoralRelToSqlNodeConverter
.Added
CoralSqlNodeToTrinoSqlNodeConverter
to enable conversion from CoralSqlNode to TrinoSqlNode.Testing:
(1) Updated / Added tests in HiveToTrinoConverterTest to validate hive -> trino SQL translations via new code path
Pending:
Migrate RelToTrinoConverterTest to use hive SQL as input.
Update expected trino SQLs in TrinoToRelConverterTest