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

Fix projection in IPC reader #1736

Merged
merged 5 commits into from
May 26, 2022
Merged

Conversation

iyupeng
Copy link
Contributor

@iyupeng iyupeng commented May 24, 2022

Which issue does this PR close?

Closes #1735 .

Rationale for this change

Fix possible panic caused by projection in IPC reader.

What changes are included in this PR?

Fix the function read_record_batch in arrow/src/ipc/reader.rs.

Are there any user-facing changes?

No.

@github-actions github-actions bot added the arrow Changes to the arrow crate label May 24, 2022
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Thanks @iyupeng. Can you add a related test with the change? I saw you already have reproducible example in the issue. Seems it is easy to make it as a test?

@iyupeng
Copy link
Contributor Author

iyupeng commented May 25, 2022

Hi @viirya , thanks for your comment. A new test is added to IPC reader in my last commit.

@codecov-commenter
Copy link

codecov-commenter commented May 25, 2022

Codecov Report

Merging #1736 (a18617d) into master (ca1d85f) will increase coverage by 0.00%.
The diff coverage is 95.58%.

❗ Current head a18617d differs from pull request most recent head 3e81464. Consider uploading reports for the commit 3e81464 to get more accurate results

@@           Coverage Diff           @@
##           master    #1736   +/-   ##
=======================================
  Coverage   83.31%   83.32%           
=======================================
  Files         196      195    -1     
  Lines       55961    56047   +86     
=======================================
+ Hits        46625    46699   +74     
- Misses       9336     9348   +12     
Impacted Files Coverage Δ
parquet/benches/arrow_writer.rs 0.00% <0.00%> (ø)
parquet/src/arrow/array_reader/list_array.rs 93.41% <ø> (ø)
parquet/src/arrow/schema.rs 96.81% <ø> (ø)
parquet/src/util/cursor.rs 63.86% <ø> (-13.45%) ⬇️
parquet/src/util/io.rs 88.33% <ø> (-1.67%) ⬇️
parquet_derive/src/lib.rs 0.00% <ø> (ø)
parquet_derive/src/parquet_field.rs 65.98% <0.00%> (+0.22%) ⬆️
parquet/src/data_type.rs 75.84% <50.00%> (ø)
parquet/src/file/writer.rs 92.85% <92.17%> (-0.45%) ⬇️
arrow/src/ipc/reader.rs 90.74% <100.00%> (+2.01%) ⬆️
... and 11 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 ca1d85f...3e81464. Read the comment docs.

arrow/src/ipc/reader.rs Outdated Show resolved Hide resolved
Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Looks a correct fix to me. I checked and compared it with create_array.

Although I think we can still improve the test. For example, projection on complex types such as struct, list, union, etc.

@iyupeng
Copy link
Contributor Author

iyupeng commented May 25, 2022

Hi @viirya, I can add more commits to improve the test on projection. Please wait for some time before merging.

@viirya
Copy link
Member

viirya commented May 25, 2022

Thank you @iyupeng

@iyupeng
Copy link
Contributor Author

iyupeng commented May 26, 2022

Hi @viirya , the test for projection has been improved to check more data types. It is ready for review now.

let struct_data_type = DataType::Struct(struct_fields);

// define schema
Schema::new(vec![
Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member

@viirya viirya left a comment

Choose a reason for hiding this comment

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

Thanks @iyupeng! The new test looks good to me. I will leave this open for a while and will merge in next day if no more comments from other possible reviewers.

Thanks for the fix.

@viirya viirya merged commit 7391710 into apache:master May 26, 2022
@viirya
Copy link
Member

viirya commented May 26, 2022

Merged, thanks!

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.

IPC reader may break on projection
3 participants