etseidl commented on code in PR #197:
URL: https://github.com/apache/parquet-format/pull/197#discussion_r1302246559
##########
src/main/thrift/parquet.thrift:
##########
@@ -974,6 +1050,13 @@ struct ColumnIndex {
/** A list containing the number of null values for each page **/
5: optional list<i64> null_counts
+ /**
+ * Repetition and definition level histograms for the pages.
+ *
+ * This contains some redundancy with null_counts, however, to accommodate
the
+ * widest range of readers both should be populated.
+ **/
+ 6: optional list<RepetitionDefinitionLevelHistogram>
repetition_definition_level_histograms;
Review Comment:
> A few clarifying questions here:
>
> 1. Does page level detail add value here? As @mapleFU stated We have
this information at the column chunk level already.
Yes. What I'm trying to do is a log-structured merge with parquet files
sorted by a particular column. This is being done on a GPU as opposed to a set
of streaming readers, so sizing info needs to be known up front. What I do is
take page sizing info across the set of files, calculate page ranges that can
fit into GPU RAM, and then translate those page ranges to key ranges based on
the ColumnIndex. Chunk level statistics are too coarse grained for this
purpose.
>
> 2. Is there a reason to use the index here vs having the data present
on page headers (and read as you scan)?
Yes, since we're operating on a GPU, we're not streaming. Having to scan the
file to find page headers would be a non-starter. Having the info in the footer
would be a boon.
>
> 3. If we need to add this to OffsetIndex, it seems like we would want
SizeEstimateStatistics in full?
I'd have to think on this...I just need the overall sizing info. I'm not
sure how I'd use the histogram, although I'm assuming it's used to get data
sizes for fixed width data?
> At least for the initial version I was trying to limit places to add new
stastistics and identify if the other cases are actual problems. As long as
others are open to it do we could add it elsewhere but I get the sense the some
of the utility might be more limited.
Understandable. I know sticking it into the `OffsetIndex` is kind of weird.
I was originally thinking of adding a new structure like `OffsetIndex` and
placing that in the file before the other indexes, and then adding a pointer to
the info in the ColumnChunk.
Sorry for trying to piggy-back on your PR :sweat_smile: If you think this
should be a new issue I could start a discussion on the dev list.
--
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: dev-unsubscr...@parquet.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
