You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "etseidl (via GitHub)" <gi...@apache.org> on 2024/07/23 20:33:22 UTC
[PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
etseidl opened a new pull request, #6105:
URL: https://github.com/apache/arrow-rs/pull/6105
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #5022.
# Rationale for this change
Final piece of the new `SizeStatistics`. Supercedes #5486.
# What changes are included in this PR?
Adds repetition and definition level histograms to `ParquetMetaData` and to the `PageIndex` struct (representation of the thrift `ColumnIndex`).
# Are there any user-facing changes?
Yes.
<!--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2251464912
Merge went ok...once #6126 merges I'll rebase this on master.
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2253012008
> Hi @etseidl -- could you please retarget this PR at the `main` branch so I can merge it in?
done
> I also made [etseidl#1](https://github.com/etseidl/arrow-rs/pull/1) for your consideration (that make the level histogram a struct). We could also merge this PR as is and consider that as a follow on.
I too created a `LevelHistogram` struct last night...yours is much more elegant 😄. I'm game to merge your PR into my branch before merging this into master. Or you can submit yours as a follow on PR. Whichever is easier for you.
> Thank you for bearing with me here.
Thank *you* for all the help and for the free Rust schooling 😄
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#discussion_r1693392323
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -1045,6 +1216,28 @@ impl ColumnIndexBuilder {
self.null_counts.push(null_count);
}
+ /// Append the given page-level histograms to the [`ColumnIndex`] histograms.
+ /// Does nothing if the `ColumnIndexBuilder` is not in the `valid` state.
+ pub fn append_histograms(
+ &mut self,
+ repetition_level_histogram: &Option<LevelHistogram>,
+ definition_level_histogram: &Option<LevelHistogram>,
+ ) {
+ if !self.valid {
+ return;
+ }
+ if let Some(ref rep_lvl_hist) = repetition_level_histogram {
+ let hist = self.repetition_level_histograms.get_or_insert(Vec::new());
+ hist.reserve(rep_lvl_hist.len());
+ hist.extend(rep_lvl_hist.values());
Review Comment:
I went back and forth with this -- I think I was thinking this was building the thrift structure so we should use the thrift representation (`Vec<i64>`) but I may have gotten that wrong as I find the naming quite confusing
How about we explore doing it as a follow on 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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2251400644
> The only thing I could think that we might want to explore is using a rust struct rather than a `Option<Vec<i64>` so it could be easier to use / document. I'll make a PR to explore what that looks like
❤️
Yes, while I was reviewing my changes I was thinking the same thing. A histogram class that supports updates, unions, and concatenation.
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb closed pull request #6105: Add support for level histograms added in PARQUET-2261 to `ParquetMetaData`
URL: https://github.com/apache/arrow-rs/pull/6105
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "etseidl (via GitHub)" <gi...@apache.org>.
etseidl commented on code in PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#discussion_r1693378223
##########
parquet/src/file/metadata/mod.rs:
##########
@@ -1045,6 +1216,28 @@ impl ColumnIndexBuilder {
self.null_counts.push(null_count);
}
+ /// Append the given page-level histograms to the [`ColumnIndex`] histograms.
+ /// Does nothing if the `ColumnIndexBuilder` is not in the `valid` state.
+ pub fn append_histograms(
+ &mut self,
+ repetition_level_histogram: &Option<LevelHistogram>,
+ definition_level_histogram: &Option<LevelHistogram>,
+ ) {
+ if !self.valid {
+ return;
+ }
+ if let Some(ref rep_lvl_hist) = repetition_level_histogram {
+ let hist = self.repetition_level_histograms.get_or_insert(Vec::new());
+ hist.reserve(rep_lvl_hist.len());
+ hist.extend(rep_lvl_hist.values());
Review Comment:
@alamb should we also replace the vectors in `ColumnIndexBuilder` with `LevelHistogram`? We could add an `append()` method to `LevelHistogram` to reduce some code duplication here.
Same question for `PageIndex`.
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2253198541
> Thank you for all the help and for the free Rust schooling 😄
That is half the fun of working on these projects -- I see a lot of cool things while reviewing / working on PRs
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2253199248
Thanks again @etseidl -- I merged this to main and let's keep working to improve things there.
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2252439240
I merged the 53 dev branch and that seems to have closed this PR -- any chance you can retarget main?
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2251462463
Update for anyone following along: https://github.com/apache/arrow-rs/pull/6126 is the merge of the dev branch to main
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2253193760
Let's do this!
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2252619635
Hi @etseidl -- could you please retarget this PR at the `main` branch so I can merge it in?
I also made https://github.com/etseidl/arrow-rs/pull/1 for your consideration (that make the level histogram a struct). We could also merge this PR as is and consider that as a follow on.
Thank you for bearing with me here.
--
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
Re: [PR] Add support for level histograms added in PARQUET-2261 to `ParquetMetaData` [arrow-rs]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #6105:
URL: https://github.com/apache/arrow-rs/pull/6105#issuecomment-2251391164
The clippy failure is likely due to the new rust version which was released today. I fixed it on main. Also since we have made a 52.2.0 release candidate I think we can move `53.0.0` development off of `53.0.0-dev` to main. I will create a PR to do so
--
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