Skip to content

feat: hook native Parquet writes into Spark's WriteFilesExec seam on Spark 4.0+ - #5763

Open
andygrove wants to merge 5 commits into
apache:mainfrom
andygrove:native-write-files-seam
Open

feat: hook native Parquet writes into Spark's WriteFilesExec seam on Spark 4.0+#5763
andygrove wants to merge 5 commits into
apache:mainfrom
andygrove:native-write-files-seam

Conversation

@andygrove

@andygrove andygrove commented Sep 7, 2026

Copy link
Copy Markdown
Member

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 _SUCCESS file)
Closes #3521 (INSERT INTO ... SELECT invisible 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 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. Fixing them one at a time against the old design means writing a
second, worse FileFormatWriter inside Comet.

Spark 4.0 added the right seam. V1WritesUtils.getWriteFilesOpt matches the
WriteFilesExecBase trait there (introduced in 4.0 precisely for this), so a Comet
node that extends it gets driven through FileFormatWriter.executeWrite
SparkPlan.executeWritedoExecuteWrite, and Spark keeps ownership of everything
above the per-task write.

Why this is additive rather than a replacement. On 3.4/3.5 getWriteFilesOpt
matches the concrete WriteFilesExec case class. A Comet node there would not be
found, writeFilesOpt would be None, and Spark would silently take
FileFormatWriter's non-planned, row-based branch, ignoring doExecuteWrite
entirely. The only way in on 3.x is to inherit from a case class, which brings
copy/equals hazards. So CometDataWritingCommand and CometNativeWriteExec stay
exactly as they are and remain the 3.4/3.5 path. CometExecRule picks the path by
Spark 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+:

Execute InsertIntoHadoopFsRelationCommand   <- Spark: SaveMode, catalog, commitJob, _SUCCESS
+- CometWriteFiles                          <- Comet: native per-task write only
   +- CometNativeScan ...

Spark 3.4/3.5 is unchanged:

CometNativeWrite                            <- Comet: the whole write
+- CometNativeScan ...
  • New CometWriteFilesExec overriding doExecuteWrite, 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.
  • Nothing is deleted. CometNativeWriteExec, CometDataWritingCommand,
    CometMetricNode.reportNativeWriteOutputMetrics and the
    EliminateRedundantTransitions rule for native writes all remain and serve 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. The 3.x writer keeps
    inventing its own names.
  • The output path is re-escaped through Path before being parsed as a URI. Both
    serdes receive it as Path.toString, which decodes percent escapes, so a directory
    containing a space or a literal % produced a string URI.create rejects - a query
    failure on 4.0+, and a silent fallback to Spark's writer on 3.x. Covered by a
    version-independent test.
  • Column names, nullability and Parquet field IDs come from
    WriteJobDescription.dataColumns rather than the query output, so
    INSERT INTO t SELECT a+1 writes the target column's name ([COMET NATIVE WRITER] INSERT INTO TABLE - complex type but different names #3426).
  • Byte/row counts come from BasicWriteTaskStatsTracker, which stats files through the
    FileSystem API and is therefore correct on HDFS. The native writer's
    std::fs::metadata call reports 0 there.
  • Proto: ParquetWriter.work_dir becomes genuinely optional. When it is set (3.x) the
    native writer derives the file name from it as before; when it is unset (4.0+),
    output_path is the exact file to write and is used verbatim. output_path was
    already unused on the 3.x path, so no field changes meaning for an existing plan.
  • On 4.0+ the opt-in moves to spark.comet.operator.WriteFilesExec.allowIncompatible,
    with the old DataWritingCommandExec key kept as a deprecated alternative.
    CometConf.isOperatorAllowIncompat now resolves alternatives; the planner's by-name
    lookup previously bypassed the ConfigEntry, so an old key would have read true
    from the entry while the planner saw false.
  • WriteFilesExec declines dynamic partition overwrite (it is always a partitioned
    write) and spark.sql.files.maxRecordsPerFile, which Spark's own writer uses to roll
    a new file every N rows.

Fixed along the way

