support full u32 and u64 roundtrip through parquet #258

crepererum · 2021-05-05T13:14:56Z

Which issue does this PR close?

Closes #254 .

Rationale for this change

Up until now u64 values larger than i64::MAX were silently truncated to "invalid" when going through parquet. With this change we follow the C++ stack and

What changes are included in this PR?

a bit of prep (first two commits)
map all u64 values to i64 via overflow/reinterpret cast
account of logical type is_signed flag in statistics calculation (as per spec)

Are there any user-facing changes?

Yes, users can now expect to have full u64 storage support. Old files should still work since we previously marked values larger than i64::MAX as invalid.

codecov-commenter · 2021-05-05T13:29:39Z

Codecov Report

Merging #258 (2710f31) into master (8bd769b) will increase coverage by 0.01%.
The diff coverage is 90.21%.

@@            Coverage Diff             @@
##           master     #258      +/-   ##
==========================================
+ Coverage   82.53%   82.54%   +0.01%     
==========================================
  Files         162      162              
  Lines       43796    43862      +66     
==========================================
+ Hits        36149    36208      +59     
- Misses       7647     7654       +7

Impacted Files	Coverage Δ
parquet/src/column/writer.rs	`93.64% <84.00%> (-0.13%)`	⬇️
parquet/src/arrow/array_reader.rs	`77.12% <85.71%> (-0.02%)`	⬇️
parquet/src/arrow/arrow_writer.rs	`98.04% <94.33%> (-0.38%)`	⬇️

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 8bd769b...2710f31. Read the comment docs.

alamb · 2021-05-05T15:19:10Z

parquet/src/arrow/array_reader.rs

@@ -380,6 +380,18 @@ impl<T: DataType> ArrayReader for PrimitiveArrayReader<T> {
                }
                Arc::new(builder.finish()) as ArrayRef
            }
