Skip to content

fix: propagate Parquet field-name folding failures - #5845

Open
sunchao wants to merge 2 commits into
apache:mainfrom
sunchao:codex/upstream-unicode-errors
Open

fix: propagate Parquet field-name folding failures#5845
sunchao wants to merge 2 commits into
apache:mainfrom
sunchao:codex/upstream-unicode-errors

Conversation

@sunchao

@sunchao sunchao commented Sep 10, 2026

Copy link
Copy Markdown
Member

Which issue does this PR close?

Follow-up to #5602 and #5495.

Rationale for this change

A case-insensitive Parquet read must resolve field names using the same Unicode rules as Spark. Comet delegates non-ASCII lowercasing to the JVM for that reason. Today, however, a failed JVM call produces a warning and the read continues using Rust's Unicode lowercasing. The JDK and Rust can use different Unicode tables, so this recovery path can change which physical column the query reads.

For example, consider a file with a field named (U+A7DC) and a requested schema containing ƛ (U+019B):

Name Lowercased by JDK 17 Lowercased by Rust
File field ƛ
Requested field ƛ ƛ ƛ

Under JDK 17 these are distinct names. If the JVM call fails, the Rust fallback makes them match. A transient failure can therefore turn into a successful read of a column that Spark's name resolver would not select. Avoiding cache insertion for the fallback does not protect the read already in progress.

What changes are included in this PR?

The reader now treats successful Spark-compatible name resolution as a prerequisite for continuing. If a required JVM fold fails, the original error propagates through schema adaptation and nested-field matching to the caller. A production read that needs the JVM also fails if no JVM has been initialized. The failure contributes no new cache entries, allowing a later successful call to retry normally. ASCII names and case-sensitive lookups retain their existing local fast paths.

This error handling also extends to default values for missing columns. A nested default must be converted to the requested schema before it can be used. Previously, a conversion error was discarded and the original value was inserted anyway. For example, a default struct containing both RÉSUMÉ and résumé is ambiguous when resolving a requested résumé field case-insensitively; the reader now reports that ambiguity. This closes a path that could otherwise swallow the newly propagated field-matching errors.

How are these changes tested?

The new regressions inject a folding failure, verify that it is returned without caching substitute names, and then verify that a successful retry is cached. They also check that ASCII and case-sensitive lookups bypass the JVM, and that an ambiguous nested default reports its conversion error.

On the original PR revision, upstream CI passed the Linux Rust tests, native builds, and Linux scan suites. Local formatting checks and five isolated folding/cache tests also passed; those helper tests do not exercise JNI.

The original CI run's only failed test was a shared decimal codegen coverage assertion. This branch now includes its upstream correction, #5849. The assertion was reproduced before the correction and passed afterward in a focused local run. Fresh CI for revision 0655a9c7 validates the updated branch. Local native builds remain limited by dependency availability in the managed registry, so native integration evidence comes from CI.

@github-actions github-actions Bot added bug Something isn't working area:scan Parquet scan / data reading labels Sep 10, 2026
@sunchao
sunchao marked this pull request as ready for review September 11, 2026 06:25

@rich7420 rich7420 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.

+1, LGTM , it's a clean one
@sunchao thanks for the patch

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scan Parquet scan / data reading bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants