Skip to content

Support binary data type in build_struct_array.#702

Merged
alamb merged 3 commits intoapache:masterfrom
zijie0:build_struct_array_for_binary_type
Aug 21, 2021
Merged

Support binary data type in build_struct_array.#702
alamb merged 3 commits intoapache:masterfrom
zijie0:build_struct_array_for_binary_type

Conversation

@zijie0
Copy link
Contributor

@zijie0 zijie0 commented Aug 20, 2021

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.

@github-actions github-actions bot added the arrow Changes to the arrow crate label Aug 20, 2021
@houqp houqp requested review from Dandandan and nevi-me August 20, 2021 04:13
@codecov-commenter
Copy link

codecov-commenter commented Aug 20, 2021 •
edited
Loading

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 jorgecarleitao mentioned this pull request Aug 20, 2021
Merged
jorgecarleitao
jorgecarleitao approved these changes Aug 20, 2021
View reviewed changes
Copy link
Member

@jorgecarleitao jorgecarleitao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

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

arrow/src/array/array.rs
DataType::Struct(vec![
Field::new("key", DataType::Utf8, false),
Field::new("key", DataType::Int32, true),
Field::new("value", DataType::Int32, true),
Copy link
Member

@jorgecarleitao jorgecarleitao Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why this?

Copy link
Contributor

@nevi-me nevi-me Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@zijie0 zijie0 Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

nevi-me
nevi-me approved these changes Aug 20, 2021
View reviewed changes
Copy link
Contributor

@nevi-me nevi-me left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

arrow/src/array/array.rs
DataType::Struct(vec![
Field::new("key", DataType::Utf8, false),
Field::new("key", DataType::Int32, true),
Field::new("value", DataType::Int32, true),
Copy link
Contributor

@nevi-me nevi-me Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

arrow/src/json/reader.rs
rows.iter()
.map(|row| {
let maybe_value = row.get(field.name());
maybe_value.and_then(|value| value.as_str())
Copy link
Contributor

@nevi-me nevi-me Aug 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

nevi-me commented Aug 20, 2021

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
Copy link
Contributor

alamb commented Aug 20, 2021

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
Copy link
Contributor Author

zijie0 commented Aug 21, 2021

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

@alamb alamb merged commit 8308615 into apache:master Aug 21, 2021
@alamb
Copy link
Contributor

alamb commented Aug 21, 2021

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)

alamb added a commit that referenced this pull request Aug 21, 2021
@zijie0 @alamb
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 <[email protected]>
@alamb alamb mentioned this pull request Aug 21, 2021
Merged
@houqp
Copy link
Member

houqp commented Aug 22, 2021

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.

alamb added a commit that referenced this pull request Aug 23, 2021
@alamb @zijie0
Cherry pick of Support binary data type in build_struct_array (#705)
4ca0d95 
* 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 <[email protected]>

* Remove accidentally included test

Co-authored-by: Yuan Zhou <[email protected]>
@daniel-martinez-maqueda-sap daniel-martinez-maqueda-sap mentioned this pull request Aug 1, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@houqp houqp houqp approved these changes

@nevi-me nevi-me nevi-me approved these changes

@jorgecarleitao jorgecarleitao jorgecarleitao approved these changes

@Dandandan Dandandan Awaiting requested review from Dandandan

Assignees

No one assigned

Labels

arrow Changes to the arrow crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Support reading json string into binary data type.

6 participants

@zijie0 @codecov-commenter @nevi-me @alamb @houqp @jorgecarleitao