Uh oh!
There was an error while loading. Please reload this page.
test: enable the Iceberg split-operator and native write by default to surface test failures - #5677
test: enable the Iceberg split-operator and native write by default to surface test failures#5677andygrove wants to merge 4 commits into
Conversation
Both toggles have defaulted to false since they landed, so no CI job has ever exercised the two-operator plan or the iceberg-rust writer outside the handful of suites that set them explicitly. Flip the defaults to find out what the Iceberg Spark SQL suites and Comet's own Iceberg suites say when every write goes through them. Exploratory: `iceberg-writes.md` still documents both as off by default and is deliberately not updated here.
The Iceberg test sessions already configure the write split operator, but they leave `spark.comet.exec.localTableScan.enabled` off. Spark emits a row-based `LocalTableScanExec` for an inline `VALUES` list, and `CometIcebergNativeWrite` sets `requiresNativeChildren`, so those writes are declined and silently run on the JVM writer. Iceberg's suites seed data that way constantly, so the native writer stayed hidden from most of the write surface even with `spark.comet.iceberg.write.enabled` on. Measured on Spark 4.1.3 with the Iceberg 1.11 runtime and the Iceberg CI session config: unpartitioned `INSERT INTO ... VALUES` and `df.writeTo(...).append()` both selected the JVM writer before this change and the native writer after it. Regenerated all four diffs against their own tags per the contributor guide. Also document the flag and the existing `spark.comet.explainFallback.enabled` setting, which reports every declined operator with its reason, and drop the stale claim that the split-operator flag is off by default.
CI failure baseline: |
| Count | Cause | Suites |
|---|---|---|
| 34 | Native writer throws CometNativeException: Unexpected => The input is not sorted! Cannot write to partition that was previously closed where Iceberg expects IllegalStateException | TestRequiredDistributionAndOrdering (32), TestMetadataTablesWithPartitionEvolution (2) |
| 28 | Target file size not honored: writer emits half the expected data files (8 -> 4, 4 -> 2) | TestSparkDataWrite (24), TestRewriteDataFilesAction (4) |
| 11 | Plan assertions expect the two-op IcebergWriteExec, plan now has CometIcebergWrite | CometIcebergWriteActionSuite (same 11 in all 5 [scans] jobs) |
| 8 | Native panic: called Option::unwrap() on a None value | TestSparkDataFile |
| 5 | Wrong query results after partition evolution (expected: 1 but was: 3) | TestMetadataTablesWithPartitionEvolution |
| 5 | Native panic: itertools: .zip_eq() reached end of one iterator before the other | TestAlterTablePartitionFields |
| 5 | Invalid Iceberg scan task: DataInvalid => Non-empty FileScanTask partition requires a partition spec | TestForwardCompatibility (3), TestRewriteDataFilesAction (2) |
| 1 | Unexpected => No column with source column id 2 in schema | TestAlterTablePartitionFields |
Per-job Iceberg counts: 1.8/3.4 spark 14, extensions 4; 1.9/3.5 spark 16, extensions 4; 1.10/3.5 spark 15, extensions 6; 1.11/4.1 spark 19, extensions 8.
Counts are test-case instances summed over jobs, so the same root cause is counted once per Iceberg version and once per Spark profile. Will repost this summary as fixes land on the branch.
Correction to the table above. Two rows were mis-grouped by the log parser (a stack trace bled into the next test's window). Corrected split, same 86 total:
- "The input is not sorted!" is 32, all
TestRequiredDistributionAndOrdering. The 2TestMetadataTablesWithPartitionEvolutioncases attributed to it are actually row-order divergences. - Wrong data-file counts is 30:
TestSparkDataWrite24 +TestRewriteDataFilesAction6 (bothtestBinPackCombineMediumFilesandtestRemoveDangledPositionDeletesPartitionEvolutionare count assertions, not native errors). Noted on Native Iceberg writer does not honour write.target-file-size-bytes the same way as iceberg-java #5690. - Row-order divergence is 7, all
testPartitionColumnNamedPartition. Filed as Iceberg TestMetadataTablesWithPartitionEvolution.testPartitionColumnNamedPartition returns rows in a different order with native Iceberg writes enabled #5776. Non-empty FileScanTask partition requires a partition specis 3, allTestForwardCompatibility. Fixed by fix: read Iceberg tables partitioned by an unknown transform #5759 (Iceberg native scan fails on a table partitioned by an unknown transform (TestForwardCompatibility CI failure) #5758).
Issue coverage for the eight causes: #5698 and #5690 open; #5691, #5693, #5694 (all fixed by #5729) and #5758 (fixed by #5759) closed and now on main; #5776 newly filed; the 11 CometIcebergWriteActionSuite plan assertions are test staleness rather than a bug and belong on the #5644 checklist.
This branch has since merged main (f15d424), which picks up #5729 and #5759, so the next run should clear the 16 native-panic and scan failures. Next summary will come from that run.
Which issue does this PR close?
None, this is exploratory. Related to #5308 (native Iceberg write) and the split-operator work in #4658.
Draft, not for merge. The point is to find out what breaks.
Rationale for this change
spark.comet.iceberg.write.enabledhas defaulted tofalsesince it landed, so the iceberg-rust writer hasonly ever run in the handful of Comet suites that set it explicitly:
CometIcebergWriteActionSuite,CometIcebergWriteDetectionSuite,CometIcebergRewriteActionSuite, andCometIcebergSystemFunctionSuite.Everything else, including the Iceberg Spark SQL suites that run Iceberg's own tests against Comet, has been
writing through Spark's stock executor-side writer. So we do not currently know how much of the Iceberg test
surface the native writer actually passes.
spark.comet.write.iceberg.splitOperator.enabledis a different story. The Iceberg test diffs have set it totruein every Comet-configured session since #5259, so the two-operator plan has been running againstIceberg's write, commit, and row-level-operation tests all along. Only the native writer is genuinely new
here, and flipping the user-facing default keeps the two consistent.
Turning the native write flag on is not sufficient on its own.
CometIcebergNativeWritesetsrequiresNativeChildren, and Spark emits a row-basedLocalTableScanExecfor an inlineVALUESlist, so awrite fed that way is declined and silently runs on the JVM writer. Iceberg's suites seed data with
INSERT INTO ... VALUESanddf.writeTo(...).append()constantly, and nothing in those suites asserts whichwriter produced the files, so without
spark.comet.exec.localTableScan.enabledthe native writer would stayhidden from a large part of the write surface while everything still passed green.
Measured locally on Spark 4.1.3 with the Iceberg 1.11 runtime, using the session config the Iceberg diffs
install:
INSERT INTO ... VALUES, unpartitioneddf.writeTo(...).append(), unpartitionedWrites that already had a Comet-native child reached the native writer either way: partitioned
INSERT INTO ... VALUES,INSERT INTO ... SELECT, copy-on-writeUPDATE, CTAS, and a target carryingwrite.parquet.compression-codec.What changes are included in this PR?
CometConf.scala: defaultspark.comet.write.iceberg.splitOperator.enabledandspark.comet.iceberg.write.enabledtotrue, and drop the now false "Off by default" sentence from thenative write config doc.
dev/diffs/iceberg/{1.8.1,1.9.1,1.10.0,1.11.0}.diff: setspark.comet.exec.localTableScan.enabled=truealongside the existing Comet configuration in every Comet-configured session, so
VALUESdriven andDataFrame driven writes can reach the native writer instead of silently falling back. Each diff was
regenerated against its own tag following
docs/source/contributor-guide/iceberg-spark-tests.md.docs/source/contributor-guide/iceberg-spark-tests.md: document that flag and the existingspark.comet.explainFallback.enabledsetting, and drop the stale claim that the split-operator flag is offby default.
Deliberately not included:
docs/source/user-guide/latest/iceberg-writes.mdstill says both features areexperimental and off by default, and still lists
splitOperator.enabledbeingfalseas the first fallbackreason. If any of this is worth keeping, the guide has to be rewritten, but that is premature until we see the
failures.
How are these changes tested?
By CI, which is the whole point. The
run-iceberg-testslabel is applied so the Iceberg 1.8 / 1.9 / 1.10 jobsrun alongside 1.11, giving coverage across Spark 3.4 / 3.5 / 4.1 and four Iceberg versions. Comet's own
Iceberg suites run in the
[scans]bucket on every profile regardless.The Iceberg diffs also set
spark.comet.explainFallback.enabled=true, so Comet logs every operator itdeclines together with the reason. That output is not usable from CI as things stand: gradle does not forward
Spark's log output into the job log, and a 130k line Iceberg job log contains zero WARN lines. Reading fallback
rates needs either a gradle test-logging change or a local run. Until then, the pass or fail result is all CI
gives us, and it does not distinguish a native write from a silent fallback.
The regenerated diffs were checked to apply cleanly against their tags and to produce one
localTableScan.enabledline per existingsplitOperator.enabledline. The measurement in the table abovecame from a throwaway probe suite built on the same session config the diffs install.
What the run found
All eight Iceberg jobs fail. The failures are dominated by one bug, filed as #5689: converting
IcebergWriteExectoCometIcebergWriteExecdrops theColumnarToRowthat sat between Iceberg's columnarBatchScanand the whole-stage codegen stage above it, so every copy-on-write DELETE, UPDATE and MERGE dieswith
ColumnarBatch cannot be cast to InternalRow. It only shows up when AQE is disabled, which is why noexisting Comet suite caught it. Isolated locally: the split-operator plan alone does not trigger it and the
local table scan flag is irrelevant, it needs the native write flag plus AQE off.
Beware of reading the extension-suite failure counts as a trend. Iceberg's
ExtensionsTestBasesetsADAPTIVE_EXECUTION_ENABLEDtoRANDOM.nextBoolean()per session, so the number of tests #5689 takes downswings between runs of the same behaviour (99 in one run, 31 in the next).
Also found:
write.target-file-size-bytes, producing consistently half as many files, which also perturbs compactionresults in
TestRewriteDataFilesAction.ManifestWriter::construct_partition_summaries(zip_eqlength mismatch) when writing toa table whose partition spec has been evolved.
No column with source column id 2 in schemaafter an identity partition field and its source columnare dropped. Same phase as Native Iceberg write panics in construct_partition_summaries after partition spec evolution #5691 and possibly the same root cause.
Option::unwrap()panic inmicroseconds_to_datetimetzwhile building the partition path for atimestamptzpartition column.TestRequiredDistributionAndOrdering.testDisabledDistributionAndOrderingexpectsIllegalStateExceptionfrom the JVM clustered writer and getsCometNativeException: The input is not sorted!instead. Same semantics, different exception type. This one is deterministic on all four Icebergversions and passed before the local table scan flag was added, because the write used to fall back to the
JVM writer.
In the
[scans]jobs, severalCometIcebergWriteActionSuitecases fail withexpected >= 1 IcebergWriteExec in captured plans, got 0. Those tests assert that the JVM split writer ispresent in the plan, and the native writer now takes over instead. Their premise has to be revisited if these
defaults ever ship.