FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

GH-3710: Tolerate unrecognized logical/physical type combinations when reading by divjotarora · Pull Request #3711 · apache/parquet-java · GitHub

/ parquet-java Public

GH-3710: Tolerate unrecognized logical/physical type combinations when reading - #3711

Open
divjotarora wants to merge 3 commits into
apache:masterfrom
divjotarora:log-phys-type-combo
ConversationCommits (3)ChecksFiles changed
Open

GH-3710: Tolerate unrecognized logical/physical type combinations when reading#3711
divjotarora wants to merge 3 commits into
apache:masterfrom
divjotarora:log-phys-type-combo

Conversation

divjotarora commented Aug 6, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Rationale for this change

See apache/parquet-format#607 for rationale.

What changes are included in this PR?

This PR modifies parquet-java to gracefully handle unrecognized logical/physical type combinations by dropping the logical type during the read and dropping any associated statistics for the relevant columns. Note that unrecognized logical types are already handled gracefully and no changes were required.

Are these changes tested?

Yes, several unit tests added.

Are there any user-facing changes?

No.

Closes #3710

divjotarora force-pushed the log-phys-type-combo branch from 57ea343 to 8469645 Compare August 6, 2026 15:13
divjotarora marked this pull request as ready for review August 6, 2026 15:13
divjotarora mentioned this pull request Aug 6, 2026
Open
emkornfield requested changes Aug 13, 2026
View reviewed changes

emkornfield left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I think most important question is how we make this transition and an end-to-end test.

divjotarora commented Aug 18, 2026
View reviewed changes

divjotarora left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@emkornfield I added an e2e test that reads a golden file with an INT32 column annotated with UUID. We can add this file to parquet-testing as part of this work as well.

Comment thread parquet-column/src/main/java/org/apache/parquet/schema/Types.java
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/Types.java Outdated
Comment thread ...t-hadoop/src/test/java/org/apache/parquet/format/converter/TestParquetMetadataConverter.java
emkornfield reviewed Aug 20, 2026
View reviewed changes
Comment thread parquet-hadoop/src/test/resources/invalid_type_combination.parquet

emkornfield Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

is this just a place holder for the until the parquet testing file is merged?

divjotarora Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes, I'll update the PR once that one is merged

emkornfield reviewed Aug 20, 2026
View reviewed changes
Comment thread parquet-column/src/main/java/org/apache/parquet/schema/Types.java Outdated
protected PrimitiveType build(String name) {
try {
return validateAndBuild(name);
} catch (IllegalStateException e) {

emkornfield Aug 20, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

it is possible to make this more specific? Also there is a general spec question on whether we should error for clearly invalid types (e.g. decimal with a precision that is too high on ints). This is probably also a spec level question.

divjotarora Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Good point, I agree this should be scoped to only throwing on invalid combinations. Right now everything throws IllegalStateException so we can't distinguish at this level. I added a new UnsupportedLogicalTypeAnnotation exception to distinguish.

emkornfield reviewed Aug 20, 2026
View reviewed changes

emkornfield left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

main concern is on the breadth of the exception cast.

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

emkornfield Awaiting requested review from emkornfield

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Specify handling for unrecognized logical/physical type combinations

2 participants

Back | FazBrowse Home | New Git URL