Skip to content

Commit 1ed2141

Browse files
res-lifeclaude
andauthored
[BUG FIX] Iceberg: fix Missing required field for newly-added nested MAP/LIST (#14880)
Fixes #14879. ### Description When an Iceberg table evolves via `ALTER TABLE ... ADD COLUMN` with a nested optional `MAP` or `LIST`, and a pre-evolution data file is read on GPU, `GpuParquetReaderPostProcessor` used to throw `IllegalArgumentException: Missing required field` before the missing container's own `map()`/`list()` handler could emit `FillNull`. The CPU reader handles this case correctly; the GPU read fails. **User experience after this change:** queries against Iceberg tables that have schema-evolved to add a nested `MAP`/`LIST` now succeed end-to-end on GPU; pre-evolution rows materialize the new column as `NULL`, matching CPU behavior. No new configs or user-facing knobs. **Technical fix:** 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 at `GpuParquetReaderPostProcessor.scala:385` before the container's own handler ran. The fix extends `ActionBuildingVisitor`'s field stack to track each field's partner type and adds a strict-ancestor `isInsideMissingContainer` flag (excludes the current field itself, so the missing container's own `struct`/`list`/`map` handler still runs the normal `partner == null` logic). The new flag is treated 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). **Tests added** (`GpuPostProcessorSuite`): - `Missing nested optional map with required key does not throw` — the reproducer; verifies both action-plan construction and `process()` execution - `Missing nested optional list with required element does not throw` - `Doubly nested missing container with required descendant does not throw` — exercises the strict-ancestor logic across multiple levels All 15 `GpuPostProcessorSuite` tests pass under buildver=356. ### Checklists Documentation - [ ] Updated for new or modified user-facing features or behaviors - [x] No user-facing change Testing - [x] Added or modified tests to cover new code paths - [ ] Covered by existing tests - [ ] Not required Performance - [ ] Tests ran and results are added in the PR description - [ ] Issue filed with a link in the PR description - [x] Not required --------- Signed-off-by: Chong Gao <res_life@163.com> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
1 parent ba38a05 commit 1ed2141

2 files changed

Lines changed: 179 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: 153 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -935,4 +935,157 @@ 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 scoreFieldId = 2
947+
val propsMapId = 3 // newly-added MAP, missing from file
948+
val propsKeyId = 4
949+
val propsValueId = 5
950+
951+
// Parquet file: info STRUCT<score: BIGINT> — no props
952+
val scoreType =
953+
ShadedTypes.primitive(ShadedPrimitiveTypeName.INT64, ShadedRepetition.OPTIONAL)
954+
.id(scoreFieldId).named("score")
955+
val infoStruct = ShadedTypes.optionalGroup().addField(scoreType)
956+
.id(infoStructId).named("info")
957+
val parquetSchema = new ShadedMessageType("test",
958+
Seq[ShadedType](infoStruct).asJava)
959+
960+
// Expected: info STRUCT<score, props: MAP<STRING, BIGINT>>
961+
val expectedSchema = new Schema(
962+
Types.NestedField.optional(infoStructId, "info",
963+
Types.StructType.of(
964+
Types.NestedField.optional(scoreFieldId, "score", Types.LongType.get()),
965+
Types.NestedField.optional(propsMapId, "props",
966+
Types.MapType.ofOptional(propsKeyId, propsValueId,
967+
Types.StringType.get(), Types.LongType.get()))
968+
))
969+
)
970+
971+
val rowCount = 3
972+
val (parquetInfo, shadedSchema) = createParquetInfo(parquetSchema, rowCount.toLong)
973+
val processor = new GpuParquetReaderPostProcessor(
974+
parquetInfo,
975+
new JHashMap[Integer, Any](),
976+
expectedSchema,
977+
shadedSchema,
978+
Map.empty)
979+
980+
val plan = processor.displayActionPlan()
981+
// The missing map itself must emit FillNull (it's the only valid behavior
982+
// for an optional missing field). Children are discarded by the parent.
983+
assert(plan.contains("FillNull(map<"),
984+
s"expected FillNull for missing map in plan:\n$plan")
985+
986+
// Exercise process() too: action-tree construction succeeding is not the
987+
// same guarantee as execute() succeeding for FillNull(map<...>).
988+
import com.nvidia.spark.rapids.{FuzzerUtils, GpuColumnVector, SpillableColumnarBatch}
989+
import com.nvidia.spark.rapids.SpillPriorities
990+
import com.nvidia.spark.rapids.Arm.{closeOnExcept, withResource}
991+
import org.apache.spark.sql.types.{LongType, StructField, StructType => SparkStructType}
992+
993+
val inputSparkSchema = SparkStructType(Array(StructField(
994+
"info",
995+
SparkStructType(Seq(StructField("score", LongType, true))),
996+
true)))
997+
val inputBatch = FuzzerUtils.createColumnarBatch(inputSparkSchema, rowCount, seed = 42)
998+
val spillable = closeOnExcept(inputBatch) { batch =>
999+
SpillableColumnarBatch(batch, SpillPriorities.ACTIVE_ON_DECK_PRIORITY)
1000+
}
1001+
withResource(spillable) { _ =>
1002+
withResource(processor.process(spillable.getColumnarBatch())) { outputBatch =>
1003+
assert(outputBatch.numRows() == rowCount)
1004+
assert(outputBatch.numCols() == 1)
1005+
val infoCol = outputBatch.column(0).asInstanceOf[GpuColumnVector].getBase
1006+
// info struct gains props as a second child.
1007+
assert(infoCol.getNumChildren == 2,
1008+
s"expected info struct to have 2 children, got ${infoCol.getNumChildren}")
1009+
withResource(infoCol.getChildColumnView(1)) { propsCol =>
1010+
assert(propsCol.getNullCount == rowCount,
1011+
s"props should be all-null; nullCount=${propsCol.getNullCount} rowCount=$rowCount")
1012+
}
1013+
}
1014+
}
1015+
}
1016+
1017+
// Same defect, with LIST<required element> as the missing container.
1018+
test("Missing nested optional list with required element does not throw") {
1019+
val outerStructId = 1
1020+
val keepFieldId = 2
1021+
val newListId = 3 // newly-added list, missing from file
1022+
val newListElementId = 4
1023+
1024+
val keepType =
1025+
ShadedTypes.primitive(ShadedPrimitiveTypeName.INT64, ShadedRepetition.OPTIONAL)
1026+
.id(keepFieldId).named("keep")
1027+
val outerStruct = ShadedTypes.optionalGroup().addField(keepType)
1028+
.id(outerStructId).named("outer")
1029+
val parquetSchema = new ShadedMessageType("test",
1030+
Seq[ShadedType](outerStruct).asJava)
1031+
1032+
val expectedSchema = new Schema(
1033+
Types.NestedField.optional(outerStructId, "outer",
1034+
Types.StructType.of(
1035+
Types.NestedField.optional(keepFieldId, "keep", Types.LongType.get()),
1036+
// List with a REQUIRED element — the bug also fires here because the
1037+
// required element primitive is visited before the parent list().
1038+
Types.NestedField.optional(newListId, "new_list",
1039+
Types.ListType.ofRequired(newListElementId, Types.LongType.get()))
1040+
))
1041+
)
1042+
1043+
val (parquetInfo, shadedSchema) = createParquetInfo(parquetSchema)
1044+
val processor = new GpuParquetReaderPostProcessor(
1045+
parquetInfo,
1046+
new JHashMap[Integer, Any](),
1047+
expectedSchema,
1048+
shadedSchema,
1049+
Map.empty)
1050+
1051+
val plan = processor.displayActionPlan()
1052+
assert(plan.contains("FillNull(array<"),
1053+
s"expected FillNull for missing list in plan:\n$plan")
1054+
}
1055+
1056+
// Doubly-nested: a missing struct that itself contains a missing map with
1057+
// a required key. Exercises strict-ancestor isInsideMissingContainer when
1058+
// multiple levels above also have partner == null.
1059+
test("Doubly nested missing container with required descendant does not throw") {
1060+
val metaStructId = 1 // missing struct
1061+
val propsMapId = 2 // missing map (inside metaStruct)
1062+
val propsKeyId = 3
1063+
val propsValueId = 4
1064+
1065+
val parquetSchema = new ShadedMessageType("test",
1066+
Seq.empty[ShadedType].asJava)
1067+
val expectedSchema = new Schema(
1068+
Types.NestedField.optional(metaStructId, "meta",
1069+
Types.StructType.of(
1070+
Types.NestedField.optional(propsMapId, "props",
1071+
Types.MapType.ofOptional(propsKeyId, propsValueId,
1072+
Types.StringType.get(), Types.LongType.get()))
1073+
))
1074+
)
1075+
1076+
val (parquetInfo, shadedSchema) = createParquetInfo(parquetSchema)
1077+
val processor = new GpuParquetReaderPostProcessor(
1078+
parquetInfo,
1079+
new JHashMap[Integer, Any](),
1080+
expectedSchema,
1081+
shadedSchema,
1082+
Map.empty)
1083+
1084+
val plan = processor.displayActionPlan()
1085+
// Outer struct is missing; its child map is also missing — only the
1086+
// outer FillNull(struct<...>) is composed into the plan.
1087+
assert(plan.contains("FillNull(struct<"),
1088+
s"expected FillNull for outer missing struct in plan:\n$plan")
1089+
}
1090+
9381091
}

0 commit comments

Comments
 (0)