andygrove commented on code in PR #5763:
URL: https://github.com/apache/datafusion-comet/pull/5763#discussion_r3970126689
##########
spark/src/main/scala/org/apache/comet/serde/operator/CometDataWritingCommand.scala:
##########
@@ -129,8 +128,11 @@ object CometDataWritingCommand extends
CometOperatorSerde[DataWritingCommandExec
// Collect S3/cloud storage configurations
val session = op.session
val hadoopConf =
session.sessionState.newHadoopConfWithOptions(cmd.options)
+ // `outputPath` is `Path.toString`, which is not a valid URI string: it
leaves spaces and
+ // literal `%` unescaped, so `URI.create` would throw (and the catch
below would silently
+ // 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)
Review Comment:
Fixed in 4095ea6fd — 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.
--
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]
