wgtmac commented on code in PR #3700:
URL: https://github.com/apache/parquet-java/pull/3700#discussion_r3829110143
##########
parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java:
##########
@@ -70,37 +69,60 @@ public static boolean shouldIgnoreStatistics(String
createdBy, PrimitiveTypeName
try {
ParsedVersion version = VersionParser.parse(createdBy);
+ return shouldIgnoreStatistics(version, columnType);
+ } catch (RuntimeException | VersionParseException e) {
+ warnParseErrorOnce(createdBy, e);
Review Comment:
```suggestion
// couldn't parse the created_by field, log what went wrong, don't
trust the
// stats, but don't make this fatal.
warnParseErrorOnce(createdBy, e);
```
Let's keep the original comment.
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -962,13 +982,7 @@ static org.apache.parquet.column.statistics.Statistics
fromParquetStatisticsInte
boolean isSet = formatStats.isSetMax() && formatStats.isSetMin();
boolean maxEqualsMin = isSet ? Arrays.equals(formatStats.getMin(),
formatStats.getMax()) : false;
boolean sortOrdersMatch = SortOrder.SIGNED == typeSortOrder;
- // NOTE: See docs in CorruptStatistics for explanation of why this
check is needed
Review Comment:
Could we preserve these comments?
##########
parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java:
##########
@@ -70,37 +69,60 @@ public static boolean shouldIgnoreStatistics(String
createdBy, PrimitiveTypeName
try {
ParsedVersion version = VersionParser.parse(createdBy);
+ return shouldIgnoreStatistics(version, columnType);
+ } catch (RuntimeException | VersionParseException e) {
+ warnParseErrorOnce(createdBy, e);
+ return true;
+ }
+ }
+
+ /**
+ * Decides if the statistics from a file should be ignored because they are
potentially corrupt.
+ * Use this overload when the writer version has already been parsed to
avoid redundant parsing.
+ *
+ * @param writerVersion the pre-parsed writer version, or {@code null} if
unknown/unparseable
+ * @param columnType the type of the column that this is checking
+ * @return true if the statistics may be invalid and should be ignored,
false otherwise
+ */
+ public static boolean shouldIgnoreStatistics(ParsedVersion writerVersion,
PrimitiveTypeName columnType) {
Review Comment:
This overload makes calls like `shouldIgnoreStatistics(null, type)`
ambiguous; the cast in the new test demonstrates the source incompatibility.
Could we use a distinct method name for the `ParsedVersion` path, including the
new converter overloads?
##########
parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java:
##########
@@ -945,7 +947,25 @@ public static
org.apache.parquet.column.statistics.Statistics fromParquetStatist
// Visible for testing
static org.apache.parquet.column.statistics.Statistics
fromParquetStatisticsInternal(
String createdBy, Statistics formatStats, PrimitiveType type, SortOrder
typeSortOrder) {
- // create stats object based on the column type
+ return fromParquetStatisticsInternal(
+ CorruptStatistics.shouldIgnoreStatistics(createdBy,
type.getPrimitiveTypeName()),
Review Comment:
This evaluates `shouldIgnoreStatistics` even when stats are null or V2
min/max is used, so it may log “Ignoring statistics” and consume the one-shot
warning when nothing is ignored. Could we keep this check inside the legacy
min/max branch and add a regression test?
##########
parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java:
##########
@@ -70,37 +69,60 @@ public static boolean shouldIgnoreStatistics(String
createdBy, PrimitiveTypeName
try {
ParsedVersion version = VersionParser.parse(createdBy);
+ return shouldIgnoreStatistics(version, columnType);
+ } catch (RuntimeException | VersionParseException e) {
+ warnParseErrorOnce(createdBy, e);
+ return true;
+ }
+ }
+
+ /**
+ * Decides if the statistics from a file should be ignored because they are
potentially corrupt.
+ * Use this overload when the writer version has already been parsed to
avoid redundant parsing.
+ *
+ * @param writerVersion the pre-parsed writer version, or {@code null} if
unknown/unparseable
+ * @param columnType the type of the column that this is checking
+ * @return true if the statistics may be invalid and should be ignored,
false otherwise
+ */
+ public static boolean shouldIgnoreStatistics(ParsedVersion writerVersion,
PrimitiveTypeName columnType) {
- if (!"parquet-mr".equals(version.application)) {
- // assume other applications don't have this bug
- return false;
- }
-
- if (Strings.isNullOrEmpty(version.version)) {
- warnOnce("Ignoring statistics because created_by did not contain a
semver (see PARQUET-251): "
- + createdBy);
- return true;
- }
-
- SemanticVersion semver = SemanticVersion.parse(version.version);
-
- if (semver.compareTo(PARQUET_251_FIXED_VERSION) < 0
- && !(semver.compareTo(CDH_5_PARQUET_251_FIXED_START) >= 0
- && semver.compareTo(CDH_5_PARQUET_251_FIXED_END) < 0)) {
- warnOnce("Ignoring statistics because this file was created prior to "
- + PARQUET_251_FIXED_VERSION
- + ", see PARQUET-251");
- return true;
- }
-
- // this file was created after the fix
+ if (columnType != PrimitiveTypeName.BINARY && columnType !=
PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY) {
return false;
- } catch (RuntimeException | SemanticVersionParseException |
VersionParseException e) {
- // couldn't parse the created_by field, log what went wrong, don't trust
the stats,
- // but don't make this fatal.
- warnParseErrorOnce(createdBy, e);
+ }
+
+ if (writerVersion == null) {
+ warnOnce("Ignoring statistics because created_by is null or empty! See
PARQUET-251 and PARQUET-297");
return true;
}
+
+ if (!"parquet-mr".equals(writerVersion.application)) {
+ return false;
+ }
+
+ if (Strings.isNullOrEmpty(writerVersion.version)) {
+ warnOnce("Ignoring statistics because created_by did not contain a
semver (see PARQUET-251): "
+ + writerVersion);
+ return true;
+ }
+
+ if (!writerVersion.hasSemanticVersion()) {
Review Comment:
`ParsedVersion` has already swallowed `SemanticVersionParseException` here,
so this no longer preserves the old `warnParseErrorOnce(createdBy, e)`
behavior. Could we keep the original string and parse exception for this path?
--
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]
