perf: preserve dictionary encoding for character_length, initcap, and reverse - #23930
Conversation
github-actions
Bot
added
sqllogictest
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #23930 +/- ##
==========================================
- Coverage 80.88% 80.88% -0.01%
==========================================
Files 1101 1101
Lines 375720 375746 +26
Branches 375720 375746 +26
==========================================
+ Hits 303895 303914 +19
+ Misses 53729 53722 -7
- Partials 18096 18110 +14 ☔ 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?
There was a problem hiding this comment.
at this point its more a question of if we should keep the existing behaviour or not 🤔
it looks like we didnt rely on the previous behaviour on our test suite (?) but maybe a downstream user could have used it. i am in favour of having a stricter signature, but i know in general we've been limiting breaking changes where possible
(i do agree things like reversing the array looks off)
if we substitute in TypeSignatureClass::Any for TypeSignatureClass::Native(logical_binary()) does it keep the previous behaviour? then we could in a separate followup explicitly tighten the input signature so at least its more visible in its own PR
There was a problem hiding this comment.
I’ve switched both character_length and reverse to Any to restore the previous implicit coercions. One difference for reverse is that non-string inputs are now coerced to Utf8View instead of Utf8.
Tightening the signatures in a followup sounds good, especially since there may be other functions with similar issues.
| Self { | ||
| signature: Signature::uniform( | ||
| 1, | ||
| vec![Utf8View, Utf8, LargeUtf8], |
There was a problem hiding this comment.
similarly here it used to accept anything
| Self { | ||
| signature: Signature::uniform( | ||
| 1, | ||
| vec![Utf8, LargeUtf8, Utf8View], |
There was a problem hiding this comment.
at this point its more a question of if we should keep the existing behaviour or not 🤔
it looks like we didnt rely on the previous behaviour on our test suite (?) but maybe a downstream user could have used it. i am in favour of having a stricter signature, but i know in general we've been limiting breaking changes where possible
(i do agree things like reversing the array looks off)
if we substitute in TypeSignatureClass::Any for TypeSignatureClass::Native(logical_binary()) does it keep the previous behaviour? then we could in a separate followup explicitly tighten the input signature so at least its more visible in its own PR
| let args = &args.args; | ||
| match args[0].data_type() { | ||
| Utf8 | Utf8View | LargeUtf8 => make_scalar_function(reverse, vec![])(args), | ||
| Utf8 | Utf8View | LargeUtf8 | DataType::Dictionary(_, _) => { |
There was a problem hiding this comment.
we can probably just remove this datatype check here, its unnecessary since signature guards it for us
|
thanks @lyne7-sc |
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