[GitHub] [calcite] JiajunBernoulli commented on a diff in pull request #3085: [CALCITE-5530] RelToSqlConverter[ORDER BY] generates an incorrect fie…

["JiajunBernoulli (via GitHub)" <gi...@apache.org>]

JiajunBernoulli commented on code in PR #3085:
URL: https://github.com/apache/calcite/pull/3085#discussion_r1155275105


##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -879,6 +879,80 @@ private static String toSql(RelNode root, SqlDialect dialect,
         .withPresto().ok(expectedPresto);
   }
 
+  /**
+   * Test case for
+   * <a href="https://issues.apache.org/jira/browse/CALCITE-5530">[CALCITE-5530]
+   * RelToSqlConverter[ORDER BY] generates an incorrect field alias
+   * when 2 projection fields have the same name</a>.
+   */
+  @Test void testSortWithAnFieldNotInTheProjectionWithASameAliasAsThatInTheProjection() {
+    final RelBuilder builder = relBuilder();
+    final RelNode base = builder
+        .scan("EMP")
+        .project(
+            builder.alias(
+                builder.call(SqlStdOperatorTable.UPPER, builder.field("ENAME")), "EMPNO"),
+            builder.field("EMPNO")
+        )
+        .sort(1)
+        .project(builder.field(0))
+        .build();
+
+    // The expected string should deliberately have a subquery to handle a scenario in which
+    // the projection field has an alias with the same name as that of the field used in the
+    // ORDER BY
+    String expectedSql1 = ""
+        + "SELECT \"EMPNO\"\n"
+        + "FROM (SELECT UPPER(\"ENAME\") AS \"EMPNO\", \"EMPNO\" AS \"EMPNO0\"\n"
+        + "FROM \"scott\".\"EMP\"\n"
+        + "ORDER BY 2) AS \"t0\"";
+    String actualSql1 = toSql(base);
+    assertThat(actualSql1, isLinux(expectedSql1));
+    // SqlConformance#isSortByAlias is false for Spark.
+    String actualSql2 = toSql(base, DatabaseProduct.SPARK.getDialect());
+    String expectedSql2 = "SELECT EMPNO\n"
+        + "FROM (SELECT UPPER(ENAME) EMPNO, EMPNO EMPNO0\n"
+        + "FROM scott.EMP\n"
+        + "ORDER BY 2 NULLS LAST) t0";
+    assertThat(actualSql2, isLinux(expectedSql2));
+  }
+
+  @Test void testSortWithAnExpressionNotInTheProjectionThatRefersToUnderlyingFieldWithSameAlias() {
+    final RelBuilder builder = relBuilder();
+    final RelNode base = builder
+        .scan("EMP")
+        .project(
+            builder.alias(
+                builder.call(SqlStdOperatorTable.UPPER, builder.field("ENAME")), "EMPNO"),
+            builder.call(
+                SqlStdOperatorTable.PLUS, builder.field("EMPNO"),
+                builder.literal(1)
+            )
+        )
+        .sort(1)
+        .project(builder.field(0))
+        .build();
+
+    // An output such as
+    // "SELECT UPPER(\"ENAME\") AS \"EMPNO\"\nFROM \"scott\".\"EMP\"\nORDER BY \"EMPNO\" + 1"
+    // would be incorrect since the rel is sorting by the field \"EMPNO\" + 1 in which EMPNO
+    // refers to the physical column EMPNO and not the alias
+    String actualSql1 = toSql(base);
+    String expectedSql1 = ""
+        + "SELECT \"EMPNO\"\n"
+        + "FROM (SELECT UPPER(\"ENAME\") AS \"EMPNO\", \"EMPNO\" + 1 AS \"$f1\"\n"
+        + "FROM \"scott\".\"EMP\"\n"
+        + "ORDER BY 2) AS \"t0\"";
+    assertThat(actualSql1, isLinux(expectedSql1));
+    // SqlConformance#isSortByAlias is false for Spark.

Review Comment:
   Thank you for your careful review.
   
   I added a new commit to solve it.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@calcite.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org