ARROW-10766: [Rust] [Parquet] Compute nested list definitions #9240

nevi-me · 2021-01-18T00:53:31Z

This mainly computes definition and repetition leves for lists.
It also partially adds deeply nested write support.
I am however going to complete this in a separate PR.

This has really been challenging because we can't roundtrip without nested writers,
so it's taken me months to complete.
In the process, I've had to rely on using Spark to verify my work.

This PR is also not optimised. I've left TODOs in a few places (sparingly).
The biggest next step is to remove array_mask: Vec and replace it with a bitpacked vector to save memory.

github-actions · 2021-01-18T00:53:50Z

https://issues.apache.org/jira/browse/ARROW-10766

nevi-me · 2021-01-18T01:06:02Z

Hi everyone interested in the Parquet writer.

This PR effectively gives us the ability to compute how to write arbitrarily nested types. It has the side effect that nested lists can also be written.
There's a few places where I need to tidy up, but they're dependent on the Arrow reader (ARROW-10391), which unfortunately might be a lot of work on its own. I'm a bit worried that I might have to rework a fair share of the writer to handle nesting correctly. I've already seen instances where we don't always have enough information to arrive at the correct solution.

I'll open JIRAs as I go along.

For reviewers, please note:

This has taken me a few months on weekends to get right. I've iterated over various solutions to arrive here.
The implementation is not optimal (I haven't benchmarked the latest impl), but I'm confident that it's correct.
the extensive tests on the levels.rs will allow us to refactor with some confidence.

I've spent far too long on this, so I practically don't have any fresh eyes here. I worked on all the edge-cases that I could think with lists and structs. I've documented them, but I'll review the doc comments and add more detail where I still feel that it's lacking.

Thank you ❤️

codecov-io · 2021-01-18T01:09:05Z

Codecov Report

Merging #9240 (75487cf) into master (1393188) will increase coverage by 0.14%.
The diff coverage is 85.38%.

@@            Coverage Diff             @@
##           master    #9240      +/-   ##
==========================================
+ Coverage   81.61%   81.75%   +0.14%     
==========================================
  Files         215      215              
  Lines       51867    52400     +533     
==========================================
+ Hits        42329    42839     +510     
- Misses       9538     9561      +23

Impacted Files	Coverage Δ
rust/parquet/src/arrow/array_reader.rs	`75.00% <71.42%> (+3.59%)`	⬆️
rust/parquet/src/arrow/levels.rs	`81.55% <84.69%> (+9.71%)`	⬆️
rust/parquet/src/arrow/arrow_writer.rs	`96.78% <100.00%> (+1.20%)`	⬆️
rust/parquet/src/arrow/schema.rs	`91.67% <100.00%> (+0.17%)`	⬆️
rust/arrow/src/ipc/gen/Schema.rs	`40.43% <0.00%> (+0.71%)`	⬆️
rust/arrow/src/array/equal/utils.rs	`74.75% <0.00%> (+0.97%)`	⬆️
rust/arrow/src/ipc/convert.rs	`93.67% <0.00%> (+1.58%)`	⬆️
rust/arrow/src/array/transform/fixed_binary.rs	`84.21% <0.00%> (+5.26%)`	⬆️

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 71b6b9c...75487cf. Read the comment docs.

rust/parquet/src/arrow/levels.rs

nevi-me · 2021-01-18T01:19:53Z

rust/parquet/src/arrow/levels.rs

+
+    #[test]
+    fn test_filter_array_indices() {
+        let level = LevelInfo {


I didn't have enough test cases to compute here. I'll add more when I increase coverage of deeply-nested lists

rust/parquet/src/arrow/array_reader.rs

This mainly computes definition and repetition leves for lists. It also partially adds deeply nested write support. I am however going to complete this in a separate PR. This has really been challenging because we can't roundtrip without nested writers, so it's taken me months to complete. In the process, I've had to rely on using Spark to verify my work. This PR is also not optimised. I've left TODOs in a few places (sparingly). The biggest next step is to remove array_mask: Vec<u8> and replace it with a bitpacked vector to save memory.

nevi-me · 2021-01-20T05:06:51Z

@sunchao may you please kindly review this when you get a chance

sunchao · 2021-01-20T05:09:52Z

Sure. Thanks for pinging me. Will take a look soon.

rtyler · 2021-01-20T22:53:48Z

rust/parquet/src/arrow/array_reader.rs

+            | ArrowType::LargeList(field) => field,
+            _ => {
+                // Panic: this is safe as we only write lists from list datatypes
+                unreachable!()


For my own curiosity, is there a specific reason to use unreachable! rather than panic! in cases like this? I understand the outcome will be the same.

They don't make any difference, unreachable!() will call panic!() with a message about unreachable code being reached. So it's probably a more descriptive panic.

I tohught that marking a condition as unreachable!() lets the compiler optimise out that condition, but it seems like only its unsafe equivalent does.

jorgecarleitao

While I did not grasp all the details here, I think that this is ready to merge.

Impressive work, @nevi-me 💯

sunchao · 2021-01-22T15:38:09Z

@alamb given that this is a 1k+ line PR, could you give us a chance to review it properly before eagerly merging it?

alamb · 2021-01-22T19:08:05Z

I am really sorry @sunchao -- I missed your earlier comment that you would be reviewing this more carefully. I have been trying to clear out the backlog of Rust PRs and I agree I was too eager on this one

Would you like me to revert this PR and prepare a new one to re-merge?

sunchao · 2021-01-22T20:02:02Z

No worries @alamb . I'll do review on this closed PR and we can address any feedback in followups.

alamb · 2021-01-22T22:26:23Z

😓 Thank you for understanding @sunchao

This mainly computes definition and repetition leves for lists. It also partially adds deeply nested write support. I am however going to complete this in a separate PR. This has really been challenging because we can't roundtrip without nested writers, so it's taken me months to complete. In the process, I've had to rely on using Spark to verify my work. This PR is also not optimised. I've left TODOs in a few places (sparingly). The biggest next step is to remove array_mask: Vec<u8> and replace it with a bitpacked vector to save memory. Closes #9240 from nevi-me/ARROW-10766-v2 Authored-by: Neville Dipale <nevilledips@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

This mainly computes definition and repetition leves for lists. It also partially adds deeply nested write support. I am however going to complete this in a separate PR. This has really been challenging because we can't roundtrip without nested writers, so it's taken me months to complete. In the process, I've had to rely on using Spark to verify my work. This PR is also not optimised. I've left TODOs in a few places (sparingly). The biggest next step is to remove array_mask: Vec<u8> and replace it with a bitpacked vector to save memory. Closes apache#9240 from nevi-me/ARROW-10766-v2 Authored-by: Neville Dipale <nevilledips@gmail.com> Signed-off-by: Andrew Lamb <andrew@nerdnetworks.org>

nevi-me requested a review from sunchao January 18, 2021 00:53

github-actions bot added Component: Rust Component: Parquet labels Jan 18, 2021

nevi-me commented Jan 18, 2021

View reviewed changes

rust/parquet/src/arrow/array_reader.rs Outdated Show resolved Hide resolved

nevi-me added 2 commits January 20, 2021 06:15

clean up

a59613b

nevi-me force-pushed the ARROW-10766-v2 branch from 75487cf to a59613b Compare January 20, 2021 05:06

rtyler reviewed Jan 21, 2021

View reviewed changes

jorgecarleitao approved these changes Jan 21, 2021

View reviewed changes

alamb closed this in 251ecac Jan 22, 2021

asfimport mentioned this pull request Jan 22, 2021

[Rust] Compute nested definition and repetition for list arrays #18397

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-10766: [Rust] [Parquet] Compute nested list definitions #9240

ARROW-10766: [Rust] [Parquet] Compute nested list definitions #9240

nevi-me commented Jan 18, 2021

github-actions bot commented Jan 18, 2021

nevi-me commented Jan 18, 2021

codecov-io commented Jan 18, 2021

nevi-me Jan 18, 2021

nevi-me commented Jan 20, 2021 •

edited

Loading

sunchao commented Jan 20, 2021

rtyler Jan 20, 2021

nevi-me Jan 21, 2021

jorgecarleitao left a comment •

edited

Loading

sunchao commented Jan 22, 2021 •

edited

Loading

alamb commented Jan 22, 2021 •

edited

Loading

sunchao commented Jan 22, 2021

alamb commented Jan 22, 2021

ARROW-10766: [Rust] [Parquet] Compute nested list definitions #9240

ARROW-10766: [Rust] [Parquet] Compute nested list definitions #9240

Conversation

nevi-me commented Jan 18, 2021

github-actions bot commented Jan 18, 2021

nevi-me commented Jan 18, 2021

codecov-io commented Jan 18, 2021

Codecov Report

nevi-me Jan 18, 2021

Choose a reason for hiding this comment

nevi-me commented Jan 20, 2021 • edited Loading

sunchao commented Jan 20, 2021

rtyler Jan 20, 2021

Choose a reason for hiding this comment

nevi-me Jan 21, 2021

Choose a reason for hiding this comment

jorgecarleitao left a comment • edited Loading

Choose a reason for hiding this comment

sunchao commented Jan 22, 2021 • edited Loading

alamb commented Jan 22, 2021 • edited Loading

sunchao commented Jan 22, 2021

alamb commented Jan 22, 2021

nevi-me commented Jan 20, 2021 •

edited

Loading

jorgecarleitao left a comment •

edited

Loading

sunchao commented Jan 22, 2021 •

edited

Loading

alamb commented Jan 22, 2021 •

edited

Loading