andygrove opened a new issue, #6978: URL: https://github.com/apache/datafusion-comet/issues/6978
### What is the problem the feature request solves? Parquet field resolution by field ID and by case-insensitive name is implemented twice, once for top-level columns and once for nested struct fields, and the two must be kept in sync by hand: - [`remap_physical_schema` in schema_adapter.rs (L198-L343)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/parquet/schema_adapter.rs#L198-L343): cognitive complexity 67, nesting depth 5, 30 commits in the last year. - [`match_struct_fields` in parquet_support.rs (L458-L534)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/parquet/parquet_support.rs#L458-L534): the same ID/case-folding resolution for nested fields. `remap_physical_schema` mixes four concerns in one function: duplicate-ID validation (L207-228), building the id-to-logical map (L231-241), name folding, and a per-field closure with three precedence stages (ID match, fake-name shadowing, case-insensitive name match). The renamed-field construction (`Field::new(logical.name(), ...).with_metadata(..)` plus `name_map.insert`) is copy-pasted between the ID path (L262-272) and the name path (L305-315). The closure uses early returns and mutates `name_map`, `fake_counter` and `occupied_names`, so the precedence rules that mirror Spark's `matchIdField` are hard to follow or unit test. Because this file is still patched often for Spark-compatibility fixes, a fix to one path that misses the other could reintroduce a schema-evolution bug for nested columns only. The surrounding adapter code has the same shape: [`SparkPhysicalExprAdapterFactory::create` (L900-L1088)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/parquet/schema_adapter.rs#L900-L1088) and [`SparkPhysicalExprAdapter::rewrite` (L1152-L1251)](https://github.com/apache/datafusion-comet/blob/7d8d9b1544b734c85a93cca2cc9d5380ad70accd/native/core/src/parquet/schema_adapter.rs#L1152-L1251) each inline several independent steps (duplicate-column checks, id-duplicate roots, rebase policies, physical-name restore). ### Describe the potential solution 1. Extract a field resolver shared by both paths that returns an enum such as `{IdMatch, Shadowed, NameMatch, Unchanged}`, so each precedence stage can be unit tested against Spark's behaviour. 2. Extract `check_duplicate_field_ids`, `build_id_to_logical`, a `rename_field` helper used by both the ID and name branches, and a small `FakeNameGenerator` holding `occupied_names` and `fake_counter`. 3. Use the shared resolver from `match_struct_fields`. 4. Split `create` and `rewrite` into named steps (e.g. `check_duplicate_columns`, `id_duplicate_roots`, `resolve_rebase_policies`, `restore_physical_names`), keeping the Spark-reference comments with the extracted helpers. 5. Optionally move the roughly 1,900 lines of tests in schema_adapter.rs into a separate file. ### Additional context Line references are pinned to commit 7d8d9b1544b7. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
