perf: preserve dictionary encoding for character_length, initcap, and reverse - #23930
perf: preserve dictionary encoding for character_length, initcap, and reverse#23930lyne7-sc wants to merge 5 commits into
character_length, initcap, and reverse#23930Conversation
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23930 +/- ##
==========================================
+ Coverage 80.67% 80.75% +0.08%
==========================================
Files 1095 1096 +1
Lines 372376 373312 +936
Branches 372376 373312 +936
==========================================
+ Hits 300397 301457 +1060
+ Misses 54052 53869 -183
- Partials 17927 17986 +59 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| Self { | ||
| signature: Signature::uniform( | ||
| 1, | ||
| vec![Utf8, LargeUtf8, Utf8View], |
There was a problem hiding this comment.
i think this allowed any type;
> select character_length(10);
+-----------------------------+
| character_length(Int64(10)) |
+-----------------------------+
| 2 |
+-----------------------------+
1 row(s) fetched.
Elapsed 0.004 seconds.There was a problem hiding this comment.
yeah, this narrows the set of accepted inputs, but some of the implicit coercions allowed by the previous signatures don’t seem very intuitive to me. For example, reverse([1, 2, 3]) returns "]3 ,2 ,1[", and reverse(true) returns "eurt".
I checked postgresql and duckdb, and both require an explicit string cast here. This behavior seems more reasonable to me.
select reverse(true);
ERROR: function reverse(boolean) does not exist
HINT: You might need to add explicit type casts.
select reverse(ARRAY[1,2,3]);
ERROR: function reverse(integer[]) does not exist
HINT: You might need to add explicit type casts.
so I’m not sure whether we should keep the existing implicit conversions or require an explicit cast?
| if let DataType::Utf8View = data_type { | ||
| Ok(DataType::Utf8View) | ||
| } else { | ||
| utf8_to_str_type(data_type, "initcap") | ||
| } |
There was a problem hiding this comment.
| if let DataType::Utf8View = data_type { | |
| Ok(DataType::Utf8View) | |
| } else { | |
| utf8_to_str_type(data_type, "initcap") | |
| } | |
| data_type |
i think we can clean this up like so, since we're just preserving input utf8 type
There was a problem hiding this comment.
That makes sense, initcap preserves the whole input type here.
| exec_err!("Unsupported data type {other:?} for function `initcap`") | ||
| } | ||
| } | ||
| let ColumnarValue::Array(array) = arg else { |
There was a problem hiding this comment.
would prefer to refactor this function into a match statement so we dont need this destructuring (which introduces an unreachable)
| Self { | ||
| signature: Signature::uniform( | ||
| 1, | ||
| vec![Utf8View, Utf8, LargeUtf8], |
There was a problem hiding this comment.
similarly here it used to accept anything
3 participants
Which issue does this PR close?
CoercionAPI #19458 and String scalar functions should preserve Dictionary encoding for Dictionary-typed inputs #20935Rationale for this change
Previously, coercion materialized dictionary-encoded inputs for
character_length,initcap, andreverse. This lost the encoding and evaluated the function for every row instead of once per dictionary value entry.This PR extends dictionary preservation to these functions.
character_lengthandreversenow useCoerciblesignatures to explicitly model string inputs and binary-to-string coercion while preserving dictionary encoding.What changes are included in this PR?
character_length,initcap, andreverse.character_lengthandreversefromUniformtoCoercible.Are these changes tested?
Yes, covered by SQL logic tests.
Are there any user-facing changes?
Yes. These functions now preserve dictionary encoding in their output.
character_lengthandreversenow accept logical string and binary inputs instead of implicitly converting unrelated types to strings.Benchmark