[CALCITE-6182] Add LENGTH/LEN functions (enabled in Snowflake library)#3604
[CALCITE-6182] Add LENGTH/LEN functions (enabled in Snowflake library)#3604tanclary merged 1 commit intoapache:mainfrom
Conversation
|
Should this wait for #3590? |
@mihaibudiu Yes going to rebase once thats merged. |
tanclary
force-pushed
the
snowflake-length
branch
2 times, most recently
from
33fa420 to
b5e23f8
Compare
julianhyde
left a comment
julianhyde
left a comment
There was a problem hiding this comment.
I suggested a few clarifying comments.
Also I would like clarity whether this will go in before or after the fix for CALCITE-6172. Before is probably better. But if before, then 6172 should add tests for LEN and LENGTH as aliases of CHAR_LENGTH.
| f.checkNull("CHARACTER_LENGTH(cast(null as varchar(1)))"); | ||
| } | ||
|
|
||
| @Test void testLenFunc() { |
There was a problem hiding this comment.
Could use a comment - that LEN is a snowflake-specific alias for CHAR_LENGTH, and that snowflake also supports LENGTH
| 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)))"); |
There was a problem hiding this 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.
There was a problem hiding this comment.
I just forgot to change them, fixed now
| 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); |
There was a problem hiding this 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.
There was a problem hiding this comment.
Added a comment and combined lines
tanclary
force-pushed
the
snowflake-length
branch
from
74573dc to
de676bb
Compare
|
3 participants
Enabling LENGTH for Snowflake and adding the LEN alias