sunchao commented on PR #5763: URL: https://github.com/apache/datafusion-comet/pull/5763#issuecomment-5621741874
**One confirmed P2 correctness issue remains. I would fix it before merging.** Reviewed head `39e02de3` against base `7f1e0018`. ### Finding: HDFS filename prefixes can cause silent data loss [CometWriteFilesExec.scala:210](https://github.com/apache/datafusion-comet/blob/39e02de39908ec4973e7a233cf6f202e86795b0e/spark/src/main/scala/org/apache/spark/sql/comet/CometWriteFilesExec.scala#L210) The new guard checks the output directory, but Spark’s generated filename also includes `mapreduce.output.basename`. With a basename such as `part?x` or `part#x`, native URL parsing discards everything after `?` or `#`, including Spark’s unique partition/job suffix. Each task therefore writes a file named `part`. During commit, those files overwrite each other. **I reproduced this with real Spark/native execution and a local HDFS cluster**, using 100 rows across two partitions: | Writer | Basename | Rows committed | Output | |---|---|---:|---| | Spark | `part?x` / `part#x` | 100 | Two data files | | Native | `part?x` / `part#x` | **50** | One file named `part` | | Native | `part` | 100 | Two data files | All writes returned successfully and created `_SUCCESS`. The affected plans contained `CometWriteFiles`. **Suggested fix:** extend admission checks to the effective filename prefix and protect the complete committer-returned path, or correct native Hadoop-path conversion. Add a multi-partition HDFS regression test. The older writer’s fixed filename generation did not expose this configuration. ### Other review areas - **Performance:** the per-row statistics callback adds work whose throughput impact remains unmeasured. No additional performance blocker confirmed. - **Design:** retaining Spark’s job commit, SaveMode handling, and catalog refresh is sound. - **Abstraction & complexity:** the version shims and separate task state are justified. No further actionable issue found. ### Validation - Local writer suite: **43/43 passed**. - Local task-metrics suite: **15/15 passed**. - Destination guard: **1,128,853 inputs checked**, with no mismatch among admitted paths. - [Current CI](https://github.com/apache/datafusion-comet/actions/runs/34399188780): **65 passed, nine skipped**. **Validation boundary:** local native compilation was blocked by the dependency mirror. JVM tests and HDFS reproduction used current-head JVM code with a checksum-verified CI native library from merge `d5bbfa54`. The writer source, writer protobuf definition, and relevant native planner section match the reviewed head. -- 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]
