Skip to content

Conversation

@nevi-me
Copy link
Contributor

@nevi-me nevi-me commented Mar 22, 2021

Minor change to compute the levels for FSB arrays and write them out. Added a roundtrip test.

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.

LGTM. Great addition, thanks a lot @nevi-me !

ArrowDataType::FixedSizeBinary(_) => {
let mut col_writer = get_col_writer(&mut row_group_writer)?;
write_leaf(
&mut col_writer,
array,
levels.pop().expect("Levels exhausted"),
)?;
row_group_writer.close_column(col_writer)?;
Ok(())
}
Copy link
Member

Choose a reason for hiding this comment

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

I can't spot the difference between this and all cases above. Is there any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

lol, there isn't. Someone (won't name them) copied this chunk separately, thinking that it'll be different. Then they forgot to merge it when they realised that there was no difference.

I'll fix it shortly

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed this

@github-actions
Copy link

Copy link
Contributor

@alamb alamb left a comment

Choose a reason for hiding this comment

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

Looks good to me -- thanks @nevi-me

Comment on lines 204 to 196
"Attempting to write an Arrow type that is not yet implemented"
.to_string(),
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
"Attempting to write an Arrow type that is not yet implemented"
.to_string(),
format!("Attempting to write an Arrow type {?} to parquet that is not yet implemented", array.data_type())

( I realize the error message is not changed by this PR I just saw it when looking)

@nevi-me nevi-me force-pushed the ARROW-12043 branch 3 times, most recently from 1e91b45 to 5bf4197 Compare March 25, 2021 21:06
@codecov-io
Copy link

Codecov Report

Merging #9771 (4ecc2d7) into master (29feea0) will decrease coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #9771      +/-   ##
==========================================
- Coverage   82.59%   82.41%   -0.18%     
==========================================
  Files         248      252       +4     
  Lines       58294    58977     +683     
==========================================
+ Hits        48149    48608     +459     
- Misses      10145    10369     +224     
Impacted Files Coverage Δ
rust/arrow/src/array/transform/fixed_binary.rs 0.00% <0.00%> (-78.95%) ⬇️
rust/arrow/src/array/transform/null.rs 0.00% <0.00%> (-66.67%) ⬇️
rust/arrow/src/array/transform/structure.rs 44.44% <0.00%> (-38.89%) ⬇️
rust/arrow/src/array/transform/primitive.rs 71.42% <0.00%> (-28.58%) ⬇️
rust/arrow/src/array/transform/mod.rs 70.23% <0.00%> (-22.96%) ⬇️
rust/arrow/src/array/transform/variable_size.rs 87.87% <0.00%> (-12.13%) ⬇️
rust/arrow/src/array/array_binary.rs 79.84% <0.00%> (-11.80%) ⬇️
rust/arrow/src/buffer/mutable.rs 83.89% <0.00%> (-6.87%) ⬇️
rust/parquet/src/schema/parser.rs 86.27% <0.00%> (-3.93%) ⬇️
rust/parquet/src/arrow/schema.rs 89.08% <0.00%> (-3.29%) ⬇️
... and 62 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 017a93d...4ecc2d7. Read the comment docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants