GH-3242: Emit and Read min/max statistics for int96 timestamp columns #3243
Conversation
|
It looks like Alkis started a discussion on the ML, but we probably want to come to a consensus and update https://github.com/apache/parquet-format/blob/master/src/main/thrift/parquet.thrift#L1079 first before merging this. |
| bb2.order(java.nio.ByteOrder.LITTLE_ENDIAN); | ||
| int jd1 = bb1.getInt(8); | ||
| int jd2 = bb2.getInt(8); | ||
| if (jd1 != jd2) return Integer.compareUnsigned(jd1, jd2) < 0 ? -1 : 1; |
There was a problem hiding this comment.
shouldn't this be signed comparison? Year 0001-01-01 is in the wild, and I think that would be a negative value for days since julien epoch?
There was a problem hiding this comment.
Never mind, I think this is actually counting from something pre-year 1.
| ByteBuffer bb1 = b1.toByteBuffer(); | ||
| ByteBuffer bb2 = b2.toByteBuffer(); | ||
| bb1.order(java.nio.ByteOrder.LITTLE_ENDIAN); | ||
| bb2.order(java.nio.ByteOrder.LITTLE_ENDIAN); |
There was a problem hiding this comment.
this logic should probably be commented.
| /* | ||
| * This comparator is for comparing two timestamps represented as int96 binary. | ||
| */ | ||
| static final PrimitiveComparator<Binary> BINARY_AS_INT_96_COMPARATOR = new BinaryComparator() { |
There was a problem hiding this comment.
Shouldn't this be AS_INT96_TIMESTAMP? This is not a comparison that would work for int96.
| try { | ||
| ParsedVersion version = VersionParser.parse(createdBy); | ||
| if ("parquet-mr".equals(version.application)) { | ||
| return true; |
There was a problem hiding this comment.
Wouldn't this cause all previous stats to be valid? Do we know that is the case?
There was a problem hiding this comment.
Previously parquet-mr does not emit stats for int96 columns. After this PR is merged int96 stats will be emitted so they're safe to read.
There was a problem hiding this comment.
Were int96 stats ever emitted? I don't remember.
Rationale for this change
Parquet-java does not emit or read stats for int96 timestamp columns. Since int96 is used as the default timestamp in spark, this limits a lot of optimization opportunities. Engines like Photon populate the statistics for the int96 timestamps correctly. So parquet-java can also emit the statistics, and also allow reading these statistics from known good writers.
What changes are included in this PR?
parquet.read.int96stats.enabledto control if the stats are read. It is defaulted to trueValidInt96Stats: Reads stats from known good writers. Currently including:a.
parquet-mrb.
photonAre these changes tested?
Are there any user-facing changes?
toParquetStatisticsandfromParquetStatisticsare no longer static functions. It doesn't look like there is a good reason for these functions to be static.ParquetMetadataConverteris not different due to an added parameter.Closes #3242