szehon-ho commented on code in PR #16977:
URL: https://github.com/apache/iceberg/pull/16977#discussion_r3920236895
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/SparkTableUtil.java:
##########
@@ -664,6 +680,35 @@ public static void importSparkPartitions(
}
}
+ private static void verifyRequiredFields(
+ Schema schema, Set<Integer> requiredFieldIds, DataFile dataFile) {
+ if (requiredFieldIds.isEmpty()) {
+ return;
+ }
+
+ Map<Integer, Long> nullValueCounts = dataFile.nullValueCounts();
+ if (nullValueCounts == null) {
+ return;
+ }
+
+ for (int fieldId : requiredFieldIds) {
+ Long nullCount = nullValueCounts.getOrDefault(fieldId, 0L);
Review Comment:
Please avoid treating an absent null-count entry as proof of zero nulls. The
entry may be absent because the physical file omits the target field. If that
field is required without an initial default, reads later fail with `Missing
required field`; omission is valid when an initial default exists. Please
distinguish these cases using the physical file schema.
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/SparkTableUtil.java:
##########
@@ -664,6 +680,35 @@ public static void importSparkPartitions(
}
}
+ private static void verifyRequiredFields(
+ Schema schema, Set<Integer> requiredFieldIds, DataFile dataFile) {
+ if (requiredFieldIds.isEmpty()) {
+ return;
+ }
+
+ Map<Integer, Long> nullValueCounts = dataFile.nullValueCounts();
Review Comment:
Please also validate required partition columns using the file’s partition
data. Spark removes partition columns from the physical file, so their null
counts are absent and a null identity-partition value currently passes this
check.
##########
spark/v4.1/spark/src/main/java/org/apache/iceberg/spark/SparkTableUtil.java:
##########
@@ -229,8 +233,14 @@ private static Iterator<ManifestFile> buildManifest(
ManifestWriter<DataFile> writer =
ManifestFiles.write(formatVersion, spec, outputFile, snapshotId);
+ Set<Integer> requiredFieldIds = requiredFieldIds(schema);
try (ManifestWriter<DataFile> writerRef = writer) {
- fileTuples.forEachRemaining(fileTuple -> writerRef.add(fileTuple._2));
+ fileTuples.forEachRemaining(
+ fileTuple -> {
+ DataFile dataFile = fileTuple._2;
+ verifyRequiredFields(schema, requiredFieldIds, dataFile);
Review Comment:
Please delete the staging manifest if validation aborts, or validate before
opening the writer. A validation failure closes but does not delete the
manifest, and retries can leave multiple orphan files.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
