andygrove commented on PR #6355: URL: https://github.com/apache/datafusion-comet/pull/6355#issuecomment-6134071631
Thanks @comphead. Some notes on the S3A magic committer handling. Falling back is the right call, since `object_store` bypasses the S3A magic-path handling the committer depends on. I opened #6929 to track supporting it. 1. **Spark 4.1 turns the magic committer on by default.** When spark-hadoop-cloud is on the classpath, `SparkContext.enableMagicCommitterIfNeeded` (SPARK-47618) sets `fs.s3a.committer.name=magic`, `PathOutputCommitProtocol` and `BindingParquetOutputCommitter` unless they are already set. Earlier versions only do this when a bucket sets `fs.s3a.bucket.<bucket>.committer.magic.enabled=true`. So on 4.1 deployments that ship spark-hadoop-cloud, the native S3 path here will fall back unless the user opts out of magic. Worth saying in `operators.md`. 2. **The magic check misses the committer factory keys.** When the S3A committers are wired up (`PathOutputCommitProtocol` or `BindingParquetOutputCommitter`), `PathOutputCommitterFactory` picks the factory from `mapreduce.outputcommitter.factory.class` and then `mapreduce.outputcommitter.factory.scheme.s3a`. Naming `org.apache.hadoop.fs.s3a.commit.magic.MagicS3GuardCommitterFactory` in either one selects the magic committer without `fs.s3a.committer.name`. `unsupportedS3Destination` admits that write, then `checkNativeWriteDestination` fails every task on the `__magic_job-*` path, so the job fails where Spark's writer would succeed. Declining when either key names `MagicS3GuardCommitterFactory` would close the gap. 3. **No test writes through the magic committer.** Comet's test classpath only has spark-hadoop-cloud's `tests` jar, so `PathOutputCommitProtocol` can't load and Spark 4.1's default never applies in our tests. The MinIO test sets `fs.s3a.committer.name=magic` while Spark's default protocol still uses `FileOutputCommitter`, so it shows the planning check fires but not what happens on a real magic write. An end-to-end test needs the main jar in test scope. That turns on the 4.1 magic defaults for every test session unless the shared test config sets `spark.hadoop.fs.s3a.committer.magic.enabled=false`. 4. **On Spark 3.x no S3A committer is ever used.** `CometNativeWriteExec` hard-codes `SQLHadoopMapReduceCommitProtocol` and builds its Hadoop contexts with `Job.getInstance()`, which has neither the session's Hadoop settings nor an output path. Replaying those calls in a Spark 3.5.8 shell gives a `FileOutputCommitter` whose output path and work path are both null. `newTaskTempFile` then returns a path directly in the destination, so nothing is staged and no `_SUCCESS` is written (#3015). Admitting S3 on 3.x would silently ignore a configured `directory` or `partitioned` committer, so I'd keep native S3 writes to Spark 4.0+ in this PR. 5. **A direction question, since this is a draft.** Supporting the magic committer probably means letting S3A create the file. If we go that way for magic, it may be worth sending all S3A writes through S3A, which would also cover the encryption, ACL, storage class, content encoding and header settings this PR falls back on. That's a question for #6929 rather than a blocker here, but it may affect how far to take the `object_store` path. -- 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]
