Support `Utf8View` for `bit_length` kernel by austin362667 · Pull Request #6671 · apache/arrow-rs

austin362667 · 2024-11-01T18:30:33Z

Which issue does this PR close?

Rationale for this change

Thanks to @jayzhan211 , he noticed following issue, array compute kernel bit_length() doesn't support Utf8View type:

create table test_source as values
  ('Andrew', 'X'),
  ('Xiangpeng', 'Xiangpeng'),
  ('Raphael', 'R'),
  (NULL, 'R');
create table test as
SELECT
  arrow_cast(column1, 'Utf8View') as column1_utf8view
FROM test_source;

select bit_length(column1_utf8view) from test;

caused the error:

query error DataFusion error: Arrow error: Compute error: bit_length not supported for Utf8View

What changes are included in this PR?

Update bit_length() array function to support Utf8View

Are there any user-facing changes?

Signed-off-by: Austin Liu <[email protected]>

tustvold

I think this needs a test, which I think will show this not working

arrow-string/src/length.rs

Signed-off-by: Austin Liu <[email protected]> Add test & handle view bytes length counting Signed-off-by: Austin Liu <[email protected]>

alamb

Thanks @austin362667 -- this is looking good. I started the CI and left some comments

arrow-string/src/length.rs

Signed-off-by: Austin Liu <[email protected]>

alamb

Thanks @austin362667

alamb · 2024-11-02T13:34:51Z

arrow-string/src/length.rs

+            let bit_lengths = list
+                .views()
+                .iter()
+                .map(|view| (*view as i32) * 8)


I think the length is currently treated as a u32 elsewhere in the code

However the actual Arrow spec I think says use i32 so this is probably fine

I also still do think you need to handle the null cases (check for which slots are null) as there might be a view of a non zero length there

Sure! Thanks @alamb for review

Signed-off-by: Austin Liu <[email protected]>

arrow-string/src/length.rs

findepi · 2024-11-02T21:31:57Z

arrow-string/src/length.rs

