Skip to content

Share Parquet field-ID and name resolution between remap_physical_schema and match_struct_fields #6978

Open

Description

@andygrove

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 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) and SparkPhysicalExprAdapter::rewrite (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 7d8d9b1.

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

Metadata

Metadata

Assignees

Labels

Type

No type

Projects

No projects

    Milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions