GH-3242: Emit and Read min/max statistics for int96 timestamp columns #3243

rahulketch · 2025-06-12T10:08:41Z

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?

Defining column ordering for int96
Implementation of comparator for int96
A flag parquet.read.int96stats.enabled to control if the stats are read. It is defaulted to true
ValidInt96Stats: Reads stats from known good writers. Currently including:
a. parquet-mr 1.15.0+
b. photon
Fixing tests which fail due to this change

Are these changes tested?

Added more unit tests for the new behaviour

Are there any user-facing changes?

toParquetStatistics and fromParquetStatistics are no longer static functions. It doesn't look like there is a good reason for these functions to be static.
Signatures of some of the constructors of ParquetMetadataConverter is now different due to an added parameter.

Closes #3242

emkornfield · 2025-06-13T16:33:21Z

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.

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java

parquet-column/src/main/java/org/apache/parquet/ValidInt96Stats.java

rahulketch · 2025-06-25T10:14:47Z

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.

@emkornfield : I created the PR: apache/parquet-format#503

rahulketch · 2025-07-01T11:58:21Z

After this change, toParquetStatistics and fromParquetStatistics in ParquetMetadataConverter are no longer static functions. This breaks a test on compatibility (see here). @emkornfield @rdblue: Do you have advice on how to move forward with those changes?

rahulketch · 2025-07-23T20:59:52Z

@emkornfield @rdblue Given the latest message in the mailing thread, can we merge this? If so, could you help me with addressing this issue

emkornfield · 2025-07-23T22:32:28Z

@emkornfield @rdblue Given the latest message in the mailing thread, can we merge this? If so, could you help me with addressing this issue

Sorry I'm not sure on the API change. Lets get (lazy) consensus on the message for this direction. I think @rdblue had the strongest opinion on SortOrder

rahulketch · 2025-07-24T12:03:11Z

After this change, toParquetStatistics and fromParquetStatistics in ParquetMetadataConverter are no longer static functions. This breaks a test on compatibility (see here). @emkornfield @rdblue: Do you have advice on how to move forward with those changes?

@wgtmac : could you help with this?

rahulketch · 2025-09-03T21:55:49Z

After this change, toParquetStatistics and fromParquetStatistics in ParquetMetadataConverter are no longer static functions. This breaks a test on compatibility (see here). @emkornfield @rdblue: Do you have advice on how to move forward with those changes?

@wgtmac : could you help with this?

@wgtmac : Could you help with this, or suggest who could help out? Thanks!

wgtmac · 2025-09-05T09:05:28Z

@rahulketch Sorry I'm too busy these days. Perhaps I can find some time next week.

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java

wgtmac · 2025-09-08T00:58:01Z

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java

