Support skip_def_levels for ColumnLevelDecoder #2111

Ted-Jiang · 2022-07-20T07:52:20Z

Which issue does this PR close?

Closes #2107 .

Rationale for this change

This pr only support skip def_levels in DefinitionLevelBufferDecoder,
@tustvold Just make sure all page with max_def_level = 1 and max_rep_level = 0,
will only use DefinitionLevelBufferDecoder decode def_level.

In another word, this will support the skip val in col like OPTIONAL type R:0 D:1

Added :
Seems it still use ColumnLevelDecoderImpl in some case facing R:0 D:1
when decoding some col`s def_level

What changes are included in this PR?

Are there any user-facing changes?

Ted-Jiang · 2022-07-20T07:53:43Z

@tustvold @thinkharderdev PTAL 😊, this block the #2106 because all type are R:0 D:1.

parquet-tools meta  ./alltypes_tiny_pages_plain.parquet
file:            ~/github/arrow-rs/parquet-testing/data/alltypes_tiny_pages_plain.parquet
creator:         parquet-mr version 1.12.0-SNAPSHOT (build 6901a2040848c6b37fa61f4b0a76246445f396db)
extra:           writer.model.name = 2.1.1-cdh6.x-SNAPSHOT

file schema:     hive_schema
--------------------------------------------------------------------------------
id:              OPTIONAL INT32 R:0 D:1
bool_col:        OPTIONAL BOOLEAN R:0 D:1
tinyint_col:     OPTIONAL INT32 L:INTEGER(8,true) R:0 D:1
smallint_col:    OPTIONAL INT32 L:INTEGER(16,true) R:0 D:1
int_col:         OPTIONAL INT32 R:0 D:1
bigint_col:      OPTIONAL INT64 R:0 D:1
float_col:       OPTIONAL FLOAT R:0 D:1
double_col:      OPTIONAL DOUBLE R:0 D:1
date_string_col: OPTIONAL BINARY L:STRING R:0 D:1
string_col:      OPTIONAL BINARY L:STRING R:0 D:1
timestamp_col:   OPTIONAL INT96 R:0 D:1
year:            OPTIONAL INT32 R:0 D:1
month:           OPTIONAL INT32 R:0 D:1

codecov-commenter · 2022-07-20T08:21:32Z

Codecov Report

Merging #2111 (24c4519) into master (7746e7d) will decrease coverage by 0.01%.
The diff coverage is 72.72%.

@@            Coverage Diff             @@
##           master    #2111      +/-   ##
==========================================
- Coverage   82.86%   82.85%   -0.02%     
==========================================
  Files         237      237              
  Lines       61296    61381      +85     
==========================================
+ Hits        50792    50856      +64     
- Misses      10504    10525      +21

Impacted Files	Coverage Δ
parquet/src/column/reader/decoder.rs	`50.00% <0.00%> (-11.34%)`	⬇️
...rquet/src/arrow/record_reader/definition_levels.rs	`87.76% <91.42%> (+1.96%)`	⬆️
arrow/src/datatypes/datatype.rs	`62.69% <0.00%> (-0.32%)`	⬇️
parquet/src/encodings/encoding.rs	`93.62% <0.00%> (+0.19%)`	⬆️
parquet_derive/src/parquet_field.rs	`66.21% <0.00%> (+0.22%)`	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us.

liukun4515 · 2022-07-20T11:03:50Z

parquet/src/arrow/record_reader/definition_levels.rs

@@ -399,6 +442,55 @@ mod tests {
        assert_eq!(decoded.as_slice(), expected.as_slice());
    }

+    #[test]
+    fn test_packed_decoder_skip() {


tustvold

Unfortunately max_def_level = 1 is an insufficient condition for using a PackedDecoder, as the nulls may not exist at the leaf level.

The way this currently works is the ArrayReader implementation will pass either a packed or full DefinitionLevelBuffer to RecordReader, which will in turn pass this to DefinitionLevelBufferDecoder. This is a bit of a hack but avoided leaking arrow-specific details into the GenericRecordReader.

Unfortunately this means that DefinitionLevelBufferDecoder does not know if it should use a PackedDecoder until it has actually been asked to read data.

I will have a brief play and see if I can sort this out for you, it's my mess to cleanup 😆

tustvold · 2022-07-20T14:26:10Z

parquet/src/arrow/record_reader/definition_levels.rs

    ) -> Result<(usize, usize)> {
-        Err(nyi_err!("https://github.com/apache/arrow-rs/issues/1792"))
+        // For now only support max_def_level == 1
+        if max_def_level == 1 {


This isn't technically correct, if you have a nullable StructArray, containing a non-nullable child. The child will have a max definition level of 1, but it will be using ColumnLevelDecoderImpl not PackedDecoder.

Oh, i understand it 🤔 thanks!
I think when using PackedDecoder just need the bitmask of the col.
And the unit of the bitmask is per row.
So you use the max_def_level == 1 and max_rep_level == 0 as the checker
am i right 😂?

Not quite, we only preserve the bitmask if this is a nullable leaf.

I.e we would preserve the bitmask for this

message Schema { OPTIONAL int32 field; }

But not

message Schema { OPTIONAL GROUP group { REQUIRED int32 field; } }

Despite field having a max_def_level == 1 in both cases.

Following #2117 I would remove all max_def_level logic, and just match the type of decoder.

Got it
One question: if we preserve the bitmask if this is a nullable leaf
like

message Schema { OPTIONAL GROUP group { OPTIONAL int32 field; } }

we can not use the PackedDecoder right ? because the value can be 0,1,2

We are decoding the level data, not the value data - see https://akshays-blog.medium.com/wrapping-head-around-repetition-and-definition-levels-in-dremel-powering-bigquery-c1a33c9695da and https://blog.twitter.com/engineering/en_us/a/2013/dremel-made-simple-with-parquet.

In this case the definition level data can only be 0s or 1s

Thanks, i will read carefully. Seems you know the everything about parquet 😄

@tustvold 6e6d39c PTAL

parquet/src/arrow/record_reader/definition_levels.rs

parquet/src/column/reader/decoder.rs

tustvold · 2022-07-20T21:31:53Z

I've created #2116 which should hopefully help with the DefinitionLevelBufferDecoder logic, which I think is currently incorrect in this PR.

tustvold · 2022-07-22T17:11:07Z

#2117 has now been merged which should make it possible to significantly simplify this PR (i.e. remove the max_def_level shenanigans)

Ted-Jiang · 2022-07-23T14:44:17Z

#2117 has now been merged which should make it possible to significantly simplify this PR (i.e. remove the max_def_level shenanigans)

@tustvold i think you linked the wrong pr.

tustvold · 2022-07-23T14:45:18Z

@Ted-Jiang I meant #2116

…rImpl

Ted-Jiang · 2022-07-24T11:22:52Z

parquet/src/column/reader/decoder.rs

+                for _ in 0..num_levels {
+                    // Values are delimited by max_def_level
+                    if max_def_level
+                        == reader


will test in #2106

tustvold

Looks good to me, just some minor nits

tustvold · 2022-07-24T11:40:37Z

parquet/src/arrow/record_reader/definition_levels.rs

-        let indicator_value = self.decode_header()?;
+        let indicator_value = self
+            .decode_header()
+            .expect("decode_header fail in PackedDecoder");


Why this change?

Before met not set decode_header when coding. Better have the error info.

Before we propogated the error to the caller, instead of panicking?

Oh! my fault. it should use previous code pass the error. fix in fix comment

parquet/src/column/reader/decoder.rs

ursabot · 2022-07-24T15:51:09Z

Benchmark runs are scheduled for baseline = 7746e7d and contender = fce6626. fce6626 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

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

Ted-Jiang changed the title ~~Support skip_def_levels （only max_def_levels=1） for ColumnLevelDecoder~~ Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder Jul 20, 2022

liukun4515 reviewed Jul 20, 2022

View reviewed changes

Ted-Jiang changed the title ~~Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder~~ Support skip_def_levels (only max_def_levels=1) for DefinitionLevelBufferDecoder Jul 20, 2022

Ted-Jiang changed the title ~~Support skip_def_levels (only max_def_levels=1) for DefinitionLevelBufferDecoder~~ Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder Jul 20, 2022

tustvold reviewed Jul 20, 2022

View reviewed changes

Ted-Jiang force-pushed the issue_2107 branch from e7a68af to 6e6d39c Compare July 24, 2022 10:33

Ted-Jiang added 5 commits July 24, 2022 18:41

Support skip_def_levels （only max_def_levels=1） for ColumnLevelDecoder

001cf10

Support skip_def_levels （only max_def_levels=1） for ColumnLevelDecode…

6e9519f

…rImpl

fix ut

fbc29f6

fix and refine support skip definition_level

da14cb0

fix clippy

24c4519

Ted-Jiang force-pushed the issue_2107 branch from f85fd50 to 24c4519 Compare July 24, 2022 10:47

Ted-Jiang commented Jul 24, 2022

View reviewed changes

tustvold approved these changes Jul 24, 2022

View reviewed changes

Ted-Jiang mentioned this pull request Jul 24, 2022

Use vectorized unpacking in ColumnLevelDecoderImpl for ColumnLevelDecoderImpl #2153

Open

fix comment

79e03b9

tustvold merged commit fce6626 into apache:master Jul 24, 2022

Ted-Jiang changed the title ~~Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder~~ Support skip_def_levels for ColumnLevelDecoder Jul 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support skip_def_levels for ColumnLevelDecoder #2111

Support skip_def_levels for ColumnLevelDecoder #2111

Ted-Jiang commented Jul 20, 2022 •

edited

Loading

Ted-Jiang commented Jul 20, 2022 •

edited

Loading

codecov-commenter commented Jul 20, 2022 •

edited

Loading

liukun4515 Jul 20, 2022

tustvold left a comment

tustvold Jul 20, 2022

Ted-Jiang Jul 23, 2022 •

edited

Loading

tustvold Jul 23, 2022

Ted-Jiang Jul 23, 2022 •

edited

Loading

tustvold Jul 23, 2022 •

edited

Loading

Ted-Jiang Jul 23, 2022

Ted-Jiang Jul 24, 2022

tustvold commented Jul 20, 2022

tustvold commented Jul 22, 2022

Ted-Jiang commented Jul 23, 2022

tustvold commented Jul 23, 2022

Ted-Jiang Jul 24, 2022

tustvold left a comment

tustvold Jul 24, 2022

Ted-Jiang Jul 24, 2022

tustvold Jul 24, 2022

Ted-Jiang Jul 24, 2022

ursabot commented Jul 24, 2022

Support skip_def_levels for ColumnLevelDecoder #2111

Support skip_def_levels for ColumnLevelDecoder #2111

Conversation

Ted-Jiang commented Jul 20, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Ted-Jiang commented Jul 20, 2022 • edited Loading

codecov-commenter commented Jul 20, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ted-Jiang Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Ted-Jiang Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

tustvold Jul 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Jul 20, 2022

tustvold commented Jul 22, 2022

Ted-Jiang commented Jul 23, 2022

tustvold commented Jul 23, 2022

Choose a reason for hiding this comment

tustvold 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

ursabot commented Jul 24, 2022

Ted-Jiang commented Jul 20, 2022 •

edited

Loading

Ted-Jiang commented Jul 20, 2022 •

edited

Loading

codecov-commenter commented Jul 20, 2022 •

edited

Loading

Ted-Jiang Jul 23, 2022 •

edited

Loading

Ted-Jiang Jul 23, 2022 •

edited

Loading

tustvold Jul 23, 2022 •

edited

Loading