Generify parquet write path (#1764) #2045

tustvold · 2022-07-11T15:07:56Z

Which issue does this PR close?

Part of #1764

Rationale for this change

This will allow encoding arrow arrays directly instead of the current approach which must first convert them to a row format.

What changes are included in this PR?

Adds generics to ColumnWriterImpl to allow custom encoders

Are there any user-facing changes?

No

tustvold · 2022-07-11T15:08:16Z

parquet/src/basic.rs

@@ -212,7 +212,7 @@ pub enum Repetition {
 /// Encodings supported by Parquet.
 /// Not all encodings are valid for all types. These enums are also used to specify the
 /// encoding of definition and repetition levels.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Ord, PartialOrd)]


This is needed to allow for stable encoding ordering

tustvold · 2022-07-11T15:17:32Z

parquet/src/data_type.rs

    {
+        const PHYSICAL_TYPE: Type;


This is moved onto ParquetValueType as it allows implementing methods in terms of ParquetValueType which has the advantage that type inference works

tustvold · 2022-07-11T15:23:44Z

parquet/src/column/writer/mod.rs


        // Find out number of batches to process.
        let write_batch_size = self.props.write_batch_size();
-        let num_batches = min_len / write_batch_size;
+        let num_batches = num_levels / write_batch_size;


I think this was a bug before, previously the presence of nulls (which would lead to less values), would result in skewed batching

tustvold · 2022-07-11T15:26:05Z

parquet/src/file/statistics.rs

+pub(crate) mod private {
+    use super::*;
+
+    pub trait MakeStatistics {


I'm not a huge fan of this, but it was the only way I could work out to do this, and it is crate-local so if we come up with a better way we can change it in the future

Maybe another reason to not switch from TypedStatistics to ValueStatistics 🤔

codecov-commenter · 2022-07-11T15:56:06Z

Codecov Report

Merging #2045 (496bb70) into master (c585544) will increase coverage by 0.00%.
The diff coverage is 88.31%.

@@           Coverage Diff           @@
##           master    #2045   +/-   ##
=======================================
  Coverage   83.73%   83.73%           
=======================================
  Files         224      225    +1     
  Lines       59391    59373   -18     
=======================================
- Hits        49733    49718   -15     
+ Misses       9658     9655    -3

Impacted Files	Coverage Δ
parquet/src/basic.rs	`91.49% <ø> (ø)`
parquet/src/data_type.rs	`74.25% <50.00%> (-1.49%)`	⬇️
parquet/src/file/statistics.rs	`91.49% <78.57%> (-1.07%)`	⬇️
parquet/src/column/writer/encoder.rs	`88.46% <88.46%> (ø)`
parquet/src/column/writer/mod.rs	`92.53% <91.37%> (ø)`
parquet/src/encodings/encoding.rs	`93.62% <0.00%> (+0.19%)`	⬆️
arrow/src/datatypes/datatype.rs	`64.41% <0.00%> (+0.35%)`	⬆️
...row/src/array/builder/string_dictionary_builder.rs	`91.36% <0.00%> (+0.71%)`	⬆️

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 c585544...496bb70. Read the comment docs.

alamb · 2022-07-11T18:57:49Z

Lint is failing https://github.com/apache/arrow-rs/runs/7285461875?check_suite_focus=true 😭

alamb

I can't say I followed all the details of this PR (especially all the generics) but the code seems easier to read and other than a few questions on the tests I think things look good to me

Epic 🏅

alamb · 2022-07-11T19:02:38Z

parquet/src/column/writer/mod.rs

    num_column_nulls: u64,
    column_distinct_count: Option<u64>,
+    encodings: BTreeSet<Encoding>,


Why is this a BTreeSet (which requires Ord on Encoding) rather than a HashSet? Is there any reason the ordering is important?

It makes the output deterministic, which makes testing easier, but in terms of correctness, there should be no requirement for a particular order

Makes sense -- Perhaps it is worth a comment explaining the rationale

parquet/src/basic.rs

alamb · 2022-07-11T19:04:56Z

parquet/src/file/statistics.rs

-pub struct TypedStatistics<T: DataType> {
-    min: Option<T::T>,
-    max: Option<T::T>,
+pub type TypedStatistics<T> = ValueStatistics<<T as DataType>::T>;


I am sorry my rust-fu isn't good enough -- what is the purpose of renaming TypedStatistics to ValueStatistics (and doing the DataType::T nonsense in the typedef)? Is that needed to #[derive] works?

It is needed so you can create Statistics from a T: ParquetValueType, whereas previously it was being created (in a very hacky way) using DataType and casting via byte slices

alamb · 2022-07-11T19:08:38Z

parquet/src/file/statistics.rs

+pub(crate) mod private {
+    use super::*;
+
+    pub trait MakeStatistics {


Maybe another reason to not switch from TypedStatistics to ValueStatistics 🤔

alamb · 2022-07-11T19:11:30Z

parquet/src/column/writer/mod.rs

        );
        check_encoding_write_support::<Int32Type>(
            WriterVersion::PARQUET_2_0,
            false,
            &[1, 2],
            None,
-            &[Encoding::DELTA_BINARY_PACKED, Encoding::RLE],
+            &[Encoding::RLE, Encoding::DELTA_BINARY_PACKED],


I don't understand these changes. Why did the order of supported encodings change? Is it due to the use of a Set?

The way we track the encodings has changed slightly (using a set) which results in these changes

alamb · 2022-07-11T19:15:26Z

parquet/src/column/writer/mod.rs

    // Column writer properties
    descr: ColumnDescPtr,
    props: WriterPropertiesPtr,
    statistics_enabled: EnabledStatistics,

    page_writer: Box<dyn PageWriter + 'a>,
-    has_dictionary: bool,


this appears to be a significant improvement / change -- not tracking DictEncoder specially

parquet/src/column/writer/encoder.rs

alamb · 2022-07-11T19:23:16Z

It might be worth asking @sunchao or @nevi-me to take a look at this PR as well

tustvold · 2022-07-14T15:31:38Z

@nevi-me are you still reviewing this?

tustvold · 2022-07-17T20:32:11Z

I'm going to get this in, @nevi-me if you have any feedback I'll be more than happy to address it in a follow up

ursabot · 2022-07-17T20:40:47Z

Benchmark runs are scheduled for baseline = c585544 and contender = 02371d2. 02371d2 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

Generify parquet write path (apache#1764)

aa72844

github-actions bot added the parquet Changes to the parquet crate label Jul 11, 2022

More docs

64e62c5

tustvold commented Jul 11, 2022

View reviewed changes

alamb approved these changes Jul 11, 2022

View reviewed changes

tustvold added 2 commits July 12, 2022 08:43

Lint

680bb77

Fix doc

2f961f2

Review feedback

42cca6b

tustvold mentioned this pull request Jul 16, 2022

Array writer indirection #2091

Merged

tustvold added 3 commits July 16, 2022 14:12

Merge remote-tracking branch 'upstream/master' into generify-write-path

1775f8f

Merge remote-tracking branch 'upstream/master' into generify-write-path

7df5199

Fix doc

496bb70

tustvold merged commit 02371d2 into apache:master Jul 17, 2022

tustvold mentioned this pull request Jul 27, 2022

EnabledStatistics::None not working #2185

Closed

tustvold mentioned this pull request Aug 11, 2023

Use IntoIter trait for write_batch/write_mini_batch #43

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generify parquet write path (#1764) #2045

Generify parquet write path (#1764) #2045

tustvold commented Jul 11, 2022

tustvold Jul 11, 2022

tustvold Jul 11, 2022

tustvold Jul 11, 2022

tustvold Jul 11, 2022

alamb Jul 11, 2022

codecov-commenter commented Jul 11, 2022 •

edited

alamb commented Jul 11, 2022

alamb left a comment

alamb Jul 11, 2022

tustvold Jul 11, 2022

alamb Jul 12, 2022

alamb Jul 11, 2022

tustvold Jul 11, 2022 •

edited

alamb Jul 11, 2022

alamb Jul 11, 2022

tustvold Jul 11, 2022

alamb Jul 11, 2022

alamb commented Jul 11, 2022

tustvold commented Jul 14, 2022

tustvold commented Jul 17, 2022

ursabot commented Jul 17, 2022

Generify parquet write path (#1764) #2045

Generify parquet write path (#1764) #2045

Conversation

tustvold commented Jul 11, 2022

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-commenter commented Jul 11, 2022 • edited

Codecov Report

alamb commented Jul 11, 2022

alamb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold Jul 11, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alamb commented Jul 11, 2022

tustvold commented Jul 14, 2022

tustvold commented Jul 17, 2022

ursabot commented Jul 17, 2022

codecov-commenter commented Jul 11, 2022 •

edited

tustvold Jul 11, 2022 •

edited