Support binary data type in `build_struct_array`. #702

zijie0 · 2021-08-20T02:10:44Z

Which issue does this PR close?

Closes #701

Rationale for this change

We encounter this issue in delta-rs project where partitionValues in delta spec could contain binary type values.

What changes are included in this PR?

Add binary type support and corresponding test case.

Are there any user-facing changes?

No.

codecov-commenter · 2021-08-20T04:30:14Z

Codecov Report

Attention: Patch coverage is 91.30435% with 2 lines in your changes missing coverage. Please review.

Project coverage is 82.46%. Comparing base (38e5d22) to head (29e63da).
Report is 2837 commits behind head on master.

Files with missing lines	Patch %	Lines
arrow/src/json/reader.rs	90.90%	2 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #702   +/-   ##
=======================================
  Coverage   82.45%   82.46%           
=======================================
  Files         168      168           
  Lines       47397    47419   +22     
=======================================
+ Hits        39083    39103   +20     
- Misses       8314     8316    +2

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jorgecarleitao

Thanks!

Out of curiosity, isn't json always valid utf8?

jorgecarleitao · 2021-08-20T07:14:16Z

arrow/src/array/array.rs

@@ -668,7 +668,7 @@ mod tests {
                "entry",
                DataType::Struct(vec![
                    Field::new("key", DataType::Utf8, false),
-                    Field::new("key", DataType::Int32, true),
+                    Field::new("value", DataType::Int32, true),


An oversight on my end, should have been "value"

Haha, learning from previous PR and found this typo ;)

arrow/src/json/reader.rs

nevi-me

LGTM

nevi-me · 2021-08-20T07:38:03Z

arrow/src/array/array.rs

@@ -668,7 +668,7 @@ mod tests {
                "entry",
                DataType::Struct(vec![
                    Field::new("key", DataType::Utf8, false),
-                    Field::new("key", DataType::Int32, true),
+                    Field::new("value", DataType::Int32, true),


An oversight on my end, should have been "value"

nevi-me · 2021-08-20T07:40:14Z

arrow/src/json/reader.rs

+                        rows.iter()
+                            .map(|row| {
+                                let maybe_value = row.get(field.name());
+                                maybe_value.and_then(|value| value.as_str())


I thought value.as_str() would return an error if the binary data can't be converted to UTF8, but then we probably wouldn't be able to read the file in that case.

Just an observation, no action needed

nevi-me · 2021-08-20T07:43:08Z

Thanks!

Out of curiosity, isn't json always valid utf8?

Yeah, I've only ever base64 encoded to a string, so I wonder if there'd be some data loss if we don't do some conversion to a string representation

alamb · 2021-08-20T20:28:21Z

I pushed a change to fix a cargo fmt nitpick and once this PR passes CI I plan to merge it in. Thanks again @zijie0 !

zijie0 · 2021-08-21T00:38:17Z

Thank you all! Feeling excited to become a new contributor.

alamb · 2021-08-21T10:34:15Z

Looks great -- thank you again @zijie0 -- I'll plan to include this as part of arrow 5.3.0 (eta release in about 10 days time)

* Support binary data type in `build_struct_array`. * Modify test case. * cargo fmt Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

houqp · 2021-08-22T23:19:58Z

Yeah, I've only ever base64 encoded to a string, so I wonder if there'd be some data loss if we don't do some conversion to a string representation

I think writer needs to take care of the encoding and the reader needs to decode accordingly. For example encoding arbitrary binary into base64 is one way to do this. Another way is to encode binary as 2 bytes unicode code point. Whatever the encoding/decoding strategy gets picked, the writer is responsible to encode the binary into valid utf8 string. So it's really just a convention that the writer and reader needs to follow.

* Support binary data type in `build_struct_array`. (#702) * Support binary data type in `build_struct_array`. * Modify test case. * cargo fmt Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org> * Remove accidentally included test Co-authored-by: Yuan Zhou <myoilbox@gmail.com>

Support binary data type in build_struct_array.

498e15d

github-actions bot added the arrow Changes to the arrow crate label Aug 20, 2021

houqp requested review from nevi-me and Dandandan August 20, 2021 04:13

houqp approved these changes Aug 20, 2021

View reviewed changes

jorgecarleitao mentioned this pull request Aug 20, 2021

Added support to read json to BinaryArray jorgecarleitao/arrow2#304

Merged

jorgecarleitao approved these changes Aug 20, 2021

View reviewed changes

nevi-me approved these changes Aug 20, 2021

View reviewed changes

zijie0 and others added 2 commits August 20, 2021 15:53

Modify test case.

a1a545b

cargo fmt

29e63da

alamb merged commit 8308615 into apache:master Aug 21, 2021

alamb added a commit that referenced this pull request Aug 21, 2021

Support binary data type in build_struct_array. (#702)

c2fc8d4

* Support binary data type in `build_struct_array`. * Modify test case. * cargo fmt Co-authored-by: Andrew Lamb <andrew@nerdnetworks.org>

alamb mentioned this pull request Aug 21, 2021

Cherry pick of Support binary data type in build_struct_array #705

Merged

alamb added the cherry-picked label Aug 21, 2021

daniel-martinez-maqueda-sap mentioned this pull request Aug 1, 2022

Fix escaped like wildcards in like_utf8 / nlike_utf8 kernels #2258

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support binary data type in `build_struct_array`. #702

Support binary data type in `build_struct_array`. #702

zijie0 commented Aug 20, 2021

codecov-commenter commented Aug 20, 2021 •

edited

Loading

jorgecarleitao left a comment

jorgecarleitao Aug 20, 2021

nevi-me Aug 20, 2021

zijie0 Aug 20, 2021

nevi-me left a comment

nevi-me Aug 20, 2021

nevi-me Aug 20, 2021

nevi-me commented Aug 20, 2021

alamb commented Aug 20, 2021

zijie0 commented Aug 21, 2021

alamb commented Aug 21, 2021

houqp commented Aug 22, 2021

Support binary data type in build_struct_array. #702

Support binary data type in build_struct_array. #702

Conversation

zijie0 commented Aug 20, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented Aug 20, 2021 • edited Loading

Codecov Report

jorgecarleitao left a comment

Choose a reason for hiding this comment

jorgecarleitao Aug 20, 2021

Choose a reason for hiding this comment

nevi-me Aug 20, 2021

Choose a reason for hiding this comment

zijie0 Aug 20, 2021

Choose a reason for hiding this comment

nevi-me left a comment

Choose a reason for hiding this comment

nevi-me Aug 20, 2021

Choose a reason for hiding this comment

nevi-me Aug 20, 2021

Choose a reason for hiding this comment

nevi-me commented Aug 20, 2021

alamb commented Aug 20, 2021

zijie0 commented Aug 21, 2021

alamb commented Aug 21, 2021

houqp commented Aug 22, 2021

Support binary data type in `build_struct_array`. #702

Support binary data type in `build_struct_array`. #702

codecov-commenter commented Aug 20, 2021 •

edited

Loading