feat: hook native Parquet writes into Spark's WriteFilesExec seam on Spark 4.0+ - #5763
feat: hook native Parquet writes into Spark's WriteFilesExec seam on Spark 4.0+#5763andygrove wants to merge 5 commits into
Conversation
…Spark 4.0+ Native writes replace the whole DataWritingCommandExec, which means InsertIntoHadoopFsRelationCommand.run never runs. Everything that method does has to be re-implemented inside CometNativeWriteExec: a hardcoded SQLHadoopMapReduceCommitProtocol (so spark.sql.sources.commitProtocolClass is ignored), dynamicPartitionOverwrite pinned to false, a hand-ported copy of the SaveMode logic, a bespoke commit-message accumulator, and its own commitJob call. Most of the open native-writer issues are symptoms of that one decision rather than independent defects. On Spark 4.0+, V1WritesUtils.getWriteFilesOpt matches the WriteFilesExecBase trait (introduced in 4.0 precisely for this), so a Comet node that extends it gets driven through FileFormatWriter.executeWrite -> SparkPlan.executeWrite -> doExecuteWrite, and Spark keeps ownership of everything above the per-task write. Spark 3.x has no such trait: getWriteFilesOpt matches the concrete WriteFilesExec case class, a Comet node there would not be found, and Spark would silently take FileFormatWriter's non-planned, row-based branch. So the new seam is additive. CometDataWritingCommand and CometNativeWriteExec are kept unchanged and remain the 3.4/3.5 path; CometExecRule picks the path by version and the two never both fire. The legacy path goes away with Spark 3.x support. Add: - CometWriteFilesExec, overriding doExecuteWrite and mirroring FileFormatWriter.executeTask for the parts Comet must do itself: build the TaskAttemptContext, ask the commit protocol for a path, run the native writer, drive the stats trackers, commit or abort. Plus the CometWriteFiles serde and a two-line ShimCometWriteFilesExec in spark-4.x / spark-3.x. - File paths come from FileCommitProtocol.newTaskTempFile and are used verbatim, so names match Spark's part-<id>-<uuid>-c000.<codec>.parquet and committers that track individual files (S3A magic, streaming manifest) work. - Column names, nullability and field IDs come from WriteJobDescription.dataColumns rather than the query output, so INSERT INTO t SELECT a+1 writes the target column's name. - Byte and row counts come from BasicWriteTaskStatsTracker, which stats files through the FileSystem API and is therefore correct on HDFS. - ParquetWriter proto: work_dir is now optional. When set (3.x) the native writer derives the file name as before; when unset (4.0+) output_path is the exact file to write and is used verbatim. - On 4.0+ the opt-in moves to spark.comet.operator.WriteFilesExec .allowIncompatible, with the old DataWritingCommandExec key kept as a deprecated alternative. isOperatorAllowIncompat now resolves alternatives, which the planner's by-name lookup previously bypassed. AQE re-plans the write command's child and re-inserts a WriteFilesExec above the node Comet already converted; leaving DataWritingCommandExec in place means Comet no longer has to guard against the resulting nested native writes.
…configs Matches the reviewed form on apache#5293: ConfigBuilder mutates in place, so the Seq destructuring was rebinding the same object. Only one operator has an alternative and there is no reason to expect more.
| outputPathOf(op).foreach { outputPath => | ||
| val hadoopConf = op.session.sessionState.newHadoopConfWithOptions(op.options) | ||
| NativeConfig | ||
| .extractObjectStoreOptions(hadoopConf, URI.create(outputPath)) |
There was a problem hiding this comment.
Could we use new Path(outputPath).toUri here? Paths containing spaces or a literal % currently fail in URI.create, while the base successfully falls back to Spark. I tested this change and both cases write natively. Could you also add regression tests for these paths?
There was a problem hiding this comment.
Done — switched to new Path(outputPath).toUri, and made the same change in CometDataWritingCommand, which had the same URI.create call on the 3.x path. There the serde's catch turned it into a silent fallback rather than a failure, so the native writer was simply never used for those paths.
The regression test covers a directory with spaces, one with a literal %, and one with both, and it isn't version-gated so it runs against both writers. I checked it fails without the fix: IllegalArgumentException: Illegal character in path on 4.1, and zero native write operators in the plan on 3.5.
|
@andygrove thanks for the patch! |
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Reviewed df968953 against 7f1e0018. The prior writer replaced the entire DataWritingCommandExec, bypassing Spark's normal insertion, job-commit and catalog-refresh lifecycle. On Spark 4.0+, the new WriteFilesExecBase implementation leaves that lifecycle with Spark and supplies the per-task native write through doExecuteWrite. I compared this with the maintained Spark 3.5 and 4.0 branches: 3.5 discovers the concrete WriteFilesExec, whereas 4.0 discovers the base trait. Retaining the old path on 3.4/3.5 is therefore justified.
The task path follows Spark's job/task/attempt identifiers, requests the committer's exact filename, finishes the native writer before committing, and aborts/rethrows on write or commit failure. Partition zero retains a schema-only file for empty input; other empty partitions produce no file. Target names, nested types, nullability and field IDs come from WriteJobDescription.dataColumns. Partitioned/bucketed writes remain excluded, and the record-count rollover guard matches Spark's option-over-configuration precedence. The deprecated opt-in is respected, with an explicitly set new key taking precedence.
[P2] Preserve Hadoop path escaping
The existing path-parsing finding remains unresolved. The output-path tag contains Hadoop Path.toString, and URI.create(outputPath) throws for spaces or a literal %. The new serde has no enclosing fallback catch; the base catches that conversion error and retains Spark's writer. A local Java 17/Hadoop component check reproduced the exception for both local and HDFS path strings, with ordinary paths as controls; Path.toUri handled all six cases. This supports the existing comment, so I have not added a duplicate inline. This local check exercised path conversion only, not a complete Spark write.
Validation and limits
The inspected CI jobs checked out merge 2e35d315, whose parents are the reviewed base/head and whose entire tree equals the reviewed head. CometParquetWriterSuite passed 41/41 on Spark 4.0 and 41/41 on Spark 4.1; Spark 3.5 passed 33 with eight version-gated cancellations, and 3.4 passed 32 with nine cancellations. Spark 4.0's metrics suite passed 15/15, including native-write output metrics. The Rust job passed 1,184 tests, including exact-path writing, with four skipped. The JVM consumers downloaded the same native artifact digest uploaded after the producer rebuilt this source.
At the reviewed snapshot, 63 checks passed, nine were skipped, and one Iceberg check failed downloading Gradle with HTTP 504 before its tests started. Maintained 3.4, 4.1 and 4.2 source branches were unavailable; CI coverage does not replace those source comparisons. No local Spark/native integration build or benchmark was run. The injected failure test checks abort and a subsequent whole-write retry; concurrent speculative attempts remain untested. The INSERT visibility test verifies readback but does not independently assert the native write plan. I found no additional verified P1/P2 issue in the authored changes.
Performance
The write stays columnar and preserves the existing native Parquet implementation. Resolving NativeWriteTask on the driver avoids capturing the whole execution-plan object in the task closure, and the small writer plan is rebuilt per task because its destination is task-specific. File-size accounting now uses Spark's filesystem-aware tracker, which also supplies the task output counters.
recordRows adds one callback per written row after native writing completes. Keeping the tracker loop outside the row loop limits dispatch overhead, but the work still scales with row count. A focused large-batch, narrow-row write benchmark with BasicWriteTaskStatsTracker would quantify this cost before making a throughput claim. The functional tests establish correctness coverage, not a measured speedup; I have no verified performance blocker.
Design
Using Spark's existing task-write seam gives job commit, SaveMode handling, _SUCCESS, cache refresh and catalog statistics one owner. Passing the exact filename back to native code also removes the old assumption that the committer only cares about a staging directory. The filesystem admission gate still limits this path to local/HDFS output; it does not establish support for object-store committers.
The conservative command/native-child/partition/bucket/rollover guards keep the supported case understandable. The maintained InsertIntoHadoopFsRelationCommand supplies the basic count-only stats tracker, which fits the current implementation. Row-inspecting trackers need a separate compatibility solution before broadening that scope; a warning cannot supply their missing row contents. Experimental opt-in remains appropriate while the disclosed timestamp/footer and Parquet writer-property gaps remain.
Abstraction & complexity
The two small version shims isolate the actual Spark API difference. NativeWriteTask has a clear serialization purpose, and the static task runner keeps executor work separate from driver plan state. Preserving the write node as originalPlan, together with restarting child native-block serialization, also makes the AQE ownership boundary explicit.
The optional work_dir remains the discriminator between the old directory-based path and the new exact-file path without changing protobuf field numbers. Both Scala execution paths are necessary while Spark 3.x is supported; the compatibility branch is localized enough to remove with that support. Beyond the remaining path-parsing correction, I found no actionable abstraction or complexity blocker.
The output path reaches both write serdes as `Path.toString`, which decodes percent escapes: a directory containing a space or a literal `%` yields a string that is not a valid URI, and `URI.create` throws on it. On Spark 4.0+ that exception escaped `CometWriteFiles.convert` and failed the query. On 3.x `CometDataWritingCommand.convert` caught it and silently handed the write back to Spark, so the native writer was never used for those paths. Round-trip through `Path` instead, which re-escapes. Only the scheme and authority reach `extractObjectStoreOptions`, but parsing has to succeed to get at them. Also assert that the INSERT INTO visibility test's write actually went native, rather than inferring it from the read-back.
|
Path escaping is fixed in d36ad9f, in both serdes — the 3.x one had the same I also took your point on the INSERT visibility test: it now captures the executed plan and asserts a native write, so a fallback can't make the read-back pass for the wrong reason. On |
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked d36ad9ff against 7f1e0018, including the three-file increment from the previous review. The original Java URI-parsing finding is fixed: Spark 3.x uses the command's Path.toUri, and Spark 4.0+ reconstructs the tagged Hadoop path before extracting configuration. The new local-path test checks native-plan presence and readback for spaces, literal percent signs, and their combination. The INSERT visibility test now asserts its native write as well.
[P2] Preserve fallback for escaped HDFS destinations
The Spark 3.x change also admits HDFS paths that previously fell back to Spark, exposing a separate native destination mismatch. With a native child reading local input and a cold HDFS object-store cache, a destination containing dir with space reaches create_hdfs_object_store, which uses Path::parse(url.path()). The URL path contains dir%20with%20space, and that constructor preserves the percent encoding. The native writer passes this different directory through OpenDAL and libhdfs. Spark's committer still owns the original directory containing spaces. This can leave data outside the committer's staging tree while job commit reports success. The same destination mismatch affects the newly admitted Spark 4.0+ case.
Please keep these HDFS destinations on Spark during planning until the native path conversion preserves Hadoop's filename semantics. Fixing the native conversion should cover both cold and warm cache paths, which currently use different path constructors. The new local-filesystem tests cannot detect this HDFS branch. This is a downstream issue exposed by the fallback removal, separate from the fixed URI.create exception.
I verified the path flow against the locked URL, object-store, OpenDAL and hdrs sources. A Java 17/Hadoop 3.4.1 component check passed eight planning cases and confirmed that the committer and traced native paths are distinct. A second check used the real Hadoop FileOutputCommitter on local storage with that destination mismatch injected: both algorithms 1 and 2 created _SUCCESS without moving the misplaced data file into the intended output. These are component checks plus source analysis, not a Spark/native/HDFS end-to-end reproduction.
The maintained Spark 4.0 comparison still supports the unchanged write lifecycle: eligibility declines happen during planning. Spark owns insertion and job commit/abort. Native writing finishes before task commit. Task failures abort and rethrow. Partitioned, bucketed and record-rollover writes remain excluded. An execution-time fallback would be too late once overwrite deletion or task output has occurred.
Validation and limits
Current CI's Rust job passed 1,208 tests with five skipped, including the exact-file-path unit test. It executed merge 45074b00 with the reviewed head and newer main 8e684685, not the authoritative base/head tree. Fourteen of the fifteen contributed files are identical to the reviewed head. The differing planner file has an identical Parquet-writer arm. At the 2026-09-08T17:47:28Z refresh, 40 checks passed, seven were skipped, 19 were running and one was queued. Current JVM writer-suite outcomes have not been independently verified in this review. The previous head's passing writer suites and the author's updated test counts are not current independent validation. Maintained Spark 3.4/4.1/4.2 source gaps remain. No local Spark/native build, HDFS cluster run or speculative-attempt test was performed.
Performance
The URI correction adds one Hadoop Path reconstruction during Spark 4.0+ planning and reuses the existing path on 3.x. It adds no per-batch or per-row work. The task writer and its per-row statistics callbacks are unchanged. The author now explicitly acknowledges that their cost has not been measured. No throughput conclusion follows from this update.
Design
The write framework remains Spark-owned, and the corrected local path reaches the existing task writer without changing commit ordering. The remaining HDFS issue belongs at the boundary between Hadoop filenames and native URL paths. A planning-time restriction is sufficient to preserve the prior safe behavior while that boundary is fixed. Catching errors after native writing starts would not recover files written outside the committer's directory.
Abstraction & complexity
Reusing cmd.outputPath.toUri on 3.x avoids an unnecessary round trip. The Spark 4.0+ conversion stays localized where the string tag is consumed. The by-name captureWritePlan overload reuses the listener setup for INSERT and improves the test without adding production machinery. No additional abstraction change is needed beyond correcting or restricting the HDFS path boundary.
| // give the write back to Spark). Going through `Path` escapes them again. | ||
| val objectStoreOptions = | ||
| NativeConfig.extractObjectStoreOptions(hadoopConf, URI.create(outputPath)) | ||
| NativeConfig.extractObjectStoreOptions(hadoopConf, cmd.outputPath.toUri) |
There was a problem hiding this comment.
Correctness
[P2] Keep escaped HDFS destinations on Spark until native path decoding is fixed
This removes the Spark 3.x fallback for HDFS destinations containing spaces, but the native HDFS path is still different from the committer's path. With local native input and a cold HDFS object-store cache, create_hdfs_object_store passes url.path() to object_store::path::Path::parse, retaining dir%20with%20space. ParquetWriter then sends that path through OpenDAL/libhdfs, while Spark commits the original dir with space staging directory. Data can therefore remain outside the committed output even though job commit succeeds. The local-path regression test does not exercise this branch. Please decline such HDFS writes during planning until native path handling preserves Hadoop filenames on both cold and warm cache paths. This is a source-confirmed destination mismatch. A Hadoop committer component probe confirmed the commit consequence, but I did not run an end-to-end HDFS write.
There was a problem hiding this comment.
Fixed in 4095ea6 — declined at planning, as you asked.
You're right that the previous change fixed the wrong half for HDFS. Making URI.create stop throwing removed the accidental fallback, but the native and committer paths still name different directories, so all that did was convert a benign fallback into a silent data-placement bug. A write that commits successfully with the data somewhere else is a worse outcome than not accelerating it.
NativeWriteUtils.escapedHdfsDestination is the new gate, called from getSupportLevel in both serdes — CometWriteFiles for 4.0+ and CometDataWritingCommand for 3.x — since the mismatch is in the native object store and does not care which seam the write came through. It compares uri.getRawPath against uri.getPath, which differ exactly when the path contained something the URI form had to escape, and it is scoped to hdfs: so local writes keep the behaviour the earlier fix gave them.
The fallback reason names the consequence rather than the rule, so the EXPLAIN output is actionable:
HDFS output paths needing URI escaping are not supported: the native writer would write to the escaped path while Spark commits the unescaped one (
/dir with space/output.parquet)
The regression test is HDFS output paths needing URI escaping are declined at planning. It exercises the predicate directly rather than through a write, since I have no HDFS to write to here — it covers a space, a literal %, and a nested escaped segment as declined, and asserts the two things that would make the gate too broad: an ordinary hdfs: path still passes, and file: paths with either shape still pass. The existing local-path test still writes natively.
I have not tried to fix the native path handling here. When that lands, this gate is one call to delete, and the reason string points at what has to change.
sunchao
left a comment
There was a problem hiding this comment.
Correctness
Rechecked 4095ea6f against 7f1e0018, including the four-file update since d36ad9ff. The new Unsupported checks run during planning in both serdes and cannot be bypassed by allowIncompatible. They fix the reported space/percent examples while preserving local native writes. The original Java URI-parsing fix remains intact.
[P2] Also decline Unicode HDFS destinations
The existing destination-mismatch finding remains for paths such as hdfs://ns/café/output. Hadoop's URI returns /café/output from both getRawPath and getPath, so the new predicate returns None. The locked Rust URL parser percent-encodes non-ASCII UTF-8 bytes, producing caf%C3%A9. With a native child reading local input and a cold HDFS object-store cache, Path::parse(url.path()) preserves that encoding and the native writer still creates files outside Spark's Unicode staging directory. This is the same unresolved P2, narrowed to a case the new guard misses.
Please also decline non-ASCII HDFS paths, or use an escape check aligned with the native URL parser, and add Unicode inputs alongside the current space/percent cases. The ordinary-HDFS and local-path controls should continue to pass. A Java getRawPath != getPath comparison alone does not cover the native encoding boundary.
I executed the exact current Scala method, copied byte-for-byte into a small Scala 2.13.17/Hadoop 3.4.1 component: all six existing controls behaved as expected, while accented, CJK, emoji and combining-character paths were admitted. Source inspection of locked url 2.5.8, percent-encoding 2.3.2 and the downstream object-store/HDFS dependencies confirms the different native filename. A real Hadoop FileOutputCommitter check with that Unicode destination mismatch injected produced _SUCCESS without the data file in the intended output under both commit algorithms. These are component checks plus source analysis. I did not run a Spark/native/HDFS write.
The maintained Spark 4.0 comparison confirms that its commit protocol returns Hadoop filenames and Spark owns job commit/abort. Native task output must land under that same staging directory before task commit. Spark 3.5 retains the concrete WriteFilesExec route and this PR's legacy writer selection. The new guard is correctly placed before write side effects, but its incomplete predicate leaves the Spark 4.0+ seam exposed. I am not claiming that Unicode handling newly regresses the pre-existing Spark 3.x writer.
Validation and limits
Current writer-suite logs show 43 passed on Spark 4.0, 4.1 and 4.2. Spark 3.4 has 34 passed/9 canceled and 3.5 has 35 passed/8 canceled. The new guard test passes in all five jobs, but contains no Unicode case. The Rust job passed 1,259 tests with five skipped, including the exact-file-path writer test.
These jobs checked out merge fc17a3b3 with this head and newer main 424c31aa, not the assigned base/head tree. Twelve of sixteen authored files match the head. The guard, both serdes, task writer and writer suite are identical, as are the checked writer regions in differing files. At 2026-09-09T18:21:01.391690+00:00, 51 checks passed, ten were skipped, one failed and one was queued. The failed Spark 4.1 SQL build reports runner shutdown/cancellation. It does not establish a source compilation failure. Maintained Spark 3.4/4.1/4.2 source gaps remain. No full local Comet build, HDFS cluster or speculative-attempt run was performed.
Performance
The helper adds path parsing and comparison during HDFS planning. Local paths return before that work, and the update adds no per-row or per-batch processing. The existing per-row statistics callbacks are unchanged. Their cost remains unmeasured. This update supplies no throughput evidence.
Design
A shared planning restriction is a suitable bounded mitigation while native path conversion remains unfixed. Both write paths consult it before execution, which avoids attempting recovery after overwrite deletion or misplaced output. Complete the predicate for native Unicode escaping. Cold and warm object-store paths still use different constructors and must both be considered when lifting the restriction.
Abstraction & complexity
The shared helper keeps one restriction and one diagnostic across the two serdes without changing their execution lifecycle. No additional abstraction is needed. Its comment currently overstates what Java's raw/decoded comparison detects. Correcting that explanation together with the Unicode guard and tests is part of the same P2.
The raw/decoded URI comparison only catches what java.net.URI had to
escape, and java.net.URI leaves non-ASCII path characters alone, so an
hdfs://ns/cafe<U+0301>/output destination was admitted. percent_encoding's
should_percent_encode is !byte.is_ascii() || set.contains(byte), so the
native parser escapes every non-ASCII byte regardless of the encode set
and the writer creates caf%C3%A9 outside Spark's staging directory.
The guard now also declines any character the native parser rewrites. The
ASCII half of that set was determined against the locked url 2.5 crate by
parsing hdfs://ns/pre<c>post/output for every printable ASCII c: space, ",
#, <, >, ?, backtick, { and } are rewritten and the rest survive, so
partition directories and Spark's _temporary attempt paths still qualify.
The comment no longer claims the Java comparison detects the divergence on
its own; both conditions are kept because the Java one still catches a
literal % that the native parser leaves alone.
Tests add accented (precomposed and combining), CJK, emoji and nested
non-ASCII cases plus the remaining escaped ASCII characters, built from
code points since scalastyle forbids non-ASCII source. Disabling the new
condition makes the accented case fail, so the Java comparison alone
demonstrably does not cover it.
|
You were right, and the reason the first guard missed it is worth writing down: Rather than special-case non-ASCII, I derived the whole set the native parser rewrites from the locked So nine ASCII characters plus every non-ASCII byte, and notably I kept the Java comparison as a second condition rather than replacing it. The native parser leaves Tests cover accented Latin both precomposed (U+00E9) and as I checked the new condition is load-bearing rather than assuming it. Disabling only With it enabled, all 43 On the lint failure you spotted in the earlier round: the first run of the suite here failed 43 of 43 for an unrelated reason worth recording, a stale |
Which issue does this PR close?
Part of #2967 and #1625. Restructures the native write path on Spark 4.0+ so the
following can be fixed at all, and closes the ones that were purely symptoms of the
old design:
Closes #2985 (no
_SUCCESSfile)Closes #3521 (
INSERT INTO ... SELECTinvisible to subsequent reads)Closes #3426 (complex type with different names)
Unblocks (not fixed here, but no longer require re-implementing Spark's write
framework inside Comet): #2957, #2970, #3015, #3041, #3193, #3194, #3417, #3428.
Supersedes #5293, which made the same change but removed the Spark 3.x writer along
the way. That regression is what held #5293 back, so this version is purely additive:
Spark 3.4/3.5 keep the existing native writer, unchanged.
Rationale for this change
Native writes replace the whole
DataWritingCommandExec, which meansInsertIntoHadoopFsRelationCommand.runnever runs. Everything that method does has tobe re-implemented inside
CometNativeWriteExec: a hardcodedSQLHadoopMapReduceCommitProtocol(sospark.sql.sources.commitProtocolClassisignored),
dynamicPartitionOverwritepinned tofalse, a hand-ported copy of theSaveMode logic, a bespoke commit-message accumulator, and its own
commitJobcall.Most of the open native-writer issues are symptoms of that one decision rather than
independent defects. Fixing them one at a time against the old design means writing a
second, worse
FileFormatWriterinside Comet.Spark 4.0 added the right seam.
V1WritesUtils.getWriteFilesOptmatches theWriteFilesExecBasetrait there (introduced in 4.0 precisely for this), so a Cometnode that extends it gets driven through
FileFormatWriter.executeWrite→SparkPlan.executeWrite→doExecuteWrite, and Spark keeps ownership of everythingabove the per-task write.
Why this is additive rather than a replacement. On 3.4/3.5
getWriteFilesOptmatches the concrete
WriteFilesExeccase class. A Comet node there would not befound,
writeFilesOptwould beNone, and Spark would silently takeFileFormatWriter's non-planned, row-based branch, ignoringdoExecuteWriteentirely. The only way in on 3.x is to inherit from a case class, which brings
copy/equalshazards. SoCometDataWritingCommandandCometNativeWriteExecstayexactly as they are and remain the 3.4/3.5 path.
CometExecRulepicks the path bySpark version and the two never both fire. The 3.x path goes away with 3.x support.
What changes are included in this PR?
Spark 4.0+:
Spark 3.4/3.5 is unchanged:
CometWriteFilesExecoverridingdoExecuteWrite, mirroringFileFormatWriter.executeTaskfor the parts Comet must do itself: build theTaskAttemptContext, ask the commit protocol for a path, run the native writer,drive the stats trackers, commit or abort. Plus the
CometWriteFilesserde and atwo-line
ShimCometWriteFilesExecinspark-4.x/spark-3.x.CometNativeWriteExec,CometDataWritingCommand,CometMetricNode.reportNativeWriteOutputMetricsand theEliminateRedundantTransitionsrule for native writes all remain and serve 3.x.FileCommitProtocol.newTaskTempFileand are used verbatim,so names match Spark's
part-<id>-<uuid>-c000.<codec>.parquetand committers thattrack individual files (S3A magic, streaming manifest) work. The 3.x writer keeps
inventing its own names.
Pathbefore being parsed as a URI. Bothserdes receive it as
Path.toString, which decodes percent escapes, so a directorycontaining a space or a literal
%produced a stringURI.createrejects - a queryfailure on 4.0+, and a silent fallback to Spark's writer on 3.x. Covered by a
version-independent test.
WriteJobDescription.dataColumnsrather than the query output, soINSERT INTO t SELECT a+1writes the target column's name ([COMET NATIVE WRITER] INSERT INTO TABLE - complex type but different names #3426).BasicWriteTaskStatsTracker, which stats files through theFileSystemAPI and is therefore correct on HDFS. The native writer'sstd::fs::metadatacall reports0there.ParquetWriter.work_dirbecomes genuinely optional. When it is set (3.x) thenative writer derives the file name from it as before; when it is unset (4.0+),
output_pathis the exact file to write and is used verbatim.output_pathwasalready unused on the 3.x path, so no field changes meaning for an existing plan.
spark.comet.operator.WriteFilesExec.allowIncompatible,with the old
DataWritingCommandExeckey kept as a deprecated alternative.CometConf.isOperatorAllowIncompatnow resolves alternatives; the planner's by-namelookup previously bypassed the
ConfigEntry, so an old key would have readtruefrom the entry while the planner saw
false.WriteFilesExecdeclines dynamic partition overwrite (it is always a partitionedwrite) and
spark.sql.files.maxRecordsPerFile, which Spark's own writer uses to rolla new file every N rows.
Fixed along the way
AQE re-plans the write command's child and re-inserts a
WriteFilesExecabove thenode Comet already converted. On the 3.x path that needs an explicit guard in
CometExecRule(still present). LeavingDataWritingCommandExecin place on 4.0+means the situation cannot arise there.
How are these changes tested?
CometParquetWriterSuite: 42/42 on Spark 4.0 and 4.1: the 33 existing tests pluseight new ones for
_SUCCESS(Comet writer doesn't create _SUCCESS file #2985), Spark-compatible file naming,INSERT INTO ... SELECTvisibility ([Native Writer] INSERT INTO ... SELECT fails due to stale catalog cache after write #3521), dynamic-overwrite fallback, themaxRecordsPerFilefallback (both the write option and the conf, verifying Spark'swriter rolls 10 files), the schema-only empty-input write (SPARK-23271), task abort
and retry through an injected failing commit protocol, and the deprecated opt-in key.
Those eight
assume(isSpark40Plus). A ninth covers output paths that need URIescaping and runs on every version.
CometParquetWriterSuiteon Spark 3.4 (33/33 + 9 skipped) and 3.5 (34/34 + 8skipped): every pre-existing test still passes on the 3.x writer, which is the point
of keeping it.
CometTaskMetricsSuite: 15/15 on both 3.5 and 4.1. The suite's native-write test nowpicks the version-appropriate opt-in key, so it genuinely exercises the native path on
both.
CometExecSuite(144),CometFallbackInvarianceSuite(6),CometPublicApiSuite(1).parquet_writerunit test asserting the writer uses acommit-protocol-chosen path verbatim (and that a non-zero partition id does not leak
into the name).
cargo test -p datafusion-comet parquet_writerandcargo clippy --all-targets --workspace -- -D warningspass.Known limitation
WriteTaskStatsTracker.newRow(filePath, row)is a per-row callback. Comet has columnarbatches, so rather than materializing every row just to hand it straight back,
recordRowspassesInternalRow.emptyand feeds only the count. That is exactly rightfor
BasicWriteTaskStatsTracker, which ignores the row argument, but a third-partytracker inspecting row contents would see empty rows, so that case logs a warning
rather than silently reporting wrong statistics. A plan-time guard isn't possible
because
statsTrackersonly exists at execution time.Follow-ups
Independent of this change and the next highest-value work, since the Spark default is
affected: full
WriterProperties(block/page size, dictionary, writer version), INT96timestamps (#3425:
spark.sql.parquet.outputTimestampTypedefaults toINT96and wewrite INT64 micros), and the four footer metadata keys (#3427:
legacyINT96andtimeZonedrive rebase decisions on read, so omitting them is a correctness risk).Then partitioned (#3193) → bucketed (#3194) → object stores.