Skip to content

Commit e9a2955

Browse files
rymurrclaude
andcommitted
fix: resolve column-mapped reads per Delta protocol mode
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). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent eeefdd3 commit e9a2955

12 files changed

Lines changed: 162 additions & 23 deletions

File tree

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{"commitInfo":{"timestamp":1700000000000,"operation":"CREATE TABLE","operationParameters":{},"isolationLevel":"Serializable","isBlindAppend":true}}
2+
{"protocol":{"minReaderVersion":3,"minWriterVersion":7,"readerFeatures":["columnMapping"],"writerFeatures":["columnMapping"]}}
3+
{"metaData":{"id":"00000000-0000-0000-0000-0000000000ab","format":{"provider":"parquet","options":{}},"schemaString":"{\"type\":\"struct\",\"fields\":[{\"name\":\"a\",\"type\":\"long\",\"nullable\":false,\"metadata\":{\"delta.columnMapping.id\":1,\"delta.columnMapping.physicalName\":\"col-a-uuid\"}},{\"name\":\"b\",\"type\":\"string\",\"nullable\":false,\"metadata\":{\"delta.columnMapping.id\":2,\"delta.columnMapping.physicalName\":\"col-b-uuid\"}}]}","partitionColumns":[],"configuration":{"delta.columnMapping.mode":"id","delta.columnMapping.maxColumnId":"2"},"createdTime":1700000000000}}
4+
{"add":{"path":"part-00000.snappy.parquet","partitionValues":{},"size":869,"modificationTime":1700000000000,"dataChange":true,"stats":"{\"numRecords\":2,\"minValues\":{\"col-a-uuid\":1,\"col-b-uuid\":\"alice\"},\"maxValues\":{\"col-a-uuid\":2,\"col-b-uuid\":\"bob\"},\"nullCount\":{\"col-a-uuid\":0,\"col-b-uuid\":0}}"}}
Binary file not shown.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{"commitInfo":{"timestamp":1700000000000,"operation":"CREATE TABLE","operationParameters":{},"isolationLevel":"Serializable","isBlindAppend":true}}
2+
{"protocol":{"minReaderVersion":3,"minWriterVersion":7,"readerFeatures":["columnMapping"],"writerFeatures":["columnMapping"]}}
3+
{"metaData":{"id":"00000000-0000-0000-0000-0000000000ad","format":{"provider":"parquet","options":{}},"schemaString":"{\"type\":\"struct\",\"fields\":[{\"name\":\"nested_col\",\"type\":{\"type\":\"struct\",\"fields\":[{\"name\":\"x\",\"type\":\"integer\",\"nullable\":true,\"metadata\":{\"delta.columnMapping.id\":2,\"delta.columnMapping.physicalName\":\"col-x-uuid\"}},{\"name\":\"y\",\"type\":\"string\",\"nullable\":true,\"metadata\":{\"delta.columnMapping.id\":3,\"delta.columnMapping.physicalName\":\"col-y-uuid\"}}]},\"nullable\":true,\"metadata\":{\"delta.columnMapping.id\":1,\"delta.columnMapping.physicalName\":\"col-nested-uuid\"}}]}","partitionColumns":[],"configuration":{"delta.columnMapping.mode":"id","delta.columnMapping.maxColumnId":"3"},"createdTime":1700000000000}}
4+
{"add":{"path":"part-00000.snappy.parquet","partitionValues":{},"size":351,"modificationTime":1700000000000,"dataChange":true,"stats":"{\"numRecords\":2}"}}
Binary file not shown.
Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
{"commitInfo":{"timestamp":1700000000000,"operation":"CREATE TABLE","operationParameters":{},"isolationLevel":"Serializable","isBlindAppend":true}}
2+
{"protocol":{"minReaderVersion":3,"minWriterVersion":7,"readerFeatures":["columnMapping"],"writerFeatures":["columnMapping"]}}
3+
{"metaData":{"id":"00000000-0000-0000-0000-0000000000ac","format":{"provider":"parquet","options":{}},"schemaString":"{\"type\":\"struct\",\"fields\":[{\"name\":\"a\",\"type\":\"long\",\"nullable\":false,\"metadata\":{\"delta.columnMapping.id\":1,\"delta.columnMapping.physicalName\":\"col-a-uuid\"}},{\"name\":\"b\",\"type\":\"string\",\"nullable\":false,\"metadata\":{\"delta.columnMapping.id\":2,\"delta.columnMapping.physicalName\":\"col-b-uuid\"}}]}","partitionColumns":[],"configuration":{"delta.columnMapping.mode":"name","delta.columnMapping.maxColumnId":"2"},"createdTime":1700000000000}}
4+
{"add":{"path":"part-00000.snappy.parquet","partitionValues":{},"size":370,"modificationTime":1700000000000,"dataChange":true,"stats":"{\"numRecords\":2,\"minValues\":{\"col-a-uuid\":1,\"col-b-uuid\":\"alice\"},\"maxValues\":{\"col-a-uuid\":2,\"col-b-uuid\":\"bob\"},\"nullCount\":{\"col-a-uuid\":0,\"col-b-uuid\":0}}"}}
Binary file not shown.

