fix: propagate Parquet field-name folding failures - #5845
Open
sunchao wants to merge 2 commits into
Open
Conversation
sunchao
marked this pull request as ready for review
September 11, 2026 06:25
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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):ƛƛƛƛ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Éandrésuméis ambiguous when resolving a requestedré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
0655a9c7validates the updated branch. Local native builds remain limited by dependency availability in the managed registry, so native integration evidence comes from CI.