+  static final PrimitiveComparator<Binary> BINARY_AS_INT96_TIMESTAMP_COMPARATOR = new BinaryComparator() {
+    @Override
+    int compareBinary(Binary b1, Binary b2) {
+      ByteBuffer bb1 = b1.toByteBuffer().slice();


Should we check if their lengths are exactly 12 before anything? I recall that BINARY_AS_FLOAT16_COMPARATOR did this.

parquet-column/src/main/java/org/apache/parquet/ValidInt96Stats.java

wgtmac · 2025-09-08T04:05:00Z

parquet-column/src/main/java/org/apache/parquet/ValidInt96Stats.java

+
+    try {
+      ParsedVersion version = VersionParser.parse(createdBy);
+      if ("parquet-mr".equals(version.application)) {


I recall that @rdblue has an opinion in maintaining an allow-list here.

parquet-column/src/main/java/org/apache/parquet/ValidInt96Stats.java

wgtmac · 2025-09-08T05:32:32Z

parquet-hadoop/src/main/java/org/apache/parquet/format/converter/ParquetMetadataConverter.java


-  public static Statistics toParquetStatistics(org.apache.parquet.column.statistics.Statistics stats) {
-    return toParquetStatistics(stats, ParquetProperties.DEFAULT_STATISTICS_TRUNCATE_LENGTH);
+  public Statistics toParquetStatistics(String createdBy, org.apache.parquet.column.statistics.Statistics stats) {


Isn't it a breaking change?

I am not sure of a better way to achieve this. We need to:

Know created-by in order to have an allow-list for valid readers

Make the function non-static so we can respect the flag which disables reading the stats.

Any ideas for this?

Should we just follow the existing pattern of toParquetStatistics to introduce yet another overload?

public static Statistics toParquetStatistics( org.apache.parquet.column.statistics.Statistics stats, int truncateLength, bool convertInt96Stats)

@gszadovszky WDYT?

I agree with @wgtmac.

Or, we can deprecate the static methods (marking them for removal in 2.0) and create non-static ones to be used from now on.

…veComparator.java Co-authored-by: Gang Wu <ustcwg@gmail.com>

…s.java Co-authored-by: Gang Wu <ustcwg@gmail.com>

rahulketch added 8 commits June 11, 2025 11:11

Add round trip int 96 stats test

0267c33

ValidInt96Stats

9a6a205

Add BINARY_AS_INT_96_COMPARATOR

91e47db

Pass readInt96stats

beb7ad3

Compile correctly

e1e2e8d

Fix more tests

26c3ddd

Formatting

d65b7b8

Fix the comparator

21bf8ae

emkornfield reviewed Jun 13, 2025

View reviewed changes

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java Show resolved Hide resolved

emkornfield reviewed Jun 13, 2025

View reviewed changes

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java Show resolved Hide resolved

etseidl mentioned this pull request Jun 17, 2025

GH-7686: [Parquet] Fix int96 min/max stats apache/arrow-rs#7687

Merged

rdblue reviewed Jun 18, 2025

View reviewed changes

parquet-column/src/main/java/org/apache/parquet/schema/PrimitiveComparator.java Outdated Show resolved Hide resolved

rdblue reviewed Jun 18, 2025

View reviewed changes

parquet-column/src/main/java/org/apache/parquet/ValidInt96Stats.java Outdated Show resolved Hide resolved

rahulketch mentioned this pull request Jun 25, 2025

GH-502: Define ordering for int96 timestamp apache/parquet-format#503

Closed

rahulketch added 4 commits June 30, 2025 15:42

Merge branch 'master' into int96-stats-imp

43028d7

BINARY_AS_INT_96_COMPARATOR -> BINARY_AS_INT96_TIMESTAMP_COMPARATOR

db9231c

Add clarification on the comparator

85f3c3c

Check parquet-mr version

5eef533

rahulketch requested review from rdblue, alkis and emkornfield June 30, 2025 16:17

Add slicing to fix bug with the comparator

efacf01

wgtmac reviewed Sep 8, 2025

View reviewed changes

rahulketch and others added 2 commits September 9, 2025 14:51

Update parquet-column/src/main/java/org/apache/parquet/schema/Primiti…

2df49cb

…veComparator.java Co-authored-by: Gang Wu <ustcwg@gmail.com>

Update parquet-column/src/main/java/org/apache/parquet/ValidInt96Stat…

90656f1

…s.java Co-authored-by: Gang Wu <ustcwg@gmail.com>

rahulketch force-pushed the int96-stats-imp branch from b107ae8 to 90656f1 Compare September 9, 2025 15:02

GH-3242: Emit and Read min/max statistics for int96 timestamp columns #3243

Are you sure you want to change the base?

GH-3242: Emit and Read min/max statistics for int96 timestamp columns #3243

Uh oh!

Conversation

rahulketch commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

emkornfield commented Jun 13, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rahulketch commented Jun 25, 2025

Uh oh!

rahulketch commented Jul 1, 2025

Uh oh!

rahulketch commented Jul 23, 2025

Uh oh!

emkornfield commented Jul 23, 2025

Uh oh!

rahulketch commented Jul 24, 2025

Uh oh!

rahulketch commented Sep 3, 2025

Uh oh!

wgtmac commented Sep 5, 2025

Uh oh!

Uh oh!

wgtmac Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

wgtmac Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

wgtmac Sep 8, 2025

Choose a reason for hiding this comment

Uh oh!

rahulketch Sep 9, 2025

Choose a reason for hiding this comment

Uh oh!

wgtmac Sep 12, 2025

Choose a reason for hiding this comment

Uh oh!

gszadovszky Sep 15, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rahulketch commented Jun 12, 2025 •

edited

Loading