[Coral-Trino] Change NullCollation.LAST to NullCollation.HIGH in TrinoSqlDialect #450
Conversation
KevinGe00
changed the title
| protected boolean condition(SqlCall sqlCall) { | ||
| return sqlCall.getOperator().kind == SqlKind.NULLS_LAST && sqlCall.operand(0).getKind() == SqlKind.DESCENDING; | ||
| } | ||
|
|
||
| @Override | ||
| protected SqlCall transform(SqlCall sqlCall) { | ||
| final List<SqlNode> sourceOperands = sqlCall.getOperandList(); | ||
| String orderBy = sourceOperands.get(0).getKind().toString(); | ||
| if (orderBy == "DESCENDING" && sqlCall.operandCount() > 0) { | ||
| // drop redundant "NULLS LAST" | ||
| return sqlCall.operand(0); | ||
| } | ||
|
|
||
| return sqlCall; | ||
| } |
There was a problem hiding this comment.
We may change condition to be return sqlCall.getOperator().kind == SqlKind.NULLS_LAST && sqlCall.operandCount() > 0 && sqlCall.operand(0).getKind() == SqlKind.DESCENDING, then transform can be modified to return sqlCall.operand(0);.
There was a problem hiding this comment.
made the change! thanks
| return sqlCall.getOperator().kind == SqlKind.NULLS_LAST && sqlCall.operand(0).getKind() == SqlKind.DESCENDING | ||
| && sqlCall.operandCount() > 0; |
There was a problem hiding this comment.
sqlCall.operandCount() > 0 should be put before sqlCall.operand(0).getKind() == SqlKind.DESCENDING?
...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/NullOrderingTransformer.java
Show resolved
Hide resolved
...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/NullOrderingTransformer.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
LGTM, thanks for working on the PR @KevinGe00.
Left minor comments for documentation.
|
|
||
| RelNode relNode = TestUtils.getHiveToRelConverter() | ||
| .convertSql("SELECT a, SUBSTR(b, 1, 1) AS aliased_column, c FROM test.tabler ORDER BY aliased_column ASC"); | ||
| // We want NULLS FIRST since we're translating from Hive and that is the default null ordering for ASC in Hive |
There was a problem hiding this comment.
is this same as existing behaviour?
There was a problem hiding this comment.
Yes, current behavior is no explicit ASC and with NULLS FIRST since it needs to overwrite the default NULLS LAST of trino
...o/src/main/java/com/linkedin/coral/trino/rel2trino/transformers/NullOrderingTransformer.java
Outdated
Show resolved
Hide resolved
What changes are proposed in this pull request, and why are they necessary?
We're changing the null collation of
TrinoSqlDialectto from NullCollation.LAST to NullCollation.HIGH sinceCoralRelToSqlNodeConverteruse NullCollation.HIGH. This is an incremental piece that works towards the goal of eventually getting rid ofTrinoSqlDialectand usingCoralRelToSqlNodeConverteras the common coral dialect.How was this patch tested?
New unit test:
testAliasOrderByASCTesting that the translated Trino sql explicitly says NULLS FIRST since we're translating from Hive and that is the default null ordering for ASC in Hive
./gradlew clean build
waiting on i-tests