+            let values = list
+                .views()
+                .iter()
+                .enumerate()
+                .map(|(i, _)| {
+                    if list.is_valid(i) {
+                        list.views()[i] as u32 * 8
+                    } else {
+                        0


In #6662 (comment) it was suggested to use from_unary.
It this applicable here?

@findepi Thanks for bringing this up. I tried it and it works trivially well.

Using from_unary reverts the optimisation of only inspecting the views, and not the string data.

Perhaps you might use the example I wrote in #6671 (comment)

arrow-string/src/length.rs

findepi · 2024-11-02T21:34:56Z

arrow-string/src/length.rs

                let result = bit_length(&array).unwrap();
                assert_eq!(len, result.len());
-                let result = result.as_any().downcast_ref::<Int32Array>().unwrap();
+                let result = result.as_any().downcast_ref::<UInt32Array>().unwrap();


I agree that bit_length should not beed to return signed integer, but it seems better not to change this within this PR.

arrow-string/src/length.rs

Signed-off-by: Austin Liu <[email protected]>

tustvold · 2024-11-04T14:43:11Z

arrow-string/src/length.rs

+            let values = (0..list.len())
+                .map(|i| {
+                    if list.is_valid(i) {
+                        list.views()[i] as u32 * 8
+                    } else {
+                        0
+                    }
+                })
+                .collect::<Vec<u32>>();
+            Ok(Arc::new(UInt32Array::new(
+                values.into(),


Suggested change

let values = (0..list.len())

.map(|i| {

if list.is_valid(i) {

list.views()[i] as u32 * 8

} else {

0

}

})

.collect::<Vec<u32>>();

Ok(Arc::new(UInt32Array::new(

values.into(),

let values = list.views().iter().map(|view| (view as i32).wrapping_mul(8)).collect();

Ok(Arc::new(Int32Array::new(

values,

As we're doing infallible wrapping arithmetic, we can just compute this for everything, this will vectorize better, and the null mask is preserved below

Wow! Thank you so much @tustvold , you are indeed making me a better Rust programmer. Really appreciate for review. 👍

tustvold

I think this can be further simplified, I also think we should return Int32Array for consistency with the other implementations.

Signed-off-by: Austin Liu <[email protected]>

austin362667 · 2024-11-04T17:25:12Z

Thanks @tustvold @alamb @findepi for the review.

I roll back to i32 rather than u32 to make code consistent.
Use from_unary suggested from @findepi
Add bit_length_null_utf8view test cases suggested by @tustvold .

tustvold · 2024-11-04T17:44:04Z

Switching to from_unary has reverted the optimisation to not inspect the string data - #6671 (comment) is how to do this correctly

Signed-off-by: Austin Liu <[email protected]>

austin362667 · 2024-11-04T18:10:07Z

Got it! Thanks @tustvold I replaced from_unary with the approach you suggested.

Signed-off-by: Austin Liu <[email protected]>

alamb

Looks great to me -- thank you @austin362667 and @tustvold and @findepi

Support Utf8View for string function bit_length()

507dcfa

Signed-off-by: Austin Liu <[email protected]>

github-actions bot added the arrow Changes to the arrow crate label Nov 1, 2024

austin362667 mentioned this pull request Nov 1, 2024

Support Utf8View for string function bit_length() apache/datafusion#13221

Merged

tustvold reviewed Nov 1, 2024

View reviewed changes

arrow-string/src/length.rs Outdated Show resolved Hide resolved

Add test & handle view bytes length counting

67201b7

Signed-off-by: Austin Liu <[email protected]> Add test & handle view bytes length counting Signed-off-by: Austin Liu <[email protected]>

alamb reviewed Nov 2, 2024

View reviewed changes

arrow-string/src/length.rs Outdated Show resolved Hide resolved

arrow-string/src/length.rs Outdated Show resolved Hide resolved

alamb mentioned this pull request Nov 2, 2024

Minor: Document pattern for accessing views in StringView #6673

Merged

Refine string_view_array

2ad7170

Signed-off-by: Austin Liu <[email protected]>

alamb reviewed Nov 2, 2024

View reviewed changes

Make length from i32 to u32 & check nullity

de5e33a

Signed-off-by: Austin Liu <[email protected]>

findepi reviewed Nov 2, 2024

View reviewed changes

Clean up

4ab6cfd

Signed-off-by: Austin Liu <[email protected]>

tustvold reviewed Nov 4, 2024

View reviewed changes

austin362667 added 2 commits November 5, 2024 01:09

Refine

cdfc465

Signed-off-by: Austin Liu <[email protected]>

Use from_unary instead

a3650ef

Signed-off-by: Austin Liu <[email protected]>

Prevent inspect the string data

098cf3e

Signed-off-by: Austin Liu <[email protected]>

tustvold approved these changes Nov 4, 2024

View reviewed changes

Clean up

2a0c8b2

Signed-off-by: Austin Liu <[email protected]>

alamb approved these changes Nov 5, 2024

View reviewed changes

alamb changed the title ~~Support Utf8View for string function bit_length()~~ Support Utf8View for bit_length kernel Nov 5, 2024

alamb merged commit 350ea26 into apache:master Nov 5, 2024

This was referenced Nov 9, 2024

Revisit bit_length Not Supporting Utf8View Array. apache/datafusion#13329

Closed

Length-related Functions Should Return Unsigned Integer #6708

Closed

alamb mentioned this pull request Dec 12, 2024

Patched DataFusion, Oct 15, 2024 influxdata/arrow-datafusion#47

Closed

Comments

Conversation

austin362667 commented Nov 1, 2024

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

alamb Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

alamb Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

austin362667 Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

austin362667 Nov 4, 2024

Choose a reason for hiding this comment

Uh oh!

tustvold Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

findepi Nov 2, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tustvold Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

austin362667 Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold left a comment

Choose a reason for hiding this comment

Uh oh!

austin362667 commented Nov 4, 2024

Uh oh!

tustvold commented Nov 4, 2024

Uh oh!

austin362667 commented Nov 4, 2024

Uh oh!

alamb left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tustvold Nov 4, 2024 •

edited

Loading

tustvold Nov 4, 2024 •

edited

Loading

austin362667 Nov 4, 2024 •

edited

Loading

alamb left a comment •

edited

Loading