fix: propagate Parquet field-name folding failures - #5845
Conversation
andygrove
left a comment
There was a problem hiding this comment.
Failing rather than silently substituting Rust's Unicode table is the right call here. Spark folds with toLowerCase(Locale.ROOT) and cannot fail, so there is no Spark analogue for the old fallback, and the example is real since U+A7DC only arrived in Unicode 16. I checked that an ordinary field name cannot make the JNI call fail, since that would turn a working query into a hard error, and it cannot. jni encodes through simd_cesu8::mutf8 so supplementary-plane characters round-trip, and empty or all-ASCII names never reach the JVM.
The branch does conflict with main now. Since your branch point, #5681 extracted match_struct_fields in parquet_support.rs and moved the fold_names call into it, and that function is now shared with the plan-time check_conversion in schema_adapter.rs, so that call site needs to propagate too. #5786 adds two more call sites in eager_page_index_reader_factory.rs, one inside the .then(|| ...) closure in with_required_schema and one inside the is_some_and closure in validate_field_names, and neither closure has anywhere to put an error as written. Worth flagging there so whoever lands second threads the Result through rather than reaching for an unwrap.
0655a9c to
77e7007
Compare
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.