You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by GitBox <gi...@apache.org> on 2022/07/20 07:52:22 UTC
[GitHub] [arrow-rs] Ted-Jiang opened a new pull request, #2111: Support skip_def_levels （only max_def_levels=1） for ColumnLevelDecoder
Ted-Jiang opened a new pull request, #2111:
URL: https://github.com/apache/arrow-rs/pull/2111
# 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 col like `OPTIONAL type R:0 D:1`
# What changes are included in this PR?
<!---
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!---
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking change` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] codecov-commenter commented on pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#issuecomment-1189975418
# [Codecov](https://codecov.io/gh/apache/arrow-rs/pull/2111?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#2111](https://codecov.io/gh/apache/arrow-rs/pull/2111?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (f192929) into [master](https://codecov.io/gh/apache/arrow-rs/commit/efd3152f85f182757138dd5984c9832c3257448c?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (efd3152) will **increase** coverage by `0.01%`.
> The diff coverage is `79.66%`.
```diff
@@ Coverage Diff @@
## master #2111 +/- ##
==========================================
+ Coverage 83.73% 83.74% +0.01%
==========================================
Files 225 225
Lines 59412 59479 +67
==========================================
+ Hits 49748 49811 +63
- Misses 9664 9668 +4
```
| [Impacted Files](https://codecov.io/gh/apache/arrow-rs/pull/2111?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...rquet/src/arrow/record\_reader/definition\_levels.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvYXJyb3cvcmVjb3JkX3JlYWRlci9kZWZpbml0aW9uX2xldmVscy5ycw==) | `84.01% <79.66%> (-1.17%)` | :arrow_down: |
| [parquet/src/encodings/encoding.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldC9zcmMvZW5jb2RpbmdzL2VuY29kaW5nLnJz) | `93.43% <0.00%> (-0.20%)` | :arrow_down: |
| [arrow/src/array/array\_primitive.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X3ByaW1pdGl2ZS5ycw==) | `95.78% <0.00%> (-0.04%)` | :arrow_down: |
| [arrow/src/array/data.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2RhdGEucnM=) | `84.94% <0.00%> (ø)` | |
| [parquet\_derive/src/parquet\_field.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-cGFycXVldF9kZXJpdmUvc3JjL3BhcnF1ZXRfZmllbGQucnM=) | `65.98% <0.00%> (+0.45%)` | :arrow_up: |
| [arrow/src/array/array\_boolean.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2FycmF5X2Jvb2xlYW4ucnM=) | `94.00% <0.00%> (+0.61%)` | :arrow_up: |
| [...row/src/array/builder/string\_dictionary\_builder.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2FycmF5L2J1aWxkZXIvc3RyaW5nX2RpY3Rpb25hcnlfYnVpbGRlci5ycw==) | `91.36% <0.00%> (+0.71%)` | :arrow_up: |
| [arrow/src/compute/kernels/boolean.rs](https://codecov.io/gh/apache/arrow-rs/pull/2111/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-YXJyb3cvc3JjL2NvbXB1dGUva2VybmVscy9ib29sZWFuLnJz) | `97.69% <0.00%> (+0.88%)` | :arrow_up: |
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] ursabot commented on pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
ursabot commented on PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#issuecomment-1193345271
Benchmark runs are scheduled for baseline = 7746e7dd4eb2be66bc6aa84b9dcebd2a76036d00 and contender = fce66260015642e691f246d29666d8f9c3197f8a. fce66260015642e691f246d29666d8f9c3197f8a is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/6fd2c405ac73458797c56e1ba65912cf...58fdef3b148a46a1b57be141f256b227/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on test-mac-arm] [test-mac-arm](https://conbench.ursa.dev/compare/runs/350e003434ca4657b065314d02886e55...9f54630cd85b4789882303e2a289db42/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/fb4a14493bac44e4a8678fb4d08ef2b8...2a17339fd85a48fe9b58f7070b9a9d03/)
[Skipped :warning: Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/5986a4af8a7b4c1894f6b5cb91e42cc2...4a263aff4db9409ebc19a8d164a69dd8/)
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928122873
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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 we need the `max_def_level == 1` and `max_rep_level == 0`
am i right 😂?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928243863
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -248,7 +254,9 @@ struct PackedDecoder {
impl PackedDecoder {
fn next_rle_block(&mut self) -> Result<()> {
- let indicator_value = self.decode_header()?;
+ let indicator_value = self
+ .decode_header()
+ .expect("decode_header fail in PackedDecoder");
Review Comment:
Why this change?
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -318,10 +318,41 @@ impl ColumnLevelDecoder for ColumnLevelDecoderImpl {
impl DefinitionLevelDecoder for ColumnLevelDecoderImpl {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> Result<(usize, usize)> {
- Err(nyi_err!("https://github.com/apache/arrow-rs/issues/1792"))
+ let mut level_skip = 0;
+ let mut value_skip = 0;
+ match self.decoder.as_mut().unwrap() {
+ LevelDecoderInner::Packed(reader, bit_width) => {
+ for _ in 0..num_levels {
Review Comment:
It might be faster to decode to a temporary buffer, to allow vectorized unpacking, but definitely something that can be done as a follow up
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928131391
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r926325032
##########
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() {
+ let mut rng = thread_rng();
+ let len: usize = rng.gen_range(512..1024) * 8;
+
+ let mut expected = BooleanBufferBuilder::new(len);
+ let mut encoder = RleEncoder::new(1, 1024 * 8);
+
+ for _ in 0..len {
+ let bool = rng.gen_bool(0.8);
+ assert!(encoder.put(bool as u64).unwrap());
+ expected.append(bool);
+ }
+ assert_eq!(expected.len(), len);
+
+ let encoded = encoder.consume().unwrap();
+ let mut decoder = PackedDecoder::new(Encoding::RLE, ByteBufferPtr::new(encoded));
+
+ let mut skip_value = 0;
+ let mut read_value = 0;
+ let mut read_data = vec![];
+
+ loop {
+ let remaining = len - read_value - skip_value;
+ if remaining == 0 {
+ break;
+ }
+ let to_read = (rng.gen_range(1..=remaining)) / 8 * 8;
+ if rng.gen_bool(0.5) {
+ skip_value += decoder.skip(to_read).unwrap();
+ } else if to_read > 0 {
+ let mut decoded = BooleanBufferBuilder::new(to_read);
+ read_value += decoder.read(&mut decoded, to_read).unwrap();
+ read_data.push(decoded.as_slice().to_vec());
+ }
+ }
+
+ assert_eq!(read_value + skip_value, len);
+
+ let expected = expected.as_slice();
+ for data in read_data.iter().enumerate() {
+ assert!(find_subsequence(expected, data.1).is_some());
Review Comment:
fix at (https://github.com/apache/arrow-rs/pull/2111/commits/e7a68af7d2f9ffcf1d3d0e4379f3e52ca6057c72)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928132492
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
Thanks, i will read carefully. Seems you know the everything about parquet 😄
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r925727366
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -307,10 +307,28 @@ impl ColumnLevelDecoder for ColumnLevelDecoderImpl {
impl DefinitionLevelDecoder for ColumnLevelDecoderImpl {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
@tustvold Thanks for your kindly explanation!
>Unfortunately max_def_level = 1 is an insufficient condition for using a PackedDecoder, as the nulls may not exist at the leaf level.
You are right, i found it at add the integration testing😂.
>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.
Got it ! I think after [7a2fa01](https://github.com/apache/arrow-rs/pull/2111/commits/7a2fa01e3f048447e61740aa02a12cd768ae11b6) add skip_def in `ColumnLevelDecoderImpl`. I think It's enough to decode all types col with `R:0 D:1` ? If i miss something plz tell me 😭
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928127548
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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 `1,2,3`
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#issuecomment-1193136661
> #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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r925716839
##########
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() {
+ let mut rng = thread_rng();
+ let len: usize = rng.gen_range(512..1024) * 8;
+
+ let mut expected = BooleanBufferBuilder::new(len);
+ let mut encoder = RleEncoder::new(1, 1024 * 8);
+
+ for _ in 0..len {
+ let bool = rng.gen_bool(0.8);
+ assert!(encoder.put(bool as u64).unwrap());
+ expected.append(bool);
+ }
+ assert_eq!(expected.len(), len);
+
+ let encoded = encoder.consume().unwrap();
+ let mut decoder = PackedDecoder::new(Encoding::RLE, ByteBufferPtr::new(encoded));
+
+ let mut skip_value = 0;
+ let mut read_value = 0;
+ let mut read_data = vec![];
+
+ loop {
+ let remaining = len - read_value - skip_value;
+ if remaining == 0 {
+ break;
+ }
+ let to_read = (rng.gen_range(1..=remaining)) / 8 * 8;
Review Comment:
a little hack here, because i use `u8` to compare equal. But the decoder is one bit. So i think if want to compare bit equal in `u8`, should read or skip in 8 bit 🤣
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#issuecomment-1190778267
I've created https://github.com/apache/arrow-rs/pull/2116 which should hopefully help with the DefinitionLevelBufferDecoder logic, which I think is currently incorrect in this PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928122873
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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 😂?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#issuecomment-1192777599
#2117 has now been merged which should make it possible to significantly simplify this PR (i.e. remove the max_def_level shenanigans)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r925719281
##########
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() {
+ let mut rng = thread_rng();
+ let len: usize = rng.gen_range(512..1024) * 8;
+
+ let mut expected = BooleanBufferBuilder::new(len);
+ let mut encoder = RleEncoder::new(1, 1024 * 8);
+
+ for _ in 0..len {
+ let bool = rng.gen_bool(0.8);
+ assert!(encoder.put(bool as u64).unwrap());
+ expected.append(bool);
+ }
+ assert_eq!(expected.len(), len);
+
+ let encoded = encoder.consume().unwrap();
+ let mut decoder = PackedDecoder::new(Encoding::RLE, ByteBufferPtr::new(encoded));
+
+ let mut skip_value = 0;
+ let mut read_value = 0;
+ let mut read_data = vec![];
+
+ loop {
+ let remaining = len - read_value - skip_value;
+ if remaining == 0 {
+ break;
+ }
+ let to_read = (rng.gen_range(1..=remaining)) / 8 * 8;
+ if rng.gen_bool(0.5) {
+ skip_value += decoder.skip(to_read).unwrap();
+ } else if to_read > 0 {
+ let mut decoded = BooleanBufferBuilder::new(to_read);
+ read_value += decoder.read(&mut decoded, to_read).unwrap();
+ read_data.push(decoded.as_slice().to_vec());
+ }
+ }
+
+ assert_eq!(read_value + skip_value, len);
+
+ let expected = expected.as_slice();
+ for data in read_data.iter().enumerate() {
+ assert!(find_subsequence(expected, data.1).is_some());
Review Comment:
Is there any API i can reuse? 🤣 I got stuck at bit change
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928235258
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
@tustvold [6e6d39c](https://github.com/apache/arrow-rs/pull/2111/commits/6e6d39cbdb23bf6ebe63420b633d259bccd367f7) PTAL
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold merged pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold merged PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928123393
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r925682705
##########
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() {
+ let mut rng = thread_rng();
+ let len: usize = rng.gen_range(512..1024) * 8;
+
+ let mut expected = BooleanBufferBuilder::new(len);
+ let mut encoder = RleEncoder::new(1, 1024 * 8);
+
+ for _ in 0..len {
+ let bool = rng.gen_bool(0.8);
+ assert!(encoder.put(bool as u64).unwrap());
+ expected.append(bool);
+ }
+ assert_eq!(expected.len(), len);
+
+ let encoded = encoder.consume().unwrap();
+ let mut decoder = PackedDecoder::new(Encoding::RLE, ByteBufferPtr::new(encoded));
+
+ let mut skip_value = 0;
+ let mut read_value = 0;
+ let mut read_data = vec![];
+
+ loop {
+ let remaining = len - read_value - skip_value;
+ if remaining == 0 {
+ break;
+ }
+ let to_read = (rng.gen_range(1..=remaining)) / 8 * 8;
Review Comment:
Why do we need to read in multiples of 8?
##########
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() {
+ let mut rng = thread_rng();
+ let len: usize = rng.gen_range(512..1024) * 8;
+
+ let mut expected = BooleanBufferBuilder::new(len);
+ let mut encoder = RleEncoder::new(1, 1024 * 8);
+
+ for _ in 0..len {
+ let bool = rng.gen_bool(0.8);
+ assert!(encoder.put(bool as u64).unwrap());
+ expected.append(bool);
+ }
+ assert_eq!(expected.len(), len);
+
+ let encoded = encoder.consume().unwrap();
+ let mut decoder = PackedDecoder::new(Encoding::RLE, ByteBufferPtr::new(encoded));
+
+ let mut skip_value = 0;
+ let mut read_value = 0;
+ let mut read_data = vec![];
+
+ loop {
+ let remaining = len - read_value - skip_value;
+ if remaining == 0 {
+ break;
+ }
+ let to_read = (rng.gen_range(1..=remaining)) / 8 * 8;
+ if rng.gen_bool(0.5) {
+ skip_value += decoder.skip(to_read).unwrap();
+ } else if to_read > 0 {
+ let mut decoded = BooleanBufferBuilder::new(to_read);
+ read_value += decoder.read(&mut decoded, to_read).unwrap();
+ read_data.push(decoded.as_slice().to_vec());
+ }
+ }
+
+ assert_eq!(read_value + skip_value, len);
+
+ let expected = expected.as_slice();
+ for data in read_data.iter().enumerate() {
+ assert!(find_subsequence(expected, data.1).is_some());
Review Comment:
Perhaps rather than this method, which will yield false positives, we could keep the original full set of bools, and build up the expected result as we go?
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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`.
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
+ let decoder = match self.data.take() {
+ Some(data) => self
+ .packed_decoder
+ .insert(PackedDecoder::new(self.encoding, data)),
Review Comment:
Again this isn't technically correct, as `PackedDecoder` can only be used if the leaf is the nullable column (and not a parent).
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -307,10 +307,28 @@ impl ColumnLevelDecoder for ColumnLevelDecoderImpl {
impl DefinitionLevelDecoder for ColumnLevelDecoderImpl {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
Again this is incorrect, should just be a case of returning nyi_err if not PackedDecoder
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#issuecomment-1193136814
@Ted-Jiang I meant #2116
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928241471
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -318,10 +318,41 @@ impl ColumnLevelDecoder for ColumnLevelDecoderImpl {
impl DefinitionLevelDecoder for ColumnLevelDecoderImpl {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> Result<(usize, usize)> {
- Err(nyi_err!("https://github.com/apache/arrow-rs/issues/1792"))
+ let mut level_skip = 0;
+ let mut value_skip = 0;
+ match self.decoder.as_mut().unwrap() {
+ LevelDecoderInner::Packed(reader, bit_width) => {
+ for _ in 0..num_levels {
+ // Values are delimited by max_def_level
+ if max_def_level
+ == reader
Review Comment:
will test in #2106
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928273409
##########
parquet/src/column/reader/decoder.rs:
##########
@@ -318,10 +318,41 @@ impl ColumnLevelDecoder for ColumnLevelDecoderImpl {
impl DefinitionLevelDecoder for ColumnLevelDecoderImpl {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> Result<(usize, usize)> {
- Err(nyi_err!("https://github.com/apache/arrow-rs/issues/1792"))
+ let mut level_skip = 0;
+ let mut value_skip = 0;
+ match self.decoder.as_mut().unwrap() {
+ LevelDecoderInner::Packed(reader, bit_width) => {
+ for _ in 0..num_levels {
Review Comment:
Agree, Add #2153
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928131391
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -226,10 +226,27 @@ impl ColumnLevelDecoder for DefinitionLevelBufferDecoder {
impl DefinitionLevelDecoder for DefinitionLevelBufferDecoder {
fn skip_def_levels(
&mut self,
- _num_levels: usize,
- _max_def_level: i16,
+ num_levels: usize,
+ max_def_level: i16,
) -> 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 {
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928274742
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -248,7 +254,9 @@ struct PackedDecoder {
impl PackedDecoder {
fn next_rle_block(&mut self) -> Result<()> {
- let indicator_value = self.decode_header()?;
+ let indicator_value = self
+ .decode_header()
+ .expect("decode_header fail in PackedDecoder");
Review Comment:
Oh! my fault. it should use previous code pass the error. fix in [fix comment](https://github.com/apache/arrow-rs/pull/2111/commits/79e03b9206b0805c6ae2848f0fd3f0051ba63483)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928273120
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -248,7 +254,9 @@ struct PackedDecoder {
impl PackedDecoder {
fn next_rle_block(&mut self) -> Result<()> {
- let indicator_value = self.decode_header()?;
+ let indicator_value = self
+ .decode_header()
+ .expect("decode_header fail in PackedDecoder");
Review Comment:
Before met not set `decode_header ` when coding. Better have the error info.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r928273217
##########
parquet/src/arrow/record_reader/definition_levels.rs:
##########
@@ -248,7 +254,9 @@ struct PackedDecoder {
impl PackedDecoder {
fn next_rle_block(&mut self) -> Result<()> {
- let indicator_value = self.decode_header()?;
+ let indicator_value = self
+ .decode_header()
+ .expect("decode_header fail in PackedDecoder");
Review Comment:
Before we propogated the error to the caller, instead of panicking?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] Ted-Jiang commented on pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
Ted-Jiang commented on PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#issuecomment-1189949006
@tustvold PTAL 😊, this block the #2106
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] liukun4515 commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
liukun4515 commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r925472285
##########
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() {
Review Comment:
👍
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow-rs] tustvold commented on a diff in pull request #2111: Support skip_def_levels (only max_def_levels=1) for ColumnLevelDecoder
Posted by GitBox <gi...@apache.org>.
tustvold commented on code in PR #2111:
URL: https://github.com/apache/arrow-rs/pull/2111#discussion_r926066176
##########
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() {
+ let mut rng = thread_rng();
+ let len: usize = rng.gen_range(512..1024) * 8;
+
+ let mut expected = BooleanBufferBuilder::new(len);
+ let mut encoder = RleEncoder::new(1, 1024 * 8);
+
+ for _ in 0..len {
+ let bool = rng.gen_bool(0.8);
+ assert!(encoder.put(bool as u64).unwrap());
+ expected.append(bool);
+ }
+ assert_eq!(expected.len(), len);
+
+ let encoded = encoder.consume().unwrap();
+ let mut decoder = PackedDecoder::new(Encoding::RLE, ByteBufferPtr::new(encoded));
+
+ let mut skip_value = 0;
+ let mut read_value = 0;
+ let mut read_data = vec![];
+
+ loop {
+ let remaining = len - read_value - skip_value;
+ if remaining == 0 {
+ break;
+ }
+ let to_read = (rng.gen_range(1..=remaining)) / 8 * 8;
+ if rng.gen_bool(0.5) {
+ skip_value += decoder.skip(to_read).unwrap();
+ } else if to_read > 0 {
+ let mut decoded = BooleanBufferBuilder::new(to_read);
+ read_value += decoder.read(&mut decoded, to_read).unwrap();
+ read_data.push(decoded.as_slice().to_vec());
+ }
+ }
+
+ assert_eq!(read_value + skip_value, len);
+
+ let expected = expected.as_slice();
+ for data in read_data.iter().enumerate() {
+ assert!(find_subsequence(expected, data.1).is_some());
Review Comment:
I'm suggesting where you write the data to the encoder, that you also build up an expected VecDequeue there. Then when you are decoding, you pop the elements off, potentially appending them to an expected vec. Then you can just compare the two?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org