Uh oh!
There was an error while loading. Please reload this page.
GH-34138: [C++][Parquet] Fix parsing stats from min_value/max_value - #34112
Conversation
wgtmac
commented
Feb 10, 2023
@pitrou@wjones127@westonpace Could you please take a look? |
| } | ||
| const format::Statistics& stats = header.statistics; | ||
| if (stats.__isset.max) { | ||
| // Use the new V2 min-max statistics over the former one if it is filled |
There was a problem hiding this comment.
Previously, page_statistics will handle min-max separately. This patch changes it to once have all min-max, otherwise, cannot use min-max
There was a problem hiding this comment.
Yes, page stats without either min or max is corrupted and cannot be used. Check parquet-mr for detail: https://github.com/apache/parquet-mr/blob/5290bd5e0ee5dc30db0576e2bfc6eea335c465cf/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java#L797
There was a problem hiding this comment.
Although in parquet.thrift, min-max can exist only one. But I think handling it like this is ok
There was a problem hiding this comment.
So we revert back to previous mode that only has min or max is ok?
westonpace
commented
Feb 10, 2023
Does this mean the following (testing my understanding)? If min is set but max is not set then we should ignore both min and max |
wjones127
commented
Feb 10, 2023
Yes, it does mean we will. Do you foresee that as an issue? It sounds like Java implementation takes the same approach. |
wgtmac
commented
Feb 11, 2023
When only one of min and max exists, it usually happens when a binary value has an extreme length or a floating value has NaN. In this case, the stats provide little value and make it tricker to use. |
wgtmac
commented
Feb 11, 2023
I have created a new issue for this PR and updated the title. A separate issue has been created for the TODO items: #34139 Thanks for the review! @wjones127 |
| page_statistics.set_max(stats.max_value); | ||
| page_statistics.set_min(stats.min_value); | ||
| } else if (stats.__isset.max && stats.__isset.min) { | ||
| // TODO: check created_by to see if it is corrupted for some types. |
There was a problem hiding this comment.
I've no problem here, but just curious, does this code meaning the parquet-mr's CorruptStatistics.shouldIgnoreStatistics? (It's really trickey...)
wgtmac
commented
Feb 13, 2023
Gentle ping @wjones127@pitrou Once this gets merged, I will rebase #34107 which is blocked by it. |
emkornfield
commented
Feb 15, 2023
CC @fatemehp |
westonpace
commented
Feb 16, 2023
In datasets, for row group statistics, we recently added a check that was roughly... In other words, if one of min or max is NaN then we still use the other side of the equality. I think my primary concern is to validate that is a safe assumption. In other words, I want to make sure we aren't using garbage data in our handling of row groups. |
wjones127
commented
Feb 16, 2023
@westonpace that makes sense.
It seems like we do have handling for these two cases. See Weston's message for NaN handling and |
| if (stats.__isset.max_value && stats.__isset.min_value) { | ||
| // TODO: check if the column_order is TYPE_DEFINED_ORDER. | ||
| page_statistics.set_max(stats.max_value); | ||
| page_statistics.set_min(stats.min_value); |
There was a problem hiding this comment.
| if (stats.__isset.max_value && stats.__isset.min_value) { | |
| // TODO: check if the column_order is TYPE_DEFINED_ORDER. | |
| page_statistics.set_max(stats.max_value); | |
| page_statistics.set_min(stats.min_value); | |
| if (stats.__isset.max_value || stats.__isset.min_value) { | |
| // TODO: check if the column_order is TYPE_DEFINED_ORDER. | |
| if (stats.__isset.max_value) { | |
| page_statistics.set_max(stats.max_value); | |
| } | |
| if (stats.__isset.min_value) { | |
| page_statistics.set_min(stats.min_value); | |
| } |
There was a problem hiding this comment.
Fixed. Please take a look again. Thanks @wjones127 !
westonpace
commented
Feb 17, 2023
Just for clarity, the PR I linked (and my thoughts) were about how we currently handle row group statistics. I'm not sure if the rules are identical for page statistics. I mainly wanted to make sure my understanding of the row group statistics wasn't invalid. |
wgtmac
commented
Feb 17, 2023
IIUC, row group statistics are aggregated from page statistics so they should share the same rules. The parquet thrift message definition does allow only one side of min or max exist: /** * Statistics per row group and per page * All fields are optional.*/structStatistics {
/** * DEPRECATED: min and max value of the column. Use min_value and max_value. * * Values are encoded using PLAIN encoding, except that variable-length byte * arrays do not include a length prefix. * * These fields encode min and max values determined by signed comparison * only. New files should use the correct order for a column's logical type * and store the values in the min_value and max_value fields. * * To support older readers, these may be set when the column order is * signed.*/1:optionalbinarymax;
2:optionalbinarymin;
/** count of null value in the column */3:optionali64null_count;
/** count of distinct values occurring */4:optionali64distinct_count;
/** * Min and max values for the column, determined by its ColumnOrder. * * Values are encoded using PLAIN encoding, except that variable-length byte * arrays do not include a length prefix.*/5:optionalbinarymax_value;
6:optionalbinarymin_value;
}On the other side, the story of page index is different. The column index definition does require existence of both min and max values if it is not a null page: /** * Description for ColumnIndex. * Each <array-field>[i] refers to the page at OffsetIndex.page_locations[i]*/structColumnIndex {
/** * A list of Boolean values to determine the validity of the corresponding * min and max values. If true, a page contains only null values, and writers * have to set the corresponding entries in min_values and max_values to * byte[0], so that all lists have the same length. If false, the * corresponding entries in min_values and max_values must be valid.*/1:requiredlist<bool> null_pages/** * Two lists containing lower and upper bounds for the values of each page * determined by the ColumnOrder of the column. These may be the actual * minimum and maximum values found on a page, but can also be (more compact) * values that do not exist on a page. For example, instead of storing ""Blart * Versenwald III", a writer may set min_values[i]="B", max_values[i]="C". * Such more compact values must still be valid values within the column's * logical type. Readers must make sure that list entries are populated before * using them by inspecting null_pages.*/2:requiredlist<binary> min_values3:requiredlist<binary> max_values/** * Stores whether both min_values and max_values are ordered and if so, in * which direction. This allows readers to perform binary searches in both * lists. Readers cannot assume that max_values[i] <= min_values[i+1], even * if the lists are ordered.*/4:requiredBoundaryOrderboundary_order/** A list containing the number of null values for each page **/5:optionallist<i64> null_counts
}So I am fine with parsing only one side min or max values from page/row group statistics. @westonpace@wjones127 |
The CI build is failed due to a recent branch rename and will be fixed by #34218 |
wgtmac
commented
Feb 17, 2023
@westonpace Could you please take another pass? |
ursabot
commented
Feb 17, 2023
Benchmark runs are scheduled for baseline = 1264e40 and contender = 8e5e438. 8e5e438 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
Rationale for this change
The code below does not read from stats.min_value/max_value at all.
What changes are included in this PR?
Do similar thing from parquet-mr to check and read min_value/max_value from thrift stats.
Are these changes tested?
Some test cases fail after the fix. Fixed them to make sure it is covered.
Are there any user-facing changes?
No.