Use LevelHistogram in PageIndex
#6135
Conversation
| @@ -569,7 +569,7 @@ pub struct ColumnChunkMetaData { | |||
| /// For example, `vec[0]` is the number of rows with level 0, `vec[1]` is the | |||
| /// number of rows with level 1, and so on. | |||
| /// | |||
| #[derive(Debug, Clone, PartialEq)] | |||
| #[derive(Debug, Clone, PartialEq, Eq, Hash, Default)] | |||
parquet/src/file/metadata/mod.rs
Outdated
| @@ -620,6 +620,12 @@ impl LevelHistogram { | |||
| } | |||
| } | |||
|
|
|||
| /// Appends values from the other histogram to this histogram | |||
There was a problem hiding this comment.
I found the "append" notion quite confusing which is why I didn't put it in this struct at first
Basically without append we have the invariant that histogram[i] represents the counts of level i
Using append breaks that invariant. If we are going to keep it, maybe we can add some more comments like
| /// Appends values from the other histogram to this histogram | |
| /// Appends values from the other histogram to this histogram | |
| /// | |
| /// For example, given `[1,2,3]` and `[4,5]` calling append will result | |
| /// in `[1,2,3,4,5]` (not `[5,7,3]`) |
There was a problem hiding this comment.
You're right, appending to a histogram makes it no longer a histogram :P I'll revert this part.
parquet/src/file/metadata/mod.rs
Outdated
| @@ -1169,9 +1175,9 @@ pub struct ColumnIndexBuilder { | |||
| null_counts: Vec<i64>, | |||
| boundary_order: BoundaryOrder, | |||
| /// contains the concatenation of the histograms of all pages | |||
| repetition_level_histograms: Option<Vec<i64>>, | |||
| repetition_level_histograms: Option<LevelHistogram>, | |||
There was a problem hiding this comment.
because ColumnIndexBuilder is a builder for the format::metadata structs (aka it is basically building up the serialized form of several LevelHistograms, and needs append) I would find it clearer to use Vec<i64> here
| @@ -40,13 +41,13 @@ pub struct PageIndex<T> { | |||
| /// | |||
| /// `repetition_level_histogram[i]` is a count of how many values are at repetition level `i`. | |||
| /// For example, `repetition_level_histogram[0]` indicates how many rows the page contains. | |||
| pub repetition_level_histogram: Option<Vec<i64>>, | |||
| pub repetition_level_histogram: Option<LevelHistogram>, | |||
There was a problem hiding this comment.
👍 this is a good one -- I missed this
etseidl
changed the title
LevelHistogram in PageIndex and ColumnIndexBuilderLevelHistogram in PageIndex
Thanks @alamb, you are correct here. I was too fixated on which side of the |
Which issue does this PR close?
Closes #6134.
Rationale for this change
Makes all histograms outside the
formatmodule share the same interface.What changes are included in this PR?
Changes remaining
Vec<i64>histograms toLevelHistogram.Are there any user-facing changes?
Yes, this contains API changes.