andygrove commented on PR #5293:
URL:
https://github.com/apache/datafusion-comet/pull/5293#issuecomment-5571927959
Closing in favour of #5763, which makes this change purely additive.
@comphead's concern was the one that mattered: this PR deleted the Spark 3.x
native
writer, so native writes became 4.0+ only and writer testing on Spark 3 (the
`3.5`
option in the manual `Spark SQL Writer Tests` workflow) would have had
nothing to test
until 3.4/3.5 support is dropped. Rather than park the change until then,
#5763 keeps
`CometDataWritingCommand` and `CometNativeWriteExec` exactly as they are on
`main` and
adds the `WriteFilesExec` seam alongside them. `CometExecRule` picks the
path by Spark
version and the two never both fire. Nothing is deleted, and the 3.x path
goes away
with 3.x support rather than ahead of it.
Every review item here is carried over:
- **P1, no test for the task failure / commit abort path** — carried over:
`a failing
task aborts, cleans up its staging file, and the retry succeeds`, using a
commit
protocol that fails `commitTask` on demand so the staging file already
exists and the
cleanup is observable.
- **P2, zero-partition RDD and SPARK-23271** — carried over: the dummy
single-partition
swap in `doExecuteWrite`, plus the `empty input still writes a schema-only
file` test.
- **P2, `maxRecordsPerFile` silently ignored** — carried over:
`rollsFilesByRecordCount`
declines the write, with a test covering both the write option and the
conf and
asserting Spark's writer rolls the expected 10 files.
- **Dynamic partition overwrite** — carried over: the fallback test that
asserts the
untouched partitions survive.
- **@sunchao's P2 on the opt-in key** — fixed properly rather than only at
the
`ConfigEntry`. `CometConf.isOperatorAllowIncompat` now resolves the
deprecated
alternative, which is what the planner's by-name `SQLConf` lookup was
bypassing. The
regression test covers both halves you asked for: old key alone still
enables native
writes on Spark 4, and an explicitly set new key takes precedence.
- **@peterxcli on the `reserved` proto tags** — moot in #5763. Nothing is
removed from
`ParquetWriter`, so there is no `reserved` block to drop. `work_dir`
simply becomes
genuinely optional and acts as the discriminator: set (3.x) means the
native writer
derives the file name as before, unset (4.0+) means `output_path` is the
exact file
and is used verbatim. `output_path` was already unused on the 3.x native
path, so no
field changes meaning for an existing plan.
- **@peterxcli on git rename detection** — also moot, and this is the nicest
consequence
of the additive approach: the old and new files coexist, so the diff is
additions
rather than a pair of low-similarity renames that GitHub renders as
unrelated files.
One thing worth flagging for #5143: it adds an `elapsed_compute` metric to
`CometNativeWriteExec`. That file is untouched by #5763, so there is no
conflict there,
but whichever lands second should add the same metric to
`CometWriteFilesExec` or Spark
4 loses it. The `parquet_writer.rs` changes in the two PRs are in different
regions but
close enough that a rebase will want a look.
--
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]
