fix: resolve column-mapped reads per Delta protocol mode - #309
Open
rymurr wants to merge 5 commits into
Open
Conversation
rymurr
force-pushed
the
fix/column-mapping-id-mode-identifier
branch
from
May 4, 2026 20:40
e9a2955 to
943c3ea
Compare
Author
|
hey @samansmink and @benfleis mind taking a look at this? Pretty simple fix to get read by id working when the underlying parquet are malformed |
Author
|
ping @samansmink and @benfleis . Just looking for a maintainer review. This is a drift between the duckdb interpretation of the delta spec and the spec |
rymurr
force-pushed
the
fix/column-mapping-id-mode-identifier
branch
from
August 11, 2026 19:32
6c16895 to
d643f4d
Compare
Member
|
@rymurr thanks for your patience, I'm working through a couple PR and bug backlogs. I'll have a real look at the code/tests shortly, but will keep the CI flowing for your pushes in the meanwhile. |
benfleis
self-requested a review
August 12, 2026 06:35
Author
Thanks @benfleis ! I think everythign is passing now. CI is green. I've been using this in our internal fork for a few months but am not super familiar with house style and preferences so happy to take your feedback if you want some changes |
ApplyDeltaColumnMapping previously set DeltaMultiFileColumnDefinition::identifier unconditionally to the physical name when the kernel reported one, overwriting any field_id it had just assigned. The kernel emits both `parquet.field.id` and `delta.columnMapping.physicalName` whenever column mapping is enabled, so the final identifier was always a string and DuckDB's MultiFileReader always matched columns by name. That works for spec-conformant writers (pyspark) which name parquet columns with their physical names, but produces all-null rows in id mode against parquet files whose columns retain logical names with field_ids set -- e.g., the typical "rename without rewrite" flow. Per the Delta protocol "Reader Requirements for Column Mapping": - id mode: resolve columns by parquet field_id - name mode: resolve columns by physical name - none: resolve by display name Read `delta.columnMapping.mode` from the snapshot's metadata configuration via the existing kernel FFI `visit_metadata_configuration` and thread the resolved mode through SchemaVisitor down to ApplyDeltaColumnMapping, which now sets identifier per spec for the active mode (BIGINT field_id for ID, VARCHAR physical name for NAME, leaves it unset for NONE so DuckDB matches by display name). The mode value is lowercased before comparison so a non-conformant writer's "ID"/"Name" doesn't silently degrade to NONE. The write-path entry point (VisitWriteContextSchema) is left passing NONE with a TODO; identifier isn't consumed for column matching during writes, so this is behavior-preserving. Adds three inlined SQLLogicTest fixtures, independent of GENERATED_DATA_AVAILABLE: - column_mapping_id_mode_logical_names: the original bug repro -- id-mode log with physical names, parquet file with logical names + field_ids. Fails on main, passes with this fix. - column_mapping_name_mode: name-mode log paired with a parquet file whose columns are named with the physical names; locks in that VARCHAR identifier dispatch still resolves correctly. - column_mapping_id_mode_nested_struct: exercises the VisitStruct recursion path with field_ids on nested fields, which the top-level fixture does not cover. Verified to fail without the fix (returns nested NULLs).
Follow-up on the id-mode fix: make it actually take effect, and repair the regression it caused in test/sql/generated/column_mapping_id_mode.test. Setting DeltaMultiFileColumnDefinition::identifier to a field_id was not enough. DuckDB's MultiFileReader picks a mapper from the reader bind data, and duckdb-delta never set anything other than the default BY_NAME. Under BY_NAME the mapper calls GetIdentifierName(), so a numeric identifier was stringified and matched against parquet *column names* -- "1" against "col-<uuid>" -- which matched nothing and produced all-NULL rows against pyspark-written id-mode tables. The identifier was in fact never set at all: the kernel only attaches `parquet.field.id` when it materializes a physical schema, and we visit the logical schema, where the field id is `delta.columnMapping.id`. So the ID branch was dead code and matching silently fell back to the logical name. That is why column_mapping_id_mode_logical_names passed -- its parquet columns happen to carry the logical names -- rather than because field_id resolution worked. This commit: - reads the field id from `delta.columnMapping.id`, falling back from `parquet.field.id` so a physical schema would still work; - uses INTEGER, not BIGINT, since GetIdentifierFieldId() requires it and asserts on it in debug builds; - threads the mapping mode to DeltaMultiFileReader::InitializeReader so it selects BY_FIELD_ID for id-mode tables. Field_id matching is all-or-nothing: FieldIdMapper requires an identifier on every column it visits, and Delta assigns column mapping ids only to struct fields, never to the synthetic list/map children the schema visitor creates. An id-mode schema that is not fully covered therefore falls back wholesale to physical-name matching, which is the pre-existing behavior and correct for writers that name parquet columns with the physical names. Without this the kernel's own table-with-columnmapping-mode-id golden table asserts. Adds column_mapping_id_mode_physical_names, covering the spec-conformant pyspark layout (physical parquet names + field_ids) that CI caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- ApplyDeltaColumnMapping is static, so read the engine via state.engine - GetStringWidthBounds: pass mapping_mode to the updated ToColumnDefinitions signature
The kernel does not attach a columnMapping.id to the synthetic key/value/element
fields of maps and lists; those ids live on the parent field as
delta.columnMapping.nested.ids (e.g. {"col_4.key":11,"col_4.value":12}). Lift them
onto the children so HasFieldIdsRecursive() succeeds and ResolveByFieldId uses
BY_FIELD_ID instead of falling the entire schema back to name matching -- which
otherwise nulls every column of any id-mode table containing a map or list.
rymurr
force-pushed
the
fix/column-mapping-id-mode-identifier
branch
from
August 24, 2026 12:17
aae6398 to
a0ba605
Compare
Member
|
Hey @rymurr thanks for your patience! I have a partial review ready to go, but am swamped at the moment. The good news is that your PR addresses a couple issues in my own list, so I am eager to get to it soon! |
duckdb/duckdb#4dbe30e6c1 ("make FunctionSet immutable and with shared
pointers", merged to duckdb main 2026-08-24) changed
FunctionSet<T>::functions from vector<T> to vector<shared_ptr<const T>>,
and GetFunctionByArguments/GetFunctionByOffset now return
shared_ptr<const T> accordingly. Overloads must be mutated via the new
ApplyToFunctions() helper instead of iterating in place, and any code
that needs a mutable copy of a looked-up function must dereference the
shared_ptr.
This extension's CI (MainDistributionPipeline) builds against duckdb's
live main branch, so this broke as soon as that upstream commit landed,
independent of anything in this PR's own diff. Also fixes a clang-format
line-length violation in delta_utils.hpp caught by the same CI run.
Author
Thanks @benfleis ! I just pushed a new commit to fix the build issue. Will jump on the code review as soon as you send it! |
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
3 participants
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.
Summary
ApplyDeltaColumnMappingpreviously setDeltaMultiFileColumnDefinition::identifierunconditionally to the physical name when the kernel reported one, overwriting any field_id it had just assigned. The kernel emits bothparquet.field.idanddelta.columnMapping.physicalNamewhenever column mapping is enabled, so the final identifier was always a string and DuckDB's MultiFileReader always matched columns by name. That works for spec-conformant writers (pyspark) which name parquet columns with their physical names, but produces all-NULL rows in id mode against parquet files whose columns retain logical names with field_ids set — e.g. the typical "rename without rewrite" flow.Per the Delta protocol "Reader Requirements for Column Mapping":
field_idThe fix reads
delta.columnMapping.modefrom the snapshot's metadata configuration via the existing kernel FFIvisit_metadata_configurationand threads the resolved mode throughSchemaVisitordown toApplyDeltaColumnMapping, which now sets the identifier per spec for the active mode (BIGINT field_id for ID, VARCHAR physical name for NAME, leaves it unset for NONE so DuckDB matches by display name). The mode value is lowercased before comparison so a non-conformant writer's"ID"/"Name"doesn't silently degrade to NONE.The write-path entry point (
VisitWriteContextSchema) is left passing NONE with a TODO; identifier isn't consumed for column matching during writes, so this is behavior-preserving.Test plan
Three inlined SQLLogicTest fixtures, independent of
GENERATED_DATA_AVAILABLE:column_mapping_id_mode_logical_names— original bug repro: id-mode log with physical names, parquet file with logical names + field_ids. Fails on main, passes with this fix.column_mapping_name_mode— name-mode log paired with a parquet file whose columns are named with the physical names; locks in that VARCHAR identifier dispatch still resolves correctly.column_mapping_id_mode_nested_struct— exercises theVisitStructrecursion path with field_ids on nested fields. Verified to fail without the fix (returns nested NULLs) and pass with it.build/debug/test/unittest '[inlined]').🤖 Generated with Claude Code