julianhyde commented on code in PR #3604:
URL: https://github.com/apache/calcite/pull/3604#discussion_r1438446393
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4108,6 +4108,19 @@ static void checkRlikeFails(SqlOperatorFixture f) {
f.checkNull("CHARACTER_LENGTH(cast(null as varchar(1)))");
}
+ @Test void testLenFunc() {
+ final SqlOperatorFixture f0 = fixture().setFor(SqlLibraryOperators.LENGTH);
+ f0.checkFails("^length('hello')^",
+ "No match found for function signature LENGTH\\(<CHARACTER>\\)",
+ false);
+ final SqlOperatorFixture f = f0.withLibrary(SqlLibrary.SNOWFLAKE);
+ f.checkScalar("length('hello')", "5", "INTEGER NOT NULL");
+ f.checkScalar("length('')", "0", "INTEGER NOT NULL");
+ f.checkScalar("length(CAST('x' as CHAR(3)))", "3", "INTEGER NOT NULL");
+ f.checkScalar("length(CAST('x' as VARCHAR(4)))", "1", "INTEGER NOT NULL");
+ f.checkNull("length(CAST(NULL as CHAR(5)))");
Review Comment:
None of these tests has a call to 'len(...)'. Maybe LEN is getting tested
via some aliasing mechanism. It's difficult to tell.
So, let's have at least one test for LEN. So we can still trust the test
even if the clever stuff fails.
##########
core/src/test/java/org/apache/calcite/rel/rel2sql/RelToSqlConverterTest.java:
##########
@@ -6696,6 +6696,20 @@ private void checkLiteral2(String expression, String
expected) {
sql(query).withLibrary(SqlLibrary.SNOWFLAKE).withSnowflake().ok(expectedSnowflake);
}
+ /** Test case for
+ * <a
href="https://issues.apache.org/jira/browse/CALCITE-6182";>[CALCITE-6182]
+ * Add LENGTH/LEN functions (enabled in Snowflake library)</a>. */
+ @Test void testSnowflakeLength() {
+ final String query = "select CHAR_LENGTH(\"brand_name\")\n"
+ + "from \"product\"";
+ final String expectedBigQuery = "SELECT CHAR_LENGTH(brand_name)\n"
+ + "FROM foodmart.product";
+ final String expectedSnowflake = "SELECT LENGTH(\"brand_name\")\n"
+ + "FROM \"foodmart\".\"product\"";
+
sql(query).withLibrary(SqlLibrary.BIG_QUERY).withBigQuery().ok(expectedBigQuery);
+
sql(query).withLibrary(SqlLibrary.BIG_QUERY).withSnowflake().ok(expectedSnowflake);
Review Comment:
can you combine those lines? you only need to parse the query once. then you
generate SQL for two dialects.
the structure of the test should make it clear that people can other target
dialects too.
I don't see any mention of LEN in the test. Not that there should be. It's
ok to generate LENGTH for Snowflake. But it needs explanation, given that the
jira case mentions LEN.
##########
testkit/src/main/java/org/apache/calcite/test/SqlOperatorTest.java:
##########
@@ -4108,6 +4108,19 @@ static void checkRlikeFails(SqlOperatorFixture f) {
f.checkNull("CHARACTER_LENGTH(cast(null as varchar(1)))");
}
+ @Test void testLenFunc() {
Review Comment:
Could use a comment - that LEN is a snowflake-specific alias for
CHAR_LENGTH, and that snowflake also supports LENGTH
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