AQE re-plans the write command's child and re-inserts a WriteFilesExec above the
node Comet already converted. On the 3.x path that needs an explicit guard in
CometExecRule (still present). Leaving DataWritingCommandExec in 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 plus
    eight new ones for _SUCCESS (Comet writer doesn't create _SUCCESS file #2985), Spark-compatible file naming,
    INSERT INTO ... SELECT visibility ([Native Writer] INSERT INTO ... SELECT fails due to stale catalog cache after write #3521), dynamic-overwrite fallback, the
    maxRecordsPerFile fallback (both the write option and the conf, verifying Spark's
    writer 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 URI
    escaping and runs on every version.
  • CometParquetWriterSuite on Spark 3.4 (33/33 + 9 skipped) and 3.5 (34/34 + 8
    skipped)
    : 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 now
    picks the version-appropriate opt-in key, so it genuinely exercises the native path on
    both.
  • Regression sweep on 4.1: CometExecSuite (144), CometFallbackInvarianceSuite (6),
    CometPublicApiSuite (1).
  • Native: new parquet_writer unit test asserting the writer uses a
    commit-protocol-chosen path verbatim (and that a non-zero partition id does not leak
    into the name). cargo test -p datafusion-comet parquet_writer and
    cargo clippy --all-targets --workspace -- -D warnings pass.
  • Compiles and test-compiles against Spark 3.4, 3.5, 4.0, 4.1 and 4.2.

Known limitation

WriteTaskStatsTracker.newRow(filePath, row) is a per-row callback. Comet has columnar
batches, so rather than materializing every row just to hand it straight back,
recordRows passes InternalRow.empty and feeds only the count. That is exactly right
for BasicWriteTaskStatsTracker, which ignores the row argument, but a third-party
tracker 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 statsTrackers only 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), INT96
timestamps (#3425: spark.sql.parquet.outputTimestampType defaults to INT96 and we
write INT64 micros), and the four footer metadata keys (#3427: legacyINT96 and
timeZone drive rebase decisions on read, so omitting them is a correctness risk).
Then partitioned (#3193) → bucketed (#3194) → object stores.

…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))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@rich7420

rich7420 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

@andygrove thanks for the patch!

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@andygrove

Copy link
Copy Markdown
Member Author

Path escaping is fixed in d36ad9f, in both serdes — the 3.x one had the same URI.create call, where the enclosing catch turned it into a silent fallback instead of a query failure. The new test isn't version-gated and I confirmed it fails both ways without the fix.

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 recordRows, agreed there's no measured number here and I'm not claiming one. Speculative concurrent attempts are still untested — the attempt path is Spark's own newTaskTempFile, but I'd rather say that's unverified than imply otherwise.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot added enhancement New feature or request area:writer Native Parquet writer labels Sep 9, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
@andygrove

Copy link
Copy Markdown
Member Author

You were right, and the reason the first guard missed it is worth writing down: percent_encoding's should_percent_encode is !byte.is_ascii() || set.contains(byte), so non-ASCII is escaped regardless of which encode set is in play, while java.net.URI treats non-ASCII path characters as legal and leaves them alone. The raw/decoded comparison can only ever see what Java itself escaped. Fixed in 39e02de.

Rather than special-case non-ASCII, I derived the whole set the native parser rewrites from the locked url 2.5 crate instead of reasoning about it. A throwaway test parsed hdfs://ns/pre<c>post/output for every printable ASCII c and compared url.path() with the input:

KEPT     : !$%&'()*+,-.0123456789:;=@A-Z[\]^_a-z|~
ESCAPED  :  "#<>?`{}
NONASCII "café"   -> "/caf%C3%A9/output"
NONASCII "日本語"  -> "/%E6%97%A5%E6%9C%AC%E8%AA%9E/output"
NONASCII "🙂"     -> "/%F0%9F%99%82/output"
NONASCII "e\u{301}" -> "/e%CC%81/output"

So nine ASCII characters plus every non-ASCII byte, and notably %, [, \, ], ^ and | survive. The guard now declines anything in that set, on the string actually handed to the native writer, which is outputPath itself rather than the URI-decoded form. Deriving the set this way is also what keeps it from over-declining: gating on "not plain ASCII alphanumerics" would have refused dt=2026-09-09/hour=17 and Spark's own _temporary/0/_temporary/attempt_.../part-0.parquet, both of which are now controls in the test.

I kept the Java comparison as a second condition rather than replacing it. The native parser leaves % alone, so 50%off never gets rewritten; it reaches Path::parse as an invalid escape instead, and only Java's raw/decoded difference sees it. The comment says that now, and no longer claims the raw/decoded comparison detects the divergence on its own, which was the overstatement you flagged.

Tests cover accented Latin both precomposed (U+00E9) and as e plus combining acute (U+0301), CJK, an astral-plane emoji, non-ASCII in a nested segment rather than the leaf, and the remaining escaped ASCII characters. They are built from code points via Character.toChars, because scalastyle's NonASCIICharacterChecker rejects non-ASCII source characters, which is also why the scaladoc refers to U+00E9 rather than spelling it.

I checked the new condition is load-bearing rather than assuming it. Disabling only nativeEscaped and leaving the tests in place:

- HDFS output paths needing URI escaping are declined at planning *** FAILED ***
  escapedHdfsDestination(path).isDefined was false
  expected hdfs://ns/café/output.parquet to be declined

With it enabled, all 43 CometParquetWriterSuite tests pass locally on the default profile, which matches the count from the Spark 4.0/4.1/4.2 jobs. spotless:apply and scalastyle are clean.

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 libcomet in spark/target/classes left by another branch's build. Maven skips the copy when the destination is newer, so the suite silently ran against a library without this branch's parquet_writer.rs and proto changes. Removing it and rebuilding gave the clean run above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:writer Native Parquet writer enhancement New feature or request

Projects

None yet

3 participants