Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read/write nested dictionary under map in ipc stream reader/writer #1583

Merged
merged 4 commits into from
Apr 20, 2022

Conversation

viirya
Copy link
Member

@viirya viirya commented Apr 18, 2022

Which issue does this PR close?

Closes #1582.

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

@github-actions github-actions bot added the arrow Changes to the arrow crate label Apr 18, 2022
@codecov-commenter
Copy link

codecov-commenter commented Apr 18, 2022

Codecov Report

Merging #1583 (91f4e7a) into master (6bb6ed0) will increase coverage by 0.07%.
The diff coverage is 93.82%.

❗ Current head 91f4e7a differs from pull request most recent head e79b0f5. Consider uploading reports for the commit e79b0f5 to get more accurate results

@@            Coverage Diff             @@
##           master    #1583      +/-   ##
==========================================
+ Coverage   82.87%   82.94%   +0.07%     
==========================================
  Files         193      193              
  Lines       55304    55411     +107     
==========================================
+ Hits        45832    45962     +130     
+ Misses       9472     9449      -23     
Impacted Files Coverage Δ
arrow/src/ipc/writer.rs 80.78% <50.00%> (-1.02%) ⬇️
arrow/src/datatypes/field.rs 54.62% <66.66%> (ø)
arrow/src/array/array_struct.rs 88.44% <100.00%> (ø)
arrow/src/array/builder.rs 86.68% <100.00%> (ø)
arrow/src/array/data.rs 83.24% <100.00%> (ø)
arrow/src/array/equal/utils.rs 74.45% <100.00%> (ø)
arrow/src/compute/kernels/boolean.rs 96.79% <100.00%> (ø)
arrow/src/compute/kernels/cast.rs 95.81% <100.00%> (+0.08%) ⬆️
arrow/src/compute/kernels/substring.rs 100.00% <100.00%> (+1.68%) ⬆️
arrow/src/csv/reader.rs 89.89% <100.00%> (ø)
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6bb6ed0...e79b0f5. Read the comment docs.

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

This looks good to me, left some additional test suggestions

@@ -1481,4 +1481,55 @@ mod tests {
let output_batch = roundtrip_ipc_stream(&input_batch);
assert_eq!(input_batch, output_batch);
}

#[test]
fn test_roundtrip_stream_nested_dict_dict_in_map() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
fn test_roundtrip_stream_nested_dict_dict_in_map() {
fn test_roundtrip_stream_nested_dict_of_map_of_dict() {

let dict_array = DictionaryArray::<Int8Type>::try_new(&keys, &values).unwrap();

let keys_array = Int32Array::from_iter_values([0, 0, 1, 2, 0, 1]);
let keys_field = Field::new("keys", DataType::Int32, false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Might want to test dictionary encoded keys as well

let values_field = Field::new_dict(
"values",
DataType::Dictionary(Box::new(DataType::Int8), Box::new(DataType::Utf8)),
false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Could also test some NULLs

@viirya
Copy link
Member Author

viirya commented Apr 19, 2022

Thank you @tustvold. Revised based on the suggestions.

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

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

Will get this in once CI finishes chugging along

@viirya
Copy link
Member Author

viirya commented Apr 20, 2022

Thank you @tustvold

@tustvold tustvold merged commit d6b3962 into apache:master Apr 20, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Read/write nested dictionary under map in ipc stream reader/write
3 participants