src/delta_utils.cpp

Lines changed: 38 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -573,8 +573,9 @@ ffi::EngineSchemaVisitor SchemaVisitor::CreateSchemaVisitor(SchemaVisitor &state
573573
}
574574

575575
vector<DeltaMultiFileColumnDefinition> SchemaVisitor::VisitSnapshotSchema(ffi::Handle<ffi::SharedExternEngine> engine,
576-
ffi::SharedSnapshot *snapshot) {
577-
SchemaVisitor state(engine);
576+
ffi::SharedSnapshot *snapshot,
577+
DeltaColumnMappingMode mapping_mode) {
578+
SchemaVisitor state(engine, mapping_mode);
578579
auto visitor = CreateSchemaVisitor(state);
579580

580581
auto schema = logical_schema(snapshot);
@@ -590,8 +591,8 @@ vector<DeltaMultiFileColumnDefinition> SchemaVisitor::VisitSnapshotSchema(ffi::H
590591

591592
vector<DeltaMultiFileColumnDefinition>
592593
SchemaVisitor::VisitSnapshotGlobalReadSchema(ffi::Handle<ffi::SharedExternEngine> engine, ffi::SharedScan *scan,
593-
bool logical) {
594-
SchemaVisitor visitor_state(engine);
594+
bool logical, DeltaColumnMappingMode mapping_mode) {
595+
SchemaVisitor visitor_state(engine, mapping_mode);
595596
auto visitor = CreateSchemaVisitor(visitor_state);
596597

597598
ffi::Handle<ffi::SharedSchema> schema;
@@ -614,7 +615,10 @@ SchemaVisitor::VisitSnapshotGlobalReadSchema(ffi::Handle<ffi::SharedExternEngine
614615
vector<DeltaMultiFileColumnDefinition>
615616
SchemaVisitor::VisitWriteContextSchema(ffi::Handle<ffi::SharedExternEngine> engine,
616617
ffi::SharedWriteContext *write_context) {
617-
SchemaVisitor visitor_state(engine);
618+
// TODO(column-mapping-writes): plumb the table's column mapping mode here so writes
619+
// emit identifiers consistent with the read path. The read path is the only consumer
620+
// today, so leaving this NONE keeps writes' behavior unchanged from before this fix.
621+
SchemaVisitor visitor_state(engine, DeltaColumnMappingMode::NONE);
618622
auto visitor = CreateSchemaVisitor(visitor_state);
619623
auto schema = ffi::get_write_schema(write_context);
620624
uintptr_t result = visit_schema(schema, &visitor);
@@ -633,7 +637,7 @@ void SchemaVisitor::VisitDecimal(SchemaVisitor *state, uintptr_t sibling_list_id
633637
DeltaMultiFileColumnDefinition decimal_def(KernelUtils::FromDeltaString(name), decimal_type, is_nullable);
634638
decimal_def.default_expression = make_uniq<ConstantExpression>(Value().DefaultCastAs(decimal_type));
635639

636-
ApplyDeltaColumnMapping(state->engine, metadata, decimal_def);
640+
ApplyDeltaColumnMapping(*state, metadata, decimal_def);
637641

638642
state->AppendToList(sibling_list_id, name, std::move(decimal_def));
639643
}
@@ -656,7 +660,7 @@ void SchemaVisitor::VisitStruct(SchemaVisitor *state, uintptr_t sibling_list_id,
656660
struct_def.children = std::move(children);
657661
struct_def.default_expression = make_uniq<ConstantExpression>(Value(struct_type));
658662

659-
ApplyDeltaColumnMapping(state->engine, metadata, struct_def);
663+
ApplyDeltaColumnMapping(*state, metadata, struct_def);
660664

661665
state->AppendToList(sibling_list_id, name, std::move(struct_def));
662666
}
@@ -676,7 +680,7 @@ void SchemaVisitor::VisitArray(SchemaVisitor *state, uintptr_t sibling_list_id,
676680
// TODO: kinda wonky, but column mapper uses this
677681
list_def.children.front().name = "list";
678682

679-
ApplyDeltaColumnMapping(state->engine, metadata, list_def);
683+
ApplyDeltaColumnMapping(*state, metadata, list_def);
680684

681685
state->AppendToList(sibling_list_id, name, std::move(list_def));
682686
}
@@ -699,7 +703,7 @@ void SchemaVisitor::VisitMap(SchemaVisitor *state, uintptr_t sibling_list_id, ff
699703

700704
map_def.default_expression = make_uniq<ConstantExpression>(Value(map_type));
701705

702-
ApplyDeltaColumnMapping(state->engine, metadata, map_def);
706+
ApplyDeltaColumnMapping(*state, metadata, map_def);
703707

704708
state->AppendToList(sibling_list_id, name, std::move(map_def));
705709
}
@@ -711,7 +715,7 @@ void SchemaVisitor::VisitVariant(SchemaVisitor *state, uintptr_t sibling_list_id
711715
// global setting.
712716
LogicalType type = LogicalType::VARIANT();
713717
DeltaMultiFileColumnDefinition col_def(KernelUtils::FromDeltaString(name), type, is_nullable);
714-
ApplyDeltaColumnMapping(state->engine, metadata, col_def);
718+
ApplyDeltaColumnMapping(*state, metadata, col_def);
715719
state->AppendToList(sibling_list_id, name, std::move(col_def));
716720
}
717721

@@ -915,6 +919,30 @@ string KernelUtils::FetchFromStringMap(ffi::Handle<ffi::SharedExternEngine> engi
915919
return val;
916920
}
917921

922+
DeltaColumnMappingMode KernelUtils::ReadColumnMappingMode(ffi::SharedSnapshot *snapshot) {
923+
struct VisitorContext {
924+
string mode;
925+
};
926+
VisitorContext ctx;
927+
auto visitor = [](ffi::NullableCvoid engine_context, ffi::KernelStringSlice key, ffi::KernelStringSlice value) {
928+
auto &c = *static_cast<VisitorContext *>(engine_context);
929+
if (FromDeltaString(key) == "delta.columnMapping.mode") {
930+
c.mode = FromDeltaString(value);
931+
}
932+
};
933+
ffi::visit_metadata_configuration(snapshot, &ctx, visitor);
934+
// The Delta protocol specifies lowercase values, but normalize defensively so a
935+
// non-conformant writer's "ID"/"Name" doesn't silently degrade to NONE.
936+
auto mode = StringUtil::Lower(ctx.mode);
937+
if (mode == "id") {
938+
return DeltaColumnMappingMode::ID;
939+
}
940+
if (mode == "name") {
941+
return DeltaColumnMappingMode::NAME;
942+
}
943+
return DeltaColumnMappingMode::NONE;
944+
}
945+
918946
vector<unique_ptr<ParsedExpression>> &
919947
KernelUtils::UnpackTransformExpression(const vector<unique_ptr<ParsedExpression>> &parsed_expression) {
920948
if (parsed_expression.size() != 1) {

src/functions/delta_scan/delta_multi_file_list.cpp

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,8 @@ void DeltaMultiFileList::Bind(vector<LogicalType> &return_types, vector<string>
620620
vector<DeltaMultiFileColumnDefinition> visited_schema;
621621
{
622622
auto snapshot_ref = snapshot->GetLockingRef();
623-
visited_schema = SchemaVisitor::VisitSnapshotSchema(extern_engine.get(), snapshot_ref.GetPtr());
623+
auto mapping_mode = KernelUtils::ReadColumnMappingMode(snapshot_ref.GetPtr());
624+
visited_schema = SchemaVisitor::VisitSnapshotSchema(extern_engine.get(), snapshot_ref.GetPtr(), mapping_mode);
624625
}
625626

626627
for (const auto &field : visited_schema) {
@@ -791,7 +792,9 @@ void DeltaMultiFileList::InitializeScan() const {
791792
}
792793
}
793794

794-
lazy_loaded_schema = SchemaVisitor::VisitSnapshotGlobalReadSchema(extern_engine.get(), scan.get(), true);
795+
auto mapping_mode = KernelUtils::ReadColumnMappingMode(snapshot_ref.GetPtr());
796+
lazy_loaded_schema =
797+
SchemaVisitor::VisitSnapshotGlobalReadSchema(extern_engine.get(), scan.get(), true, mapping_mode);
795798

796799
DeltaMultiFileColumnDefinition::Print(lazy_loaded_schema, "lazy_loaded_schema");
797800

src/include/delta_utils.hpp

Lines changed: 40 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,23 @@ struct DeltaLogPathArray {
4848
vector<ffi::FfiLogPath> log_entries;
4949
};
5050

51+
// Mirrors the Delta protocol's `delta.columnMapping.mode` table property.
52+
// Determines how readers resolve logical columns to parquet columns:
53+
// ID -> resolve by parquet field_id
54+
// NAME -> resolve by physical name
55+
// NONE -> resolve by display (logical) name
56+
enum class DeltaColumnMappingMode { NONE, ID, NAME };
57+
5158
struct KernelUtils {
5259
static LogicalType GetLogPathType();
5360
static ffi::KernelStringSlice ToDeltaString(const string &str);
5461
static string FromDeltaString(const struct ffi::KernelStringSlice slice);
5562
static vector<bool> FromDeltaBoolSlice(const struct ffi::KernelBoolSlice slice);
5663
static string FetchFromStringMap(ffi::Handle<ffi::SharedExternEngine> engine, const ffi::CStringMap *map,
5764
const string &key);
65+
// Read `delta.columnMapping.mode` from the snapshot's table-property
66+
// configuration. Returns NONE when the property is absent or `"none"`.
67+
static DeltaColumnMappingMode ReadColumnMappingMode(ffi::SharedSnapshot *snapshot);
5868

5969
static void *StringAllocationNew(const struct ffi::KernelStringSlice slice) {
6070
return new string(slice.ptr, slice.len);
@@ -276,12 +286,15 @@ struct DeltaMultiFileColumnDefinition : public MultiFileColumnDefinition {
276286
// SchemaVisitor is used to parse the schema of a Delta table from the Kernel
277287
class SchemaVisitor {
278288
public:
279-
explicit SchemaVisitor(ffi::Handle<ffi::SharedExternEngine> engine_p) : engine(engine_p) {};
289+
SchemaVisitor(ffi::Handle<ffi::SharedExternEngine> engine_p, DeltaColumnMappingMode mapping_mode_p)
290+
: engine(engine_p), mapping_mode(mapping_mode_p) {};
280291

281292
static vector<DeltaMultiFileColumnDefinition> VisitSnapshotSchema(ffi::Handle<ffi::SharedExternEngine> engine,
282-
ffi::SharedSnapshot *snapshot);
293+
ffi::SharedSnapshot *snapshot,
294+
DeltaColumnMappingMode mapping_mode);
283295
static vector<DeltaMultiFileColumnDefinition>
284-
VisitSnapshotGlobalReadSchema(ffi::Handle<ffi::SharedExternEngine> engine, ffi::SharedScan *state, bool logical);
296+
VisitSnapshotGlobalReadSchema(ffi::Handle<ffi::SharedExternEngine> engine, ffi::SharedScan *state, bool logical,
297+
DeltaColumnMappingMode mapping_mode);
285298
static vector<DeltaMultiFileColumnDefinition> VisitWriteContextSchema(ffi::Handle<ffi::SharedExternEngine> engine,
286299
ffi::SharedWriteContext *write_context);
287300

@@ -290,22 +303,38 @@ class SchemaVisitor {
290303
uintptr_t next_id = 1;
291304

292305
ffi::SharedExternEngine *engine = nullptr;
306+
DeltaColumnMappingMode mapping_mode = DeltaColumnMappingMode::NONE;
293307
ErrorData error;
294308

295309
static ffi::EngineSchemaVisitor CreateSchemaVisitor(SchemaVisitor &state);
296310

297311
typedef void(SimpleTypeVisitorFunction)(void *, uintptr_t, ffi::KernelStringSlice, bool is_nullable,
298312
const ffi::CStringMap *metadata);
299313

300-
static void ApplyDeltaColumnMapping(ffi::Handle<ffi::SharedExternEngine> engine, const ffi::CStringMap *metadata,
314+
// Set `col_def.identifier` so DuckDB's MultiFileReader resolves the column
315+
// the way the Delta protocol's "Reader Requirements for Column Mapping"
316+
// require for the active mode. Identifier type drives the dispatch:
317+
// BIGINT -> match by parquet field_id, VARCHAR -> match by name. Leaving
318+
// it unset matches by display (logical) name.
319+
static void ApplyDeltaColumnMapping(SchemaVisitor &state, const ffi::CStringMap *metadata,
301320
DeltaMultiFileColumnDefinition &col_def) {
302-
auto id = KernelUtils::FetchFromStringMap(engine, metadata, "parquet.field.id");
303-
if (!id.empty()) {
304-
col_def.identifier = Value(id).DefaultCastAs(LogicalType::BIGINT);
321+
switch (state.mapping_mode) {
322+
case DeltaColumnMappingMode::ID: {
323+
auto id = KernelUtils::FetchFromStringMap(state.engine, metadata, "parquet.field.id");
324+
if (!id.empty()) {
325+
col_def.identifier = Value(id).DefaultCastAs(LogicalType::BIGINT);
326+
}
327+
break;
328+
}
329+
case DeltaColumnMappingMode::NAME: {
330+
auto name = KernelUtils::FetchFromStringMap(state.engine, metadata, "delta.columnMapping.physicalName");
331+
if (!name.empty()) {
332+
col_def.identifier = Value(name);
333+
}
334+
break;
305335
}
306-
auto name = KernelUtils::FetchFromStringMap(engine, metadata, "delta.columnMapping.physicalName");
307-
if (!name.empty()) {
308-
col_def.identifier = Value(name);
336+
case DeltaColumnMappingMode::NONE:
337+
break;
309338
}
310339
col_def.default_expression = make_uniq<ConstantExpression>(Value(col_def.type));
311340
}
@@ -318,7 +347,7 @@ class SchemaVisitor {
318347
static void VisitSimpleTypeImpl(SchemaVisitor *state, uintptr_t sibling_list_id, ffi::KernelStringSlice name,
319348
bool is_nullable, const ffi::CStringMap *metadata) {
320349
DeltaMultiFileColumnDefinition col_def(KernelUtils::FromDeltaString(name), TypeId, is_nullable);
321-
ApplyDeltaColumnMapping(state->engine, metadata, col_def);
350+
ApplyDeltaColumnMapping(*state, metadata, col_def);
322351

323352
state->AppendToList(sibling_list_id, name, std::move(col_def));
324353
}
Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
# name: test/sql/inlined/column_mapping_id_mode_logical_names.test
2+
# description: id-mode tables whose parquet files carry logical column names plus
3+
# PARQUET:field_id metadata must resolve by field_id, not by name.
4+
# group: [inlined]
5+
6+
require parquet
7+
8+
require delta
9+
10+
statement ok
11+
from copy_dir('data/inlined/column_mapping_id_mode_logical_names',
12+
'__TEST_DIR__/data/inlined/column_mapping_id_mode_logical_names');
13+
14+
# Bug repro: the parquet file's columns are named "a"/"b" (the logical names)
15+
# and carry field_ids 1/2. The Delta log declares id-mode column mapping with
16+
# physical names "col-a-uuid"/"col-b-uuid". Resolving by physical name produces
17+
# all-NULL rows. Resolving by field_id (per the protocol) returns the data.
18+
query II
19+
from delta_scan('__TEST_DIR__/data/inlined/column_mapping_id_mode_logical_names/delta_lake') order by a
20+
----
21+
1 alice
22+
2 bob

0 commit comments

Comments
 (0)