+            ArrowType::UInt64 => match array.data_type() {


This code was confusing to me (as it is manipulating the Arrow array after it has been created rather than manipulating the data prior to creating the array).

It also seems like it will result in a copy of all Int64 columns which may not be idea.

I wonder if you considered creating the Unit64Array directly from the parquet data up here: https://github.com/apache/arrow-rs/pull/258/files#diff-0d6bed48d78c5a2472b7680a8185cabdc0bd259d6484e184439ed7830060661fR316-R319
instead?

That may be clearer to understand as well as more performant

As far as I can see we always cast the array, see default case in line 383 (pre-patch) / 395 (post-patch). So we would always copy, no?

I was thinking of the additional copy that comes from the arity call -- specifically https://github.com/apache/arrow-rs/blob/master/arrow/src/compute/kernels/arity.rs#L64

The arity call will be in addition to any copying that cast does, I think.

Also, when the types are the same, then cast is a noop

alamb · 2021-05-06T11:41:13Z

The test failure
https://github.com/apache/arrow-rs/pull/258/checks?check_run_id=2516139774 seems to be due to a network problem. Retriggering.


  nightly-2021-03-24-x86_64-unknown-linux-gnu installed - rustc 1.53.0-nightly (673d0db5e 2021-03-23)

info: checking for self-updates
error: could not download file from 'https://static.rust-lang.org/rustup/release-stable.toml' to '/tmp/rustup-updatemwNOD1/release-stable.toml'
error: caused by: failed to make network request
error: caused by: error sending request for url (https://static.rust-lang.org/rustup/release-stable.toml): connection error: Connection reset by peer (os error 104)
error: caused by: connection error: Connection reset by peer (os error 104)
error: caused by: Connection reset by peer (os error 104)
Error: Process completed with exit code 1.

alamb

I think it is looking great. Thanks @crepererum !

nevi-me · 2021-05-07T05:35:11Z

@crepererum thanks for picking this up, and working on it. Do we have the same risk for u32 <> i32?

crepererum · 2021-05-07T07:25:45Z

@nevi-me good catch. Indeed there was the same bug. Added a test and fixed as well.

crepererum · 2021-05-07T07:54:23Z

Will rebase once #267 is merged.

alamb · 2021-05-07T18:46:40Z

#267 has been merged

Seems logical since all other kernels are re-exported as well under this flat hierarchy.

…oundtrip`

- updates arrow to parquet type mapping to use reinterpret/overflow cast for u64<->i64 similar to what the C++ stack does - changes statistics calculation to account for the fact that u64 should be compared unsigned (as per spec) Fixes apache#254.

This is idential to the solution we now have for u64.

crepererum · 2021-05-10T07:53:11Z

ready :)

nevi-me · 2021-05-10T16:44:10Z

parquet/src/column/writer.rs

+
+    /// Evaluate `a > b` according to underlying logical type.
+    fn compare_greater(&self, a: &T::T, b: &T::T) -> bool {
+        if let Some(LogicalType::INTEGER(int_type)) = self.descr.logical_type() {


Note: not all implementations might write LogicalType, even though ConvertedType is deprecated. I had to think a bit about this, but it's fine because it's on the write-side, where we will always write LogicalType going forward.

If this was on the read side, we'd have to also check ConvertedType as a fallback.

nevi-me

LGTM

alamb reviewed May 5, 2021

View reviewed changes

alamb added the parquet Changes to the parquet crate label May 5, 2021

crepererum requested a review from alamb May 6, 2021 07:38

alamb approved these changes May 6, 2021

View reviewed changes

alamb requested a review from nevi-me May 6, 2021 11:45

crepererum force-pushed the crepererum/issue254 branch from 550a48f to 0a83a42 Compare May 7, 2021 07:24

nevi-me changed the title ~~support full u64 roundtrip through parquet~~ support full u32 and u64 roundtrip through parquet May 7, 2021

crepererum added 5 commits May 10, 2021 09:23

re-export arity kernels in arrow::compute

cbc87c4

Seems logical since all other kernels are re-exported as well under this flat hierarchy.

return file from `parquet::arrow::arrow_writer::tests::[one_column]_r…

dceb539

…oundtrip`

support full arrow u64 through parquet

432299b

- updates arrow to parquet type mapping to use reinterpret/overflow cast for u64<->i64 similar to what the C++ stack does - changes statistics calculation to account for the fact that u64 should be compared unsigned (as per spec) Fixes apache#254.

avoid copying array when reading u64 from parquet

d1a9e0b

support full arrow u32 through parquet

2710f31

This is idential to the solution we now have for u64.

crepererum force-pushed the crepererum/issue254 branch from 0a83a42 to 2710f31 Compare May 10, 2021 07:23

nevi-me reviewed May 10, 2021

View reviewed changes

nevi-me approved these changes May 10, 2021

View reviewed changes

nevi-me merged commit 2f5f58a into apache:master May 10, 2021

jorgecarleitao mentioned this pull request May 14, 2021

Added changelog generator script and configuration. #289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

support full u32 and u64 roundtrip through parquet #258

support full u32 and u64 roundtrip through parquet #258

crepererum commented May 5, 2021

codecov-commenter commented May 5, 2021 •

edited

Loading

alamb May 5, 2021

crepererum May 5, 2021

alamb May 5, 2021

crepererum May 6, 2021

alamb commented May 6, 2021

alamb left a comment

nevi-me commented May 7, 2021

crepererum commented May 7, 2021

crepererum commented May 7, 2021

alamb commented May 7, 2021

crepererum commented May 10, 2021

nevi-me May 10, 2021

nevi-me left a comment

support full u32 and u64 roundtrip through parquet #258

support full u32 and u64 roundtrip through parquet #258

Conversation

crepererum commented May 5, 2021

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

codecov-commenter commented May 5, 2021 • edited Loading

Codecov Report

alamb May 5, 2021

Choose a reason for hiding this comment

crepererum May 5, 2021

Choose a reason for hiding this comment

alamb May 5, 2021

Choose a reason for hiding this comment

crepererum May 6, 2021

Choose a reason for hiding this comment

alamb commented May 6, 2021

alamb left a comment

Choose a reason for hiding this comment

nevi-me commented May 7, 2021

crepererum commented May 7, 2021

crepererum commented May 7, 2021

alamb commented May 7, 2021

crepererum commented May 10, 2021

nevi-me May 10, 2021

Choose a reason for hiding this comment

nevi-me left a comment

Choose a reason for hiding this comment

codecov-commenter commented May 5, 2021 •

edited

Loading