alamb commented on code in PR #245:
URL: https://github.com/apache/parquet-format/pull/245#discussion_r1606765182
##########
src/main/thrift/parquet.thrift:
##########
@@ -1017,8 +1020,10 @@ struct OffsetIndex {
}
/**
- * Description for ColumnIndex.
- * Each <array-field>[i] refers to the page at OffsetIndex.page_locations[i]
+ * "Page Index": Optional statistics for each data page in a ColumnChunk.
Review Comment:
Thank you. I think this is addressed in
0afdcd951b022eba0fdb81bde1ee390c502a329f
##########
src/main/thrift/parquet.thrift:
##########
@@ -738,10 +738,10 @@ struct PageHeader {
}
/**
- * Wrapper struct to specify sort order
+ * Sort order within a RowGroup
*/
struct SortingColumn {
- /** The column index (in this row group) **/
+ /** The ordinal position of the column (in this row group) **/
Review Comment:
I updated this to avoid potential confusion with `ColumnIndex` which is a
different structure
##########
src/main/thrift/parquet.thrift:
##########
@@ -1017,8 +1020,10 @@ struct OffsetIndex {
}
/**
- * Description for ColumnIndex.
- * Each <array-field>[i] refers to the page at OffsetIndex.page_locations[i]
+ * "Page Index": Optional statistics for each data page in a ColumnChunk.
Review Comment:
I am glad I opened this PR as I was clearly confused about what people were
talking about when they said page index !
I addressed in 0afdcd951b022eba0fdb81bde1ee390c502a329f (by using the term
"page index"). Let me know what you think
##########
PageIndex.md:
##########
@@ -19,9 +19,11 @@
# ColumnIndex Layout to Support Page Skipping
-This document describes the format for column index pages in the Parquet
-footer. These pages contain statistics for DataPages and can be used to skip
-pages when scanning data in ordered and unordered columns.
+In Parquet, a "Page Index" is optional metadata for a
Review Comment:
After some thought, I think using "page index" (non capitalized) might be
best because there while `ColumnIndex` and `OffsetIndex` are names of thrift
structs, `PageIndex` is not. So referring to `PageIndex` as though it were a
struct I think will cause more confusion
I tried to clarify this in 0afdcd9.
##########
src/main/thrift/parquet.thrift:
##########
@@ -738,10 +738,10 @@ struct PageHeader {
}
/**
- * Wrapper struct to specify sort order
+ * Sort order within a RowGroup
*/
struct SortingColumn {
- /** The column index (in this row group) **/
+ /** The ordinal position of the column (in this row group) **/
Review Comment:
in c7e3ae7
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
