perf: optimize spark_hex dictionary path by avoiding dictionary expansion#19832
perf: optimize spark_hex dictionary path by avoiding dictionary expansion#19832Jefffrey merged 6 commits intoapache:mainfrom
spark_hex dictionary path by avoiding dictionary expansion#19832Conversation
| let encoded_values_array: ArrayRef = match encoded_values { | ||
| ColumnarValue::Array(a) => a, | ||
| ColumnarValue::Scalar(s) => Arc::new(s.to_array()?), | ||
| }; |
There was a problem hiding this comment.
We should probably refactor hex_encode_bytes and hex_encode_int64 to return arrays only, as their signature say they return ColumnarValue but they never return the scalar variant, forcing handling like this
| } | ||
| DataType::Dictionary(_, value_type) => { | ||
| DataType::Dictionary(_, _) => { | ||
| let dict = as_dictionary_array::<Int32Type>(&array); |
There was a problem hiding this comment.
nit: we should have some check that the dictionary has i32 key type, otherwise this will panic
| let dict_values = dict.values(); | ||
|
|
||
| match **value_type { | ||
| let encoded_values: ColumnarValue = match dict_values.data_type() { |
There was a problem hiding this comment.
We might want to consider arms for LargeUtf, views, etc.
| FROM VALUES ('foo'), ('bar'), ('foo'), (NULL), ('baz'), ('bar'); | ||
|
|
||
| query T | ||
| SELECT hex(dict_col) FROM t_dict_utf8; |
There was a problem hiding this comment.
Can we check the output type here with arrow_typeof to ensure they are dictionaries
There was a problem hiding this comment.
After running the SLT tests for hex, it seems the planner might unpack dictionary-encoded inputs like Dictionary(Int32, Utf8) or Dictionary(Int32, Int64) into their underlying types (Utf8View or Int64) before calling the function. However, Dictionary(Binary) appears to stay as a dictionary
logical_plan
01)Projection: arrow_typeof(hex(CAST(t_dict_utf8.dict_col AS Utf8View)))
physical_plan
01)ProjectionExec: expr=[arrow_typeof(hex(CAST(dict_col@0 AS Utf8View)))]
logical_plan
01)Projection: arrow_typeof(hex(t_dict_binary.dict_col))
physical_plan
01)ProjectionExec: expr=[arrow_typeof(hex(dict_col@0))]
There was a problem hiding this comment.
I guess it's related to this issue
We can still push through with this PR even though it only works for binary (we can change the tests to binary here)
There was a problem hiding this comment.
Thank you for the clarification. the tests have been updated to use dictionary(binary).
|
Thanks @lyne7-sc |
…ansion (apache#19832) ## Which issue does this PR close? <!-- We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes apache#123` indicates that this PR will close issue apache#123. --> Follow up to apache#19738 ## Rationale for this change <!-- Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed. Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes. --> The current hex implementation expands `DictionaryArray` inputs into a regular array, which causes loss of dictionary encoding and redundant hex computation for repeated values. ## What changes are included in this PR? - Apply hex encoding only to dictionary values - Avoid expanding dictionary arrays during execution ### Benchmark | Size | Before | After | Speedup | | ---- | ------ | ----- | ------- | | 1024 | 8.3 µs | 7.2 µs | 1.15× | | 4096 | 42.9 µs | 34.5 µs | 1.24× | | 8192 | 91.6 µs | 71.7 µs | 1.28× | <!-- There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR. --> ## Are these changes tested? Yes. Existing unit tests and `sqllogictest` tests pass. <!-- We typically require tests for all PRs in order to: 1. Prevent the code from being accidentally broken by subsequent changes 2. Serve as another way to document the expected behavior of the code If tests are not included in your PR, please explain why (for example, are they covered by existing tests)? --> ## Are there any user-facing changes? No. <!-- If there are user-facing changes then we may require documentation to be updated before approving the PR. --> <!-- If there are any breaking changes to public APIs, please add the `api change` label. -->
2 participants
Which issue does this PR close?
Follow up to #19738
Rationale for this change
The current hex implementation expands
DictionaryArrayinputs into a regular array, which causes loss of dictionary encoding and redundant hex computation for repeated values.What changes are included in this PR?
Benchmark
Are these changes tested?
Yes. Existing unit tests and
sqllogictesttests pass.Are there any user-facing changes?
No.