[CALCITE-5530] RelToSqlConverter[ORDER BY] generates an incorrect fie… #3085
Conversation
…ld alias when 2 projection fields have the same name
JiajunBernoulli
force-pushed
the
CALCITE-5530
branch
from
1a337a1 to
2a7b9fd
Compare
There was a problem hiding this comment.
@JiajunBernoulli Thanks for your PR, I've left my comments.
core/src/main/java/org/apache/calcite/rel/rel2sql/SqlImplementor.java
Outdated
Show resolved
Hide resolved
| + "FROM \"scott\".\"EMP\"\n" | ||
| + "ORDER BY 2) AS \"t0\""; | ||
| assertThat(actualSql1, isLinux(expectedSql1)); | ||
| // SqlConformance#isSortByAlias is false for Spark. |
There was a problem hiding this comment.
SqlConformance is used by parser, not RelToSqlConverter, hence it does not affact anything here, you can see that your expected sql is still sorting by ordinal.
There was a problem hiding this comment.
Thank you for your careful review.
I added a new commit to solve it.
|
@JiajunBernoulli The tests seem still fail~ |
|
The PR also solve CALCITE-5656 Would you please review it again? @libenchao |
|
I'll do a final review |
| && clauses.contains(Clause.ORDER_BY) | ||
| && dialect.getConformance().isSortByOrdinal()) { | ||
| // Cannot merge a Project that contains sort by ordinal under it. | ||
| return hasSortByOrdinal(); |
There was a problem hiding this comment.
To be honest, I felt uneasy about moving this up. I'm not sure if we will miss some opportunity to add a new sub query. The changes in #3057 was also merged by me, and now I think that is not perfect as well, since it also suffers from this.
For needNewSubQuery, we should only bail out when we returns true. In that way, we could safely add new conditions into this method, and all the cases are orthogonal.
There was a problem hiding this comment.
Agree, we should return true avoid return false blocking other conditions.
| @@ -1791,6 +1791,15 @@ private boolean needNewSubQuery( | |||
| return false; | |||
| } | |||
| final Clause maxClause = Collections.max(clauses); | |||
|
|
|||
| if (rel instanceof Project | |||
There was a problem hiding this comment.
Is there any reason that you we need to move this block from L1819-L1825 to here? If not, I would suggest to keep it there to make the change of git history minimal and trackable.
There was a problem hiding this comment.
Moved to L1819-L1825
| * RelToSqlConverter[ORDER BY] generates an incorrect field alias | ||
| * when 2 projection fields have the same name</a>. | ||
| */ | ||
| @Test void testSortWithAnFieldNotInTheProjectionWithASameAliasAsThatInTheProjection() { |
There was a problem hiding this comment.
| @Test void testSortWithAnFieldNotInTheProjectionWithASameAliasAsThatInTheProjection() { | |
| @Test void testOrderByFieldNotInTheProjectionWithASameAliasAsThatInTheProjection() { |
| assertThat(actualSql2, isLinux(expectedSql2)); | ||
| } | ||
|
|
||
| @Test void testSortWithAnExpressionNotInTheProjectionThatRefersToUnderlyingFieldWithSameAlias() { |
There was a problem hiding this comment.
| @Test void testSortWithAnExpressionNotInTheProjectionThatRefersToUnderlyingFieldWithSameAlias() { | |
| @Test void testOrderByExpressionNotInTheProjectionThatRefersToUnderlyingFieldWithSameAlias() { |
|
Kudos, SonarCloud Quality Gate passed! |
…ld alias when 2 projection fields have the same name