asifsmohammed commented on code in PR #3700:
URL: https://github.com/apache/parquet-java/pull/3700#discussion_r3833145177
##########
parquet-hadoop/src/main/java/org/apache/parquet/hadoop/metadata/FileMetaData.java:
##########
@@ -42,6 +44,8 @@ public enum EncryptionType {
private final MessageType schema;
private final Map<String, String> keyValueMetaData;
private final String createdBy;
+ private transient volatile ParsedVersion writerVersion;
+ private transient volatile boolean writerVersionParsed;
Review Comment:
**Update**: Both edge cases are now resolved. Added `createdBy` as a
parameter so log messages use the raw string, and the `!hasSemanticVersion()`
branch re-parses to recreate the exception with stack trace. Full parity
achieved for all scenarios.
##########
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:
Added String `createdBy` as a parameter to the `ParsedVersion` overload, so
this signature (`ParsedVersion`, `String`, `PrimitiveTypeName`) is no longer
ambiguous with (`String`, `PrimitiveTypeName`). The `createdBy` parameter also
solves the logging parity issue mentioned below comment. Converter overloads
follow the same pattern.
##########
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:
`createdBy` string is now passed as a parameter, and the
`!hasSemanticVersion()` branch re-parses `writerVersion.version` to recreate
the `SemanticVersionParseException` for `warnParseErrorOnce(createdBy, e)`.
This gives exact log parity (original string + stack trace). The re-parse only
fires when the `ParsedVersion` fails to parse it, so zero performance impact on
the hot 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]
