wgtmac commented on PR #3700:
URL: https://github.com/apache/parquet-java/pull/3700#issuecomment-5450573700
Sorry for the delay. I still think the current approach is not clean. To
avoid more roundtrip discussion, I've put up a draft as the diff below. Let me
know what you think.
```
diff --git
a/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
b/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
index 546b9bdf..85493efc 100644
--- a/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
+++ b/parquet-column/src/main/java/org/apache/parquet/CorruptStatistics.java
@@ -19,7 +19,6 @@
package org.apache.parquet;
import java.util.concurrent.atomic.AtomicBoolean;
-import org.apache.parquet.SemanticVersion.SemanticVersionParseException;
import org.apache.parquet.VersionParser.ParsedVersion;
import org.apache.parquet.VersionParser.VersionParseException;
import org.apache.parquet.schema.PrimitiveType.PrimitiveTypeName;
@@ -110,11 +109,7 @@ public class CorruptStatistics {
}
if (!writerVersion.hasSemanticVersion()) {
- try {
- SemanticVersion.parse(writerVersion.version);
- } catch (SemanticVersionParseException e) {
- warnParseErrorOnce(createdBy, e);
- }
+ warnParseErrorOnce(createdBy,
writerVersion.getSemanticVersionParseFailure());
return true;
}
diff --git
a/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
b/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
index ff950a25..ba4668ad 100644
---
a/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
+++
b/parquet-column/src/test/java/org/apache/parquet/CorruptStatisticsTest.java
@@ -162,6 +162,8 @@ public class CorruptStatisticsTest {
// version field present but not a valid semantic version
ParsedVersion invalidSemver = new ParsedVersion("parquet-mr",
"not-a-semver", "abc");
assertThat(invalidSemver.hasSemanticVersion()).isFalse();
+ assertThat(invalidSemver.getSemanticVersionParseFailure())
+ .isInstanceOf(SemanticVersion.SemanticVersionParseException.class);
assertThat(CorruptStatistics.shouldIgnoreStatistics(
invalidSemver, "parquet-mr version not-a-semver (build abc)",
PrimitiveTypeName.BINARY))
.isTrue();
diff --git
a/parquet-common/src/main/java/org/apache/parquet/VersionParser.java
b/parquet-common/src/main/java/org/apache/parquet/VersionParser.java
index 07feb0d3..82e45eb8 100644
--- a/parquet-common/src/main/java/org/apache/parquet/VersionParser.java
+++ b/parquet-common/src/main/java/org/apache/parquet/VersionParser.java
@@ -41,6 +41,7 @@ public class VersionParser {
private final boolean hasSemver;
private final SemanticVersion semver;
+ private final Exception semanticVersionParseFailure;
public ParsedVersion(String application, String version, String
appBuildHash) {
checkArgument(!Strings.isNullOrEmpty(application), "application
cannot be null or empty");
@@ -48,17 +49,20 @@ public class VersionParser {
this.version = Strings.isNullOrEmpty(version) ? null : version;
this.appBuildHash = Strings.isNullOrEmpty(appBuildHash) ? null :
appBuildHash;
- SemanticVersion sv;
- boolean hasSemver;
- try {
- sv = SemanticVersion.parse(version);
- hasSemver = true;
- } catch (RuntimeException | SemanticVersionParseException e) {
- sv = null;
- hasSemver = false;
+ SemanticVersion sv = null;
+ boolean hasSemver = false;
+ Exception parseFailure = null;
+ if (this.version != null) {
+ try {
+ sv = SemanticVersion.parse(this.version);
+ hasSemver = true;
+ } catch (RuntimeException | SemanticVersionParseException e) {
+ parseFailure = e;
+ }
}
this.semver = sv;
this.hasSemver = hasSemver;
+ this.semanticVersionParseFailure = parseFailure;
}
public boolean hasSemanticVersion() {
@@ -69,6 +73,14 @@ public class VersionParser {
return semver;
}
+ /**
+ * Returns the exception captured when parsing the semantic version
failed, or {@code null} if
+ * parsing succeeded.
+ */
+ Exception getSemanticVersionParseFailure() {
+ return semanticVersionParseFailure;
+ }
+
@Override
public boolean equals(Object o) {
if (this == o) return true;
diff --git
a/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
b/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
index 1f2fd6e4..8f2dd852 100644
---
a/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
+++
b/parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java
@@ -947,44 +947,7 @@ public class ParquetMetadataConverter {
// Visible for testing
static org.apache.parquet.column.statistics.Statistics
fromParquetStatisticsInternal(
String createdBy, Statistics formatStats, PrimitiveType type,
SortOrder typeSortOrder) {
- org.apache.parquet.column.statistics.Statistics.Builder statsBuilder =
-
org.apache.parquet.column.statistics.Statistics.getBuilderForReading(type);
-
- if (formatStats != null) {
- // Use the new V2 min-max statistics over the former one if it is
filled
- if (formatStats.isSetMin_value() && formatStats.isSetMax_value()) {
- byte[] min = formatStats.min_value.array();
- byte[] max = formatStats.max_value.array();
- if (isMinMaxStatsSupported(type) || Arrays.equals(min, max)) {
- statsBuilder.withMin(min);
- statsBuilder.withMax(max);
- }
- } else {
- 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
- // The sort order is checked to avoid returning min/max stats that
are not
- // valid with the type's sort order. In previous releases, all
stats were
- // aggregated using a signed byte-wise ordering, which isn't valid
for all the
- // types (e.g. strings, decimals etc.).
- if (!CorruptStatistics.shouldIgnoreStatistics(createdBy,
type.getPrimitiveTypeName())
- && (sortOrdersMatch || maxEqualsMin)) {
- if (isSet) {
- statsBuilder.withMin(formatStats.min.array());
- statsBuilder.withMax(formatStats.max.array());
- }
- }
- }
-
- if (formatStats.isSetNull_count()) {
- statsBuilder.withNumNulls(formatStats.null_count);
- }
- if (formatStats.isSetNan_count()) {
- statsBuilder.withNanCount(formatStats.getNan_count());
- }
- }
- return statsBuilder.build();
+ return fromParquetStatisticsInternal(null, createdBy, formatStats,
type, typeSortOrder);
}
// Visible for testing
@@ -1015,7 +978,10 @@ public class ParquetMetadataConverter {
// valid with the type's sort order. In previous releases, all
stats were
// aggregated using a signed byte-wise ordering, which isn't valid
for all the
// types (e.g. strings, decimals etc.).
- if (!CorruptStatistics.shouldIgnoreStatistics(writerVersion,
createdBy, type.getPrimitiveTypeName())
+ boolean shouldIgnoreStatistics = writerVersion == null
+ ? CorruptStatistics.shouldIgnoreStatistics(createdBy,
type.getPrimitiveTypeName())
+ : CorruptStatistics.shouldIgnoreStatistics(writerVersion,
createdBy, type.getPrimitiveTypeName());
+ if (!shouldIgnoreStatistics
&& (sortOrdersMatch || maxEqualsMin)) {
if (isSet) {
statsBuilder.withMin(formatStats.min.array());
@@ -1875,20 +1841,7 @@ public class ParquetMetadataConverter {
public ColumnChunkMetaData buildColumnChunkMetaData(
ColumnMetaData metaData, ColumnPath columnPath, PrimitiveType type,
String createdBy) {
- return ColumnChunkMetaData.get(
- columnPath,
- type,
- fromFormatCodec(metaData.codec),
- convertEncodingStats(metaData.getEncoding_stats()),
- fromFormatEncodings(metaData.encodings),
- fromParquetStatistics(createdBy, metaData.statistics, type),
- metaData.data_page_offset,
- metaData.dictionary_page_offset,
- metaData.num_values,
- metaData.total_compressed_size,
- metaData.total_uncompressed_size,
- fromParquetSizeStatistics(metaData.size_statistics, type),
- fromParquetStatistics(metaData.geospatial_statistics, type));
+ return buildColumnChunkMetaData(metaData, columnPath, type, null,
createdBy);
}
public ColumnChunkMetaData buildColumnChunkMetaData(
@@ -1934,10 +1887,8 @@ public class ParquetMetadataConverter {
buildFileMetaData(parquetMetadata, messageType, encryptedFooter,
fileDecryptor);
String createdBy = fileMetaData.getCreatedBy();
ParsedVersion writerVersion = null;
- boolean useWriterVersion = false;
try {
writerVersion = fileMetaData.getWriterVersion();
- useWriterVersion = true;
} catch (VersionParseException e) {
// Fall back to String-based path which logs the parse error with
full context
}
@@ -2020,10 +1971,8 @@ public class ParquetMetadataConverter {
if (!lazyMetadataDecryption) { // full column metadata (with
stats) is available
PrimitiveType primitiveType =
messageType.getType(columnPath.toArray()).asPrimitiveType();
- column = useWriterVersion
- ? buildColumnChunkMetaData(
- metaData, columnPath, primitiveType, writerVersion,
createdBy)
- : buildColumnChunkMetaData(metaData, columnPath,
primitiveType, createdBy);
+ column = buildColumnChunkMetaData(
+ metaData, columnPath, primitiveType, writerVersion,
createdBy);
column.setRowGroupOrdinal(rowGroup.getOrdinal());
if (metaData.isSetBloom_filter_offset()) {
column.setBloomFilterOffset(metaData.getBloom_filter_offset());
```
--
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]
