fix: Optimize read_side_padding #772
Conversation
Before
After
|
kazuyukitanimura
requested review from
viirya,
andygrove,
comphead and
huaxingao
| @@ -0,0 +1,7 @@ | |||
| SELECT | |||
There was a problem hiding this comment.
how this test related to rpad? 🤔
There was a problem hiding this comment.
They are related as their schema types are CHAR()
There was a problem hiding this comment.
lgtm thanks @kazuyukitanimura and the benchmark results are promising
| if length <= char_len { | ||
| builder.append_value(string); |
There was a problem hiding this comment.
If the required len is less than string's length, don't we need to take substring of it? Spark RPad does it.
There was a problem hiding this comment.
Current implementation already has this issue.
There was a problem hiding this comment.
In the line 389 there is an existing comment
/// Similar to DataFusion `rpad`, but not to truncate when the string is already longer than length
Perhaps I should change the name of this method, this is not used for rpad
| // It looks Spark's UTF8String is closer to chars rather than graphemes | ||
| // https://stackoverflow.com/a/46290728 |
There was a problem hiding this comment.
Can you add an unit test for that?
kazuyukitanimura
changed the title
|
Merged, Thanks @comphead @viirya @andygrove |
kazuyukitanimura
changed the title
## Which issue does this PR close? ## Rationale for this change This PR improves read_side_padding that is used for CHAR() schema ## What changes are included in this PR? Optimized spark_read_side_padding ## How are these changes tested? Added tests (cherry picked from commit 457d9d1)
Which issue does this PR close?
Rationale for this change
This PR improves read_side_padding that is used for CHAR() schema
What changes are included in this PR?
Optimized spark_read_side_padding
How are these changes tested?
Added tests