Simplify `ColumnReader::read_batch` #1995

tustvold · 2022-07-03T01:54:36Z

Which issue does this PR close?

Miscellaneous cleanup whilst working on #1792

Closes #1996
Closes #1997

Rationale for this change

The existing code had redundant result returns, duplicated logic, and confusing semantics.

What changes are included in this PR?

Changes the semantics of ColumnReader::read_batch to error if called on data without the necessary levels buffers, and to return the number of levels read even for columns that have max_def_level == 0 && max_rep_level == 0. This is technically a breaking change, although I think it makes the API a lot less confusing.

Cleanups some additional code

Are there any user-facing changes?

Yes

tustvold · 2022-07-03T01:59:39Z

parquet/src/arrow/record_reader/mod.rs


    use super::RecordReader;

-    struct TestPageReader {


This just duplicated InMemoryPageReader

tustvold · 2022-07-03T02:02:15Z

parquet/src/column/reader.rs

@@ -163,26 +163,18 @@ where
        }
    }

-    /// Reads a batch of values of at most `batch_size`.
+    /// Reads a batch of values of at most `batch_size`, returning a tuple containing the


This is the breaking change, we could make this non-breaking by always returning 0 if max_def_level == 0 && max_rep_level == 0, but I personally think this is a little bit confusing. This make the behaviour consistent, instead of potentially varying based on the column definition.

To be honest, I don't fully understand the subtle difference between corresponding number of levels, i.e, the total number of values including nulls, empty lists, etc.. and the actual number of levels read. but it seems like a good change to me and I would defer to you for what the most appropriate semantics should be

The difference arises because if a column is repeated / nullable, the corresponding level data is omitted. The result is that whilst there is still the same number of levels, it technically won't read any level data

tustvold · 2022-07-03T02:02:31Z

parquet/src/column/reader.rs

-        }
-    }
-
-    struct TestPageReader {


Another duplicate of InMemoryPageReader

tustvold · 2022-07-03T02:02:57Z

parquet/src/column/reader.rs

@@ -1300,44 +1272,10 @@ mod tests {
                );
            }

-            if def_levels.is_none() && rep_levels.is_none() {


Here we see the breaking change in behaviour

tustvold · 2022-07-03T02:03:46Z

parquet/src/util/test_common/page_util.rs

-            "Must call `add_rep_levels() first!`"
-        );
-
+        self.num_values = def_levels.len() as u32;


This logic was ill-formed, as it prevented having def levels without rep levels. Removing this sanity check is harmless, especially as this is just used for testing

Double checked DataPageBuilder is used for testing:

https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/apache/arrow-rs%24+DataPageBuilderImpl&patternType=literal

https://docs.rs/arrow/17.0.0/arrow/?search=DataPageBuilder

👍

tustvold · 2022-07-03T02:17:35Z

parquet/src/column/reader.rs

+                        .read(levels, levels_read..levels_read + iter_batch_size)?;
+
+                    if num_def_levels != iter_batch_size {
+                        return Err(general_err!("insufficient definition levels read from column - expected {}, got {}", iter_batch_size, num_def_levels));


This, and similar checks below, will prevent #1997

tustvold · 2022-07-03T02:18:28Z

parquet/src/column/reader.rs

-            if num_def_levels != 0
-                && num_rep_levels != 0


This is the source of #1997 - the value of 0 is used as a sentinel for no levels present, which prevents detecting the case of no actual values left

codecov-commenter · 2022-07-03T02:18:54Z

Codecov Report

Merging #1995 (d7a306f) into master (7ae97c9) will increase coverage by 0.00%.
The diff coverage is 92.30%.

@@           Coverage Diff           @@
##           master    #1995   +/-   ##
=======================================
  Coverage   83.58%   83.58%           
=======================================
  Files         222      222           
  Lines       57495    57467   -28     
=======================================
- Hits        48056    48035   -21     
+ Misses       9439     9432    -7

Impacted Files	Coverage Δ
parquet/src/arrow/arrow_reader.rs	`96.87% <ø> (ø)`
parquet/src/column/reader.rs	`68.31% <81.81%> (-0.30%)`	⬇️
parquet/src/arrow/array_reader/byte_array.rs	`85.71% <100.00%> (ø)`
...et/src/arrow/array_reader/byte_array_dictionary.rs	`83.91% <100.00%> (ø)`
parquet/src/arrow/array_reader/null_array.rs	`88.00% <100.00%> (ø)`
parquet/src/arrow/array_reader/primitive_array.rs	`89.65% <100.00%> (ø)`
parquet/src/arrow/record_reader/mod.rs	`94.73% <100.00%> (+1.16%)`	⬆️
parquet/src/util/test_common/page_util.rs	`88.54% <100.00%> (-0.12%)`	⬇️
parquet_derive/src/parquet_field.rs	`65.75% <0.00%> (-0.23%)`	⬇️
... and 2 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 7ae97c9...d7a306f. Read the comment docs.

Miscellaneous parquet cleanups

alamb

I am not an expert in this area of the code, but I read this PR carefully and it makes sense to me and it was easier to read. Thank you @tustvold

cc @sunchao @nevi-me

alamb · 2022-07-05T11:42:38Z

parquet/src/column/reader.rs

@@ -163,26 +163,18 @@ where
        }
    }

