asifsmohammed commented on code in PR #3700:
URL: https://github.com/apache/parquet-java/pull/3700#discussion_r3723288688
##########
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()) {
+ warnOnce("Ignoring statistics because created_by could not be parsed
(see PARQUET-251): " + writerVersion);
+ return true;
+ }
+
+ SemanticVersion semver = writerVersion.getSemanticVersion();
Review Comment:
`ParsedVersion` eagerly parses and caches the `SemanticVersion` in its
constructor, so `getSemanticVersion()` avoids the redundant
`SemanticVersion.parse(version.version)` that the String-based overload
previously performed on every call. The left and right spikes in flame graph
are for parsing `SemanticVersion` twice.
<img width="2558" height="624" alt="Image"
src="https://github.com/user-attachments/assets/71787343-a2e6-48e6-b0eb-7e6b4e41127e";
/>
--
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]
