wgtmac commented on code in PR #3607:
URL: https://github.com/apache/parquet-java/pull/3607#discussion_r3480359879
##########
parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java:
##########
@@ -47,20 +47,25 @@ public class CorruptStatistics {
private static final SemanticVersion CDH_5_PARQUET_251_FIXED_END = new
SemanticVersion(1, 5, 0);
/**
- * Decides if the statistics from a file created by createdBy (the
created_by field from parquet format)
- * should be ignored because they are potentially corrupt.
+ * Returns whether the given column type is one of the types affected by the
PARQUET-251 bug
+ * (BINARY or FIXED_LEN_BYTE_ARRAY).
*
- * @param createdBy the created-by string from a file footer
- * @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
+ * @param columnType the primitive type of the column
+ * @return true if this column type could have corrupt statistics
*/
- public static boolean shouldIgnoreStatistics(String createdBy,
PrimitiveTypeName columnType) {
-
- if (columnType != PrimitiveTypeName.BINARY && columnType !=
PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY) {
- // the bug only applies to binary columns
- return false;
- }
+ public static boolean isCorruptStatisticsColumnType(PrimitiveTypeName
columnType) {
Review Comment:
IMHO, we need to be cautious when adding a new public method because we have
to maintain it forever. Can we remove it from here and just let downstream
where calls it to use a private method instead?
##########
parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java:
##########
@@ -47,20 +47,25 @@ public class CorruptStatistics {
private static final SemanticVersion CDH_5_PARQUET_251_FIXED_END = new
SemanticVersion(1, 5, 0);
/**
- * Decides if the statistics from a file created by createdBy (the
created_by field from parquet format)
- * should be ignored because they are potentially corrupt.
+ * Returns whether the given column type is one of the types affected by the
PARQUET-251 bug
+ * (BINARY or FIXED_LEN_BYTE_ARRAY).
*
- * @param createdBy the created-by string from a file footer
- * @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
+ * @param columnType the primitive type of the column
+ * @return true if this column type could have corrupt statistics
*/
- public static boolean shouldIgnoreStatistics(String createdBy,
PrimitiveTypeName columnType) {
-
- if (columnType != PrimitiveTypeName.BINARY && columnType !=
PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY) {
- // the bug only applies to binary columns
- return false;
- }
+ public static boolean isCorruptStatisticsColumnType(PrimitiveTypeName
columnType) {
+ return columnType == PrimitiveTypeName.BINARY || columnType ==
PrimitiveTypeName.FIXED_LEN_BYTE_ARRAY;
+ }
+ /**
+ * Determines whether a file (identified by its created_by string) was
written by a version of
+ * parquet-mr that had the PARQUET-251 statistics bug. This is a file-level
check that does not
+ * consider column type.
+ *
+ * @param createdBy the created-by string from a file footer
+ * @return true if the file was written by a version with the corrupt
statistics bug
+ */
+ public static boolean fileHasCorruptStatistics(String createdBy) {
Review Comment:
Rename it to `mayHaveCorruptStatistics` ?
--
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]