-    /// Reads a batch of values of at most `batch_size`.
+    /// Reads a batch of values of at most `batch_size`, returning a tuple containing the


To be honest, I don't fully understand the subtle difference between corresponding number of levels, i.e, the total number of values including nulls, empty lists, etc.. and the actual number of levels read. but it seems like a good change to me and I would defer to you for what the most appropriate semantics should be

alamb · 2022-07-05T11:47:34Z

parquet/src/column/reader.rs

@@ -1036,7 +1005,7 @@ mod tests {
        } else {
            0
        };
-        let max_rep_level = if def_levels.is_some() {


alamb · 2022-07-05T11:51:01Z

parquet/src/util/test_common/page_util.rs

-            "Must call `add_rep_levels() first!`"
-        );
-
+        self.num_values = def_levels.len() as u32;


Double checked DataPageBuilder is used for testing:

https://sourcegraph.com/search?q=context:global+repo:%5Egithub%5C.com/apache/arrow-rs%24+DataPageBuilderImpl&patternType=literal

https://docs.rs/arrow/17.0.0/arrow/?search=DataPageBuilder

👍

alamb · 2022-07-05T11:53:03Z

parquet/src/arrow/record_reader/mod.rs

@@ -218,32 +218,32 @@ where
    /// The implementation has side effects. It will create a new buffer to hold those
    /// definition level values that have already been read into memory but not counted
    /// as record values, e.g. those from `self.num_values` to `self.values_written`.
-    pub fn consume_def_levels(&mut self) -> Result<Option<Buffer>> {
-        Ok(match self.def_levels.as_mut() {
+    pub fn consume_def_levels(&mut self) -> Option<Buffer> {


The changes in this file are cleanups to make the signature infallible when it always returns Ok, right? (and it is also an API change, and perhaps also fixes clippy)

tustvold added the api-change Changes to the arrow API label Jul 3, 2022

github-actions bot added arrow Changes to the arrow crate parquet Changes to the parquet crate labels Jul 3, 2022

tustvold force-pushed the parquet-cleanup branch from 5b4d248 to 12c044f Compare July 3, 2022 01:59

tustvold commented Jul 3, 2022

View reviewed changes

parquet/src/column/reader.rs

}

}

struct TestPageReader {

Copy link

Contributor Author

tustvold Jul 3, 2022

Choose a reason for hiding this comment

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

Another duplicate of InMemoryPageReader

tustvold commented Jul 3, 2022

View reviewed changes

tustvold force-pushed the parquet-cleanup branch from 12c044f to 8b57c0c Compare July 3, 2022 02:12

tustvold commented Jul 3, 2022

View reviewed changes

Simplify ColumnReader::read_batch

d7a306f

Miscellaneous parquet cleanups

tustvold force-pushed the parquet-cleanup branch from 8b57c0c to d7a306f Compare July 3, 2022 02:20

tustvold mentioned this pull request Jul 3, 2022

Stub out Skip Records API (#1792) #1998

Merged

alamb approved these changes Jul 5, 2022

View reviewed changes

tustvold merged commit c757829 into apache:master Jul 5, 2022

alamb mentioned this pull request Jul 7, 2022

Release Arrow 18.0.0 (next release after 17.0.0) #1952

Closed

4 tasks

alamb changed the title ~~Simplify ColumnReader::read_batch~~ Simplify ColumnReader::read_batch Jul 7, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify `ColumnReader::read_batch` #1995

Simplify `ColumnReader::read_batch` #1995

tustvold commented Jul 3, 2022 •

edited

tustvold Jul 3, 2022

tustvold Jul 3, 2022

alamb Jul 5, 2022

tustvold Jul 5, 2022

tustvold Jul 3, 2022

tustvold Jul 3, 2022

tustvold Jul 3, 2022

alamb Jul 5, 2022

tustvold Jul 3, 2022

tustvold Jul 3, 2022

codecov-commenter commented Jul 3, 2022 •

edited

alamb left a comment

alamb Jul 5, 2022

alamb Jul 5, 2022

alamb Jul 5, 2022

alamb Jul 5, 2022

Simplify ColumnReader::read_batch #1995

Simplify ColumnReader::read_batch #1995

Conversation

tustvold commented Jul 3, 2022 • edited

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

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 3, 2022 • edited

Codecov Report

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

Simplify `ColumnReader::read_batch` #1995

Simplify `ColumnReader::read_batch` #1995

tustvold commented Jul 3, 2022 •

edited

codecov-commenter commented Jul 3, 2022 •

edited