emkornfield commented on code in PR #197:
URL: https://github.com/apache/parquet-format/pull/197#discussion_r1302257411
##########
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:
> 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?
The information would be necessary for both fixed width types, accounting
for "size" of variable width data, and also estimating size of repeated fields
(these might not apply to your use-case, depending on how nulls are handled
> Sorry for trying to piggy-back on your PR 😅 If you think this should be a
new issue I could start a discussion on the dev list.
I think it is valuable feedback. Since I don't have bandwidth to do the
initial implementations, I'm not one to speak on scope creep.
My initial thoughts is if we were to add this in another place OffsetIndex
make more sense then here.
To summarize open questions, we need to decide:
1. Whether to use SizeStatistics on ColumnIndex (my sense is no), we should
stick with histograms if we keep anything?
2. Whether to add SizeStatistics to OffsetSetIndex (I'm somewhat indifferent
to this other then it might not have wide applicability and replicates date)?
3. It seems like there isn't a current use-case for having interleaved on
pages.
##########
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:
> 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?
The information would be necessary for both fixed width types, accounting
for "size" of variable width data, and also estimating size of repeated fields
(these might not apply to your use-case, depending on how nulls are handled
> Sorry for trying to piggy-back on your PR 😅 If you think this should be a
new issue I could start a discussion on the dev list.
I think it is valuable feedback. Since I don't have bandwidth to do the
initial implementations, I'm not one to speak on scope creep.
My initial thoughts is if we were to add this in another place OffsetIndex
make more sense then here.
To summarize open questions, we need to decide:
1. Whether to use SizeStatistics on ColumnIndex (my sense is no), we should
stick with histograms if we keep anything?
2. Whether to add SizeStatistics to OffsetSetIndex (I'm somewhat indifferent
to this other then it might not have wide applicability and replicates date)?
3. It seems like there isn't a current use-case for having interleaved on
pages.
--
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
