From d46dc30768acfa624aea521836a8ab98a7765dfe Mon Sep 17 00:00:00 2001 From: Tanner Clary Date: Wed, 20 Dec 2023 08:00:04 -0800 Subject: [PATCH] Respond to review comments --- .../sql/dialect/SnowflakeSqlDialect.java | 26 +++++----------- .../apache/calcite/sql/fun/SqlLibrary.java | 8 ++--- .../rel/rel2sql/RelToSqlConverterTest.java | 30 +++++++++++-------- .../org/apache/calcite/util/UtilTest.java | 8 ++--- site/_docs/reference.md | 6 ++-- .../apache/calcite/test/SqlOperatorTest.java | 1 + 6 files changed, 38 insertions(+), 41 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/sql/dialect/SnowflakeSqlDialect.java b/core/src/main/java/org/apache/calcite/sql/dialect/SnowflakeSqlDialect.java index 6303ed7a6db3..32c2874298c3 100644 --- a/core/src/main/java/org/apache/calcite/sql/dialect/SnowflakeSqlDialect.java +++ b/core/src/main/java/org/apache/calcite/sql/dialect/SnowflakeSqlDialect.java @@ -19,7 +19,6 @@ import org.apache.calcite.avatica.util.Casing; import org.apache.calcite.sql.SqlCall; import org.apache.calcite.sql.SqlDialect; -import org.apache.calcite.sql.SqlKind; import org.apache.calcite.sql.SqlWriter; import org.apache.calcite.sql.fun.SqlLibraryOperators; import org.apache.calcite.sql.parser.SqlParserPos; @@ -43,31 +42,22 @@ public SnowflakeSqlDialect(Context context) { @Override public void unparseCall(final SqlWriter writer, final SqlCall call, final int leftPrec, final int rightPrec) { - switch(call.getKind()) { + switch (call.getKind()) { case ENDS_WITH: + SqlCall endsWithCall = SqlLibraryOperators.ENDSWITH + .createCall(SqlParserPos.ZERO, call.getOperandList()); + super.unparseCall(writer, endsWithCall, leftPrec, rightPrec); + break; case STARTS_WITH: - unparseEndsStartsWith(writer, call); + SqlCall startsWithCall = SqlLibraryOperators.STARTSWITH + .createCall(SqlParserPos.ZERO, call.getOperandList()); + super.unparseCall(writer, startsWithCall, leftPrec, rightPrec); break; default: super.unparseCall(writer, call, leftPrec, rightPrec); } } - /** - * Remove underscore for {@code STARTS_WITH} and {@code ENDS_WITH} operators to - * comply with Snowflake syntax. - */ - private static void unparseEndsStartsWith(final SqlWriter writer, final SqlCall call) { - final SqlCall - if (call.getKind() == SqlKind.STARTS_WITH) { - SqlLibraryOperators.STARTSWITH.createCall(SqlParserPos.ZERO, call.getOperandList()) - } - final SqlWriter.Frame frame = writer.startFunCall(name); - call.operand(0).unparse(writer, 0, 0); - call.operand(1).unparse(writer, 0, 0); - writer.endFunCall(frame); - } - @Override public boolean supportsApproxCountDistinct() { return true; } diff --git a/core/src/main/java/org/apache/calcite/sql/fun/SqlLibrary.java b/core/src/main/java/org/apache/calcite/sql/fun/SqlLibrary.java index 1f981151a484..3f95150608bc 100644 --- a/core/src/main/java/org/apache/calcite/sql/fun/SqlLibrary.java +++ b/core/src/main/java/org/apache/calcite/sql/fun/SqlLibrary.java @@ -71,11 +71,11 @@ public enum SqlLibrary { /** A collection of operators that are in PostgreSQL but not in standard * SQL. */ POSTGRESQL("p", "postgresql"), + /** A collection of operators that are in Snowflake but not in standard SQL. */ + SNOWFLAKE("f", "snowflake"), /** A collection of operators that are in Apache Spark but not in standard * SQL. */ - SPARK("s", "spark"), - /** A collection of operators that are in Snowflake but not in standard SQL. */ - SNOWFLAKE("s", "snowflake"); + SPARK("s", "spark"); /** Map from {@link Enum#name() name} and {@link #fun} to library. */ public static final Map MAP; @@ -99,7 +99,7 @@ public List children() { switch (this) { case ALL: return ImmutableList.of(BIG_QUERY, CALCITE, HIVE, MSSQL, MYSQL, ORACLE, - POSTGRESQL, SPARK); + POSTGRESQL, SNOWFLAKE, SPARK); default: return ImmutableList.of(); } diff --git a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java index 31ebe2f7b22e..7561e48cce62 100644 --- a/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java +++ b/core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java @@ -6561,28 +6561,34 @@ private void checkLiteral2(String expression, String expected) { * [CALCITE-6156] * Add ENDSWITH, STARTSWITH functions (enabled in Postgres, Snowflake libraries). */ @Test void testSnowflakeStartsWith() { - final String bigQuerySql = "select starts_with(\"brand_name\", 'a')\n" + final String query = "select startswith(\"brand_name\", 'a')\n" + "from \"product\""; - final String snowflakeSql = "select startswith(\"brand_name\", 'a')\n" - + "from \"product\""; - final String expected = "SELECT STARTSWITH(\"brand_name\" 'a')\n" + final String expectedBigQuery = "SELECT STARTS_WITH(brand_name, 'a')\n" + + "FROM foodmart.product"; + final String expectedPostgres = "SELECT STARTS_WITH(\"brand_name\", 'a')\n" + + "FROM \"foodmart\".\"product\""; + final String expectedSnowflake = "SELECT STARTSWITH(\"brand_name\", 'a')\n" + "FROM \"foodmart\".\"product\""; - sql(bigQuerySql).withLibrary(SqlLibrary.BIG_QUERY).withSnowflake().ok(expected); - sql(snowflakeSql).withLibrary(SqlLibrary.SNOWFLAKE).withSnowflake().ok(expected); + sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withBigQuery().ok(expectedBigQuery); + sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withPostgresql().ok(expectedPostgres); + sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withSnowflake().ok(expectedSnowflake); } /** Test case for * [CALCITE-6156] * Add ENDSWITH, STARTSWITH functions (enabled in Postgres, Snowflake libraries). */ @Test void testSnowflakeEndsWith() { - final String bigQuerySql = "select ends_with(\"brand_name\", 'a')\n" + final String query = "select endswith(\"brand_name\", 'a')\n" + "from \"product\""; - final String snowflakeSql = "select endswith(\"brand_name\", 'a')\n" - + "from \"product\""; - final String expected = "SELECT ENDSWITH(\"brand_name\" 'a')\n" + final String expectedBigQuery = "SELECT ENDS_WITH(brand_name, 'a')\n" + + "FROM foodmart.product"; + final String expectedPostgres = "SELECT ENDS_WITH(\"brand_name\", 'a')\n" + + "FROM \"foodmart\".\"product\""; + final String expectedSnowflake = "SELECT ENDSWITH(\"brand_name\", 'a')\n" + "FROM \"foodmart\".\"product\""; - sql(bigQuerySql).withLibrary(SqlLibrary.BIG_QUERY).withSnowflake().ok(expected); - sql(snowflakeSql).withLibrary(SqlLibrary.SNOWFLAKE).withSnowflake().ok(expected); + sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withBigQuery().ok(expectedBigQuery); + sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withPostgresql().ok(expectedPostgres); + sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withSnowflake().ok(expectedSnowflake); } @Test void testSubstringInSpark() { diff --git a/core/src/test/java/org/apache/calcite/util/UtilTest.java b/core/src/test/java/org/apache/calcite/util/UtilTest.java index 78deae0229e7..2802e561c83f 100644 --- a/core/src/test/java/org/apache/calcite/util/UtilTest.java +++ b/core/src/test/java/org/apache/calcite/util/UtilTest.java @@ -951,16 +951,16 @@ private List makeConsList(int start, int end) { assertThat(SqlLibrary.expand(ImmutableList.of(a)), hasToString("[ALL, BIG_QUERY, CALCITE, HIVE, MSSQL, MYSQL, ORACLE, " - + "POSTGRESQL, SPARK]")); + + "POSTGRESQL, SNOWFLAKE, SPARK]")); assertThat(SqlLibrary.expand(ImmutableList.of(a, c)), hasToString("[ALL, BIG_QUERY, CALCITE, HIVE, MSSQL, MYSQL, ORACLE, " - + "POSTGRESQL, SPARK]")); + + "POSTGRESQL, SNOWFLAKE, SPARK]")); assertThat(SqlLibrary.expand(ImmutableList.of(c, a)), hasToString("[CALCITE, ALL, BIG_QUERY, HIVE, MSSQL, MYSQL, ORACLE, " - + "POSTGRESQL, SPARK]")); + + "POSTGRESQL, SNOWFLAKE, SPARK]")); assertThat(SqlLibrary.expand(ImmutableList.of(c, o, a)), hasToString("[CALCITE, ORACLE, ALL, BIG_QUERY, HIVE, MSSQL, MYSQL, " - + "POSTGRESQL, SPARK]")); + + "POSTGRESQL, SNOWFLAKE, SPARK]")); assertThat(SqlLibrary.expand(ImmutableList.of(o, c, o)), hasToString("[ORACLE, CALCITE]")); diff --git a/site/_docs/reference.md b/site/_docs/reference.md index 20a845ad57f0..4a82fa3c6167 100644 --- a/site/_docs/reference.md +++ b/site/_docs/reference.md @@ -2628,13 +2628,13 @@ The 'C' (compatibility) column contains value: * '*' for all libraries, * 'b' for Google BigQuery ('fun=bigquery' in the connect string), * 'c' for Apache Calcite ('fun=calcite' in the connect string), +* 'f' for Snowflake ('fun=snowflake' in the connect string), * 'h' for Apache Hive ('fun=hive' in the connect string), * 'm' for MySQL ('fun=mysql' in the connect string), * 'q' for Microsoft SQL Server ('fun=mssql' in the connect string), * 'o' for Oracle ('fun=oracle' in the connect string), * 'p' for PostgreSQL ('fun=postgresql' in the connect string), -* 's' for Apache Spark ('fun=spark' in the connect string), -* 'f' for Snowflake ('fun=snowflake' in the connect string). +* 's' for Apache Spark ('fun=spark' in the connect string). One operator name may correspond to multiple SQL dialects, but with different semantics. @@ -2826,7 +2826,7 @@ BigQuery's type system uses confusingly different names for types and functions: | s | SOUNDEX(string) | Returns the phonetic representation of *string*; return original *string* if *string* is encoded with multi-byte encoding such as UTF-8 | m | SPACE(integer) | Returns a string of *integer* spaces; returns an empty string if *integer* is less than 1 | b | SPLIT(string [, delimiter ]) | Returns the string array of *string* split at *delimiter* (if omitted, default is comma). If the *string* is empty it returns an empty array, otherwise, if the *delimiter* is empty, it returns an array containing the original *string*. -| f | STARTSWITH(string1, string2) | Returns whether *string2* is a prefix of*string1* +| f | STARTSWITH(string1, string2) | Returns whether *string2* is a prefix of *string1* | b p | STARTS_WITH(string1, string2) | Equivalent to `STARTSWITH(string1, string2)` | m | STRCMP(string, string) | Returns 0 if both of the strings are same and returns -1 when the first argument is smaller than the second and 1 when the second one is smaller than the first one | b p | STRPOS(string, substring) | Equivalent to `POSITION(substring IN string)` diff --git a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java index 6905817f6521..2e2d0f730446 100644 --- a/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java +++ b/testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java @@ -8953,6 +8953,7 @@ private void testCurrentDateFunc(Pair pair) { f.checkBoolean("ends_with(x'', x'123456')", false); f.checkBoolean("ends_with(x'', x'')", true); }; + f0.forEachLibrary(list(SqlLibrary.BIG_QUERY, SqlLibrary.POSTGRESQL), consumer); } @Test void testSnowflakeEndsWithFunction() {