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

rahulketch · 2025-06-17T14:03:56Z

Which issue does this PR close?

Closes Parquet: Incorrect min/max stats for int96 columns #7686

Rationale for this change

int96 min/max statistics emitted by arrow-rs are incorrect.

What changes are included in this PR?

Fix the int96 stats
Add round-trip test to verify the behavior

Not included in this PR:

Read stats only from known good writers. This will be implemented after a new arrow-rs release.

Are there any user-facing changes?

The int96 min/max statistics will be different and correct.

…stats

parquet/src/data_type.rs

etseidl · 2025-06-17T17:39:31Z

I tend to agree with @emkornfield (apache/parquet-java#3243 (comment)) that this is a bit of putting the cart before the horse. The sort order for INT96 is currently undefined so statistics should be ignored. I think we need changes to the Parquet spec before proceeding with this.

etseidl · 2025-07-02T21:09:55Z

I think we've achieved sufficient consensus to move forward with this. @rahulketch do you have time to address the outstanding issues here (@alamb's suggestion, CI errors, etc)? I have cycles to clean this up if you're short on time. Thanks!

rahulketch · 2025-07-03T09:43:29Z

@etseidl I have pushed the changes which I believe address the open concerns.

There is still the task of adding an allow/deny list to only accept the statistics from known good writer. See the corresponding change in parquet-java.

My suggestion for that is:

Merge this change and make a new arrow-rs release 56.0.0
After the new release add a check which allows reading the stats from:

parquet-java 1.15.0+
arrow-rs 56.0.0+
photon

What do you think?

PS: I will be on vacation 4th July - 15th July, so I'll only be able to make more changes after that.

etseidl · 2025-07-03T13:48:21Z

Thanks @rahulketch! I've taken the liberty of fixing the remaining lint errors.

There is still the task of adding an allow/deny list to only accept the statistics from known good writer.

I agree that this is a follow on task and can be done after this PR merges. I think it will need to be part of a larger review of statistics handling to see if other types with undefined sort orders are also ignored.

etseidl

I think this is good to go now. @alamb @emkornfield can you take another look? 🙏

alamb

Looks good to me -- thank you @rahulketch and @etseidl

parquet/src/data_type.rs

Co-authored-by: Alkis Evlogimenos <[email protected]>

alamb · 2025-07-14T15:30:16Z

@emkornfield this PR looks good to merge from my perspective and has several approvals and I think addresses your feedback.

If you would like more time to review, please let us know, otherwise I plan to merge this PR tomorrow

emkornfield · 2025-07-14T16:20:29Z

At the last sync we went back and forth on what was required for this from a spec perspective.

I think general consensus is we should update the spec (I think there is a PR open for this).

Beyond that there was discussion on whether we should have a new sort order or just rely on versions. I'll start a thread but #7909 is pertinent to a discussion on on how much effort adding a SortOrder would be.

alamb · 2025-07-14T16:48:57Z

@emkornfield are you opposed to merging this PR? I can't quite tell from the comments

I think general consensus is we should update the spec (I think there is a PR open for this).

I did make a PR here to clarify the spec about Int96 stats (with a recommendation, not with any actual change):

GH-502: Clarify Int96 status and add recommended ordering parquet-format#504

Beyond that there was discussion on whether we should have a new sort order or just rely on versions. I'll start a thread but #7909 is pertinent to a discussion on on how much effort adding a SortOrder would be.

In my opinion, there is no reason to hold up this PR (which improves compatibility for an "implementation defined" part of the spec) on actually changing the spec. We can proceed with actually defining a SortOrder / fixing the spec in parallel

emkornfield · 2025-07-14T16:55:26Z

Yes, I'm fine to merge this PR. It seems like a strict improvement.

emkornfield · 2025-07-14T17:00:02Z

