Skip to content

Commit bf959c3

Browse files
Chong Gaoclaude
andcommitted
[BUG FIX] Iceberg: fix Missing required field for newly-added nested MAP/LIST
When an Iceberg table evolves via ALTER TABLE ... ADD COLUMN with a nested optional MAP/LIST and a pre-evolution data file is read, the GPU post- processor used to throw `IllegalArgumentException: Missing required field` before the missing container's own handler could emit FillNull. Iceberg's SchemaWithPartnerVisitor walks post-order, so primitive children of a missing container are visited first; a required descendant (map key is intrinsically required, list elements may be required) was dispatched to MissingFieldActionBuilder.buildAction with isOptional=false and threw. Track each field's partner type on the ActionBuildingVisitor's field stack and expose a strict-ancestor `isInsideMissingContainer` flag (excludes the current field itself, so the missing container's own struct/list/map handler still runs the regular partner==null logic). Treat the new flag exactly like the existing `isInsideConstantStruct` short-circuit in the four partner==null branches: descendants of a missing container fall through to FillNull (which the parent's own FillNull discards anyway). Adds three regression tests covering: - nested optional MAP<STRING, BIGINT> missing from file (the reproducer) - nested optional LIST<required element> missing from file - doubly-nested missing struct containing a missing map (exercises the strict-ancestor logic across multiple levels) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> Signed-off-by: Chong Gao <res_life@163.com>
1 parent a9f7a35 commit bf959c3

2 files changed

Lines changed: 153 additions & 10 deletions

File tree

iceberg/common/src/main/scala/com/nvidia/spark/rapids/iceberg/parquet/GpuParquetReaderPostProcessor.scala

Lines changed: 26 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -396,12 +396,23 @@ private class ActionBuildingVisitor(
396396
idToConstant: JMap[Integer, _]
397397
) extends SchemaWithPartnerVisitor[Type, ColumnAction] {
398398

399-
// Track the current field and whether we are inside a constant struct.
400-
private val fieldStack = Stack.empty[(Types.NestedField, Boolean)]
399+
// Track the current field, its partner type in the file schema (null when the
400+
// field is missing from the file), and whether we are inside a constant struct.
401+
// The partner is stored so that strict-ancestor "inside a missing container"
402+
// can be detected by descendants — see isInsideMissingContainer below.
403+
private val fieldStack = Stack.empty[(Types.NestedField, Type, Boolean)]
401404
private def currentField: Types.NestedField =
402405
fieldStack.headOption.map(_._1).orNull
403406
private def isInsideConstantStruct: Boolean =
404-
fieldStack.headOption.exists(_._2)
407+
fieldStack.headOption.exists(_._3)
408+
// True iff a STRICT ancestor of the current field is missing from the file
409+
// schema (partner == null at an ancestor level). The current field's own
410+
// missing-ness is reflected by the visitor method's `partner` parameter and
411+
// is intentionally excluded here, so the missing container's own handler
412+
// still runs the normal partner==null logic (constant lookup, fill-null for
413+
// optional, or throw for required).
414+
private def isInsideMissingContainer: Boolean =
415+
fieldStack.size > 1 && fieldStack.tail.exists(_._2 == null)
405416

406417
override def schema(
407418
schema: Schema,
@@ -424,7 +435,11 @@ private class ActionBuildingVisitor(
424435
}
425436

426437
if (partner == null) {
427-
if (isInsideConstantStruct) {
438+
// Inside a missing ancestor: the parent's own handler will emit
439+
// FillNull for the whole container and discard this result, so any
440+
// safe placeholder works — FillNull mirrors the isInsideConstantStruct
441+
// path and never requires an input column at execute time.
442+
if (isInsideConstantStruct || isInsideMissingContainer) {
428443
return FillNull(sparkType)
429444
}
430445
return MissingFieldActionBuilder.buildAction(
@@ -463,7 +478,7 @@ private class ActionBuildingVisitor(
463478
}
464479

465480
override def beforeField(field: Types.NestedField, partner: Type): Unit = {
466-
fieldStack.push((field,
481+
fieldStack.push((field, partner,
467482
isInsideConstantStruct ||
468483
(field.`type`().isStructType &&
469484
idToConstant.containsKey(field.fieldId()))))
@@ -484,7 +499,7 @@ private class ActionBuildingVisitor(
484499
elementResult: ColumnAction): ColumnAction = {
485500
if (partner == null) {
486501
val sparkType = SparkSchemaUtil.convert(list)
487-
if (isInsideConstantStruct) {
502+
if (isInsideConstantStruct || isInsideMissingContainer) {
488503
return FillNull(sparkType)
489504
}
490505
return MissingFieldActionBuilder.buildAction(
@@ -493,7 +508,7 @@ private class ActionBuildingVisitor(
493508
currentField.isOptional,
494509
idToConstant)
495510
}
496-
511+
497512
if (elementResult == PassThrough) {
498513
PassThrough
499514
} else {
@@ -508,7 +523,7 @@ private class ActionBuildingVisitor(
508523
valueResult: ColumnAction): ColumnAction = {
509524
if (partner == null) {
510525
val sparkType = SparkSchemaUtil.convert(map)
511-
if (isInsideConstantStruct) {
526+
if (isInsideConstantStruct || isInsideMissingContainer) {
512527
return FillNull(sparkType)
513528
}
514529
return MissingFieldActionBuilder.buildAction(
@@ -517,7 +532,7 @@ private class ActionBuildingVisitor(
517532
currentField.isOptional,
518533
idToConstant)
519534
}
520-
535+
521536
if (keyResult == PassThrough && valueResult == PassThrough) {
522537
PassThrough
523538
} else {
@@ -538,7 +553,8 @@ private class ActionBuildingVisitor(
538553
} else {
539554
UpCast(fileType, expectedType)
540555
}
541-
} else if (isInsideConstantStruct) {
556+
} else if (isInsideConstantStruct || isInsideMissingContainer) {
557+
// Children of a missing container — see struct() for rationale.
542558
FillNull(expectedType)
543559
} else {
544560
MissingFieldActionBuilder.buildAction(

tests/src/test/spark350/scala/com/nvidia/spark/rapids/iceberg/GpuPostProcessorSuite.scala

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -935,4 +935,131 @@ class GpuPostProcessorSuite extends AnyFunSuite with BeforeAndAfterAll {
935935
s"FetchConstant(fieldId=$structFieldId, struct<"))
936936
}
937937

938+
// Regression: ALTER TABLE ADD COLUMN of an optional nested MAP must not
939+
// throw when reading old data files that pre-date the ADD. Iceberg map
940+
// keys are intrinsically required, so the post-order visitor used to call
941+
// MissingFieldActionBuilder for the key with isOptional=false and throw
942+
// "Missing required field" before the missing-container's own map() handler
943+
// could emit FillNull.
944+
test("Missing nested optional map with required key does not throw") {
945+
val infoStructId = 1
946+
val nameFieldId = 2
947+
val scoreFieldId = 3
948+
val propsMapId = 4 // newly-added MAP, missing from file
949+
val propsKeyId = 5
950+
val propsValueId = 6
951+
952+
// Parquet file: info STRUCT<name: STRING, score: BIGINT> — no props
953+
val nameType =
954+
ShadedTypes.primitive(ShadedPrimitiveTypeName.BINARY, ShadedRepetition.OPTIONAL)
955+
.id(nameFieldId).named("name")
956+
val scoreType =
957+
ShadedTypes.primitive(ShadedPrimitiveTypeName.INT64, ShadedRepetition.OPTIONAL)
958+
.id(scoreFieldId).named("score")
959+
val infoStruct = ShadedTypes.optionalGroup().addField(nameType).addField(scoreType)
960+
.id(infoStructId).named("info")
961+
val parquetSchema = new ShadedMessageType("test",
962+
Seq[ShadedType](infoStruct).asJava)
963+
964+
// Expected: info STRUCT<name, score, props: MAP<STRING, BIGINT>>
965+
val expectedSchema = new Schema(
966+
Types.NestedField.optional(infoStructId, "info",
967+
Types.StructType.of(
968+
Types.NestedField.optional(nameFieldId, "name", Types.StringType.get()),
969+
Types.NestedField.optional(scoreFieldId, "score", Types.LongType.get()),
970+
Types.NestedField.optional(propsMapId, "props",
971+
Types.MapType.ofOptional(propsKeyId, propsValueId,
972+
Types.StringType.get(), Types.LongType.get()))
973+
))
974+
)
975+
976+
val (parquetInfo, shadedSchema) = createParquetInfo(parquetSchema)
977+
val processor = new GpuParquetReaderPostProcessor(
978+
parquetInfo,
979+
new JHashMap[Integer, Any](),
980+
expectedSchema,
981+
shadedSchema,
982+
Map.empty)
983+
984+
val plan = processor.displayActionPlan()
985+
// The missing map itself must emit FillNull (it's the only valid behavior
986+
// for an optional missing field). Children are discarded by the parent.
987+
assert(plan.contains("FillNull(map<"),
988+
s"expected FillNull for missing map in plan:\n$plan")
989+
}
990+
991+
// Same defect, with LIST<required element> as the missing container.
992+
test("Missing nested optional list with required element does not throw") {
993+
val outerStructId = 1
994+
val keepFieldId = 2
995+
val newListId = 3 // newly-added list, missing from file
996+
val newListElementId = 4
997+
998+
val keepType =
999+
ShadedTypes.primitive(ShadedPrimitiveTypeName.INT64, ShadedRepetition.OPTIONAL)
1000+
.id(keepFieldId).named("keep")
1001+
val outerStruct = ShadedTypes.optionalGroup().addField(keepType)
1002+
.id(outerStructId).named("outer")
1003+
val parquetSchema = new ShadedMessageType("test",
1004+
Seq[ShadedType](outerStruct).asJava)
1005+
1006+
val expectedSchema = new Schema(
1007+
Types.NestedField.optional(outerStructId, "outer",
1008+
Types.StructType.of(
1009+
Types.NestedField.optional(keepFieldId, "keep", Types.LongType.get()),
1010+
// List with a REQUIRED element — the bug also fires here because the
1011+
// required element primitive is visited before the parent list().
1012+
Types.NestedField.optional(newListId, "new_list",
1013+
Types.ListType.ofRequired(newListElementId, Types.LongType.get()))
1014+
))
1015+
)
1016+
1017+
val (parquetInfo, shadedSchema) = createParquetInfo(parquetSchema)
1018+
val processor = new GpuParquetReaderPostProcessor(
1019+
parquetInfo,
1020+
new JHashMap[Integer, Any](),
1021+
expectedSchema,
1022+
shadedSchema,
1023+
Map.empty)
1024+
1025+
val plan = processor.displayActionPlan()
1026+
assert(plan.contains("FillNull(array<"),
1027+
s"expected FillNull for missing list in plan:\n$plan")
1028+
}
1029+
1030+
// Doubly-nested: a missing struct that itself contains a missing map with
1031+
// a required key. Exercises strict-ancestor isInsideMissingContainer when
1032+
// multiple levels above also have partner == null.
1033+
test("Doubly nested missing container with required descendant does not throw") {
1034+
val metaStructId = 1 // missing struct
1035+
val propsMapId = 2 // missing map (inside metaStruct)
1036+
val propsKeyId = 3
1037+
val propsValueId = 4
1038+
1039+
val parquetSchema = new ShadedMessageType("test",
1040+
Seq.empty[ShadedType].asJava)
1041+
val expectedSchema = new Schema(
1042+
Types.NestedField.optional(metaStructId, "meta",
1043+
Types.StructType.of(
1044+
Types.NestedField.optional(propsMapId, "props",
1045+
Types.MapType.ofOptional(propsKeyId, propsValueId,
1046+
Types.StringType.get(), Types.LongType.get()))
1047+
))
1048+
)
1049+
1050+
val (parquetInfo, shadedSchema) = createParquetInfo(parquetSchema)
1051+
val processor = new GpuParquetReaderPostProcessor(
1052+
parquetInfo,
1053+
new JHashMap[Integer, Any](),
1054+
expectedSchema,
1055+
shadedSchema,
1056+
Map.empty)
1057+
1058+
val plan = processor.displayActionPlan()
1059+
// Outer struct is missing; its child map is also missing — only the
1060+
// outer FillNull(struct<...>) is composed into the plan.
1061+
assert(plan.contains("FillNull(struct<"),
1062+
s"expected FillNull for outer missing struct in plan:\n$plan")
1063+
}
1064+
9381065
}

0 commit comments

Comments
 (0)