parquet/src/file/statistics.rs

                    old_format,
                ),
                Type::INT96 => {
-                    // INT96 statistics may not be correct, because comparison is signed


I don't think we should remove this until we have a filter on known good statistics.

Suggested change

// INT96 statistics may not be correct, because comparison is signed

// INT96 statistics may not be correct, because comparison is signed

I forgot to merge this one. so I made a follow on PR:

Minor: Restore warning comment on Int96 statistics read #7975

Per comment, Micah is satisfied with this PR
#7687 (comment)

alamb · 2025-07-22T22:04:16Z

Thanks again everyone for all your help and patience

@emkornfield

# Which issue does this PR close? - Follow on to #7687 # Rationale for this change I merged #7687 without addressing one of @emkornfield 's suggestions: https://github.com/apache/arrow-rs/pull/7687/files#r2205393903 # What changes are included in this PR? Implement the suggestion (restore a comment_ # Are these changes tested? BY CI # Are there any user-facing changes? No

Rahul Sharma added 14 commits April 15, 2025 15:51

Add int96 stats test

3b371ac

fix conversions

fd51210

asserts

72687dd

printing change

46d98e8

Create int96 from time since epoch

93a780c

Add ways to set int96 from timestamps

a5b9eb7

simplify

796243b

Add correct ordering for int96

34c928d

Add tests for int96ordering

eb8a77c

rename tests

ef07163

Simplify test

6abd75f

Refactor test

6108b14

simplify test

5155556

Merge remote-tracking branch 'origin/main' into add-tests-for-int-96-…

abc743a

…stats

github-actions bot added the parquet Changes to the parquet crate label Jun 17, 2025

rahulketch changed the title ~~GH-7686 [Parquet] Fix int96 min/max stats~~ GH-7686: [Parquet] Fix int96 min/max stats Jun 17, 2025

Rahul Sharma added 2 commits June 17, 2025 14:13

Improve testcase

4b0b94d

shuffle data before writing to file

325d335

alkis reviewed Jun 17, 2025

View reviewed changes

parquet/src/data_type.rs Show resolved Hide resolved

rahulketch force-pushed the add-tests-for-int-96-stats branch 3 times, most recently from ede2b9a to 63a5fd5 Compare June 17, 2025 15:55

change int96 internal format

6036398

rahulketch force-pushed the add-tests-for-int-96-stats branch from 63a5fd5 to 6036398 Compare June 17, 2025 16:07

etseidl added enhancement Any new improvement worthy of a entry in the changelog api-change Changes to the arrow API next-major-release the PR has API changes and it waiting on the next major version labels Jun 17, 2025

Rahul Sharma added 4 commits July 3, 2025 09:25

Save instructions in the comparison

f184429

Fix test

081d20f

Add comments explaining the reasoning behind the new ordering

2a3e802

Remove stale comment

3c1d4b0

etseidl added 4 commits July 3, 2025 06:30

Merge remote-tracking branch 'origin/main' into pr/rahulketch/7687

ecf5802

clippy

84b05ce

lint

c008cad

add license statement

cd63d0a

etseidl marked this pull request as ready for review July 3, 2025 13:54

etseidl approved these changes Jul 3, 2025

View reviewed changes

alamb approved these changes Jul 3, 2025

View reviewed changes

alkis reviewed Jul 3, 2025

View reviewed changes

parquet/src/data_type.rs Outdated Show resolved Hide resolved

alkis approved these changes Jul 3, 2025

View reviewed changes

mbrobbel approved these changes Jul 14, 2025

View reviewed changes

Update parquet/src/data_type.rs

810b25c

Co-authored-by: Alkis Evlogimenos <[email protected]>

alamb requested a review from emkornfield July 14, 2025 15:29

emkornfield reviewed Jul 14, 2025

View reviewed changes

alamb requested a review from emkornfield July 22, 2025 22:01

alamb merged commit dff67c9 into apache:main Jul 22, 2025
16 checks passed

alamb mentioned this pull request Jul 22, 2025

Minor: Restore warning comment on Int96 statistics read #7975

Merged

	// INT96 statistics may not be correct, because comparison is signed
	// INT96 statistics may not be correct, because comparison is signed

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

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

Uh oh!

Conversation

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

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Not included in this PR:

Are there any user-facing changes?

Uh oh!

Uh oh!

etseidl commented Jun 17, 2025

Uh oh!

etseidl commented Jul 2, 2025

Uh oh!

rahulketch commented Jul 3, 2025

Uh oh!

etseidl commented Jul 3, 2025

Uh oh!

etseidl left a comment

Choose a reason for hiding this comment

Uh oh!

alamb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Jul 14, 2025

Uh oh!

emkornfield commented Jul 14, 2025

Uh oh!

alamb commented Jul 14, 2025

Uh oh!

emkornfield commented Jul 14, 2025

Uh oh!

emkornfield Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jul 14, 2025

Choose a reason for hiding this comment

Uh oh!

alamb Jul 22, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alamb commented Jul 22, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

rahulketch commented Jun 17, 2025 •

edited by alamb

Loading