jordepic commented on PR #5298: URL: https://github.com/apache/datafusion-comet/pull/5298#issuecomment-5241789778
@andygrove Thanks for the thorough review — every point is addressed in the current push. Point by point: 1. **PropertyKeys init / catch width**: the two strict rule vals are now `lazy`, so no `PropertyKeys` access happens during object initialization — every reflective constant lookup now occurs inside `getSupportLevel`'s try, and Scala lazy vals retry on failure so nothing gets poisoned. The catch is widened to `NonFatal`. 2. **Allowlist scope / fanout**: part 3 implements both clustered and fanout writer modes (mirroring `SparkWriteConf.useFanoutWriter`), so `write.spark.fanout.enabled` is declared supported rather than gated, and `write.target-file-size-bytes` rides the documented file-rolling divergence — both now have explicit rows in the doc table and tests. The "everything else must be absent" wording is reworded to state exactly which namespaces the allowlist enforces (`write.parquet.*`, `parquet.*`) and why other `write.*` keys are out of scope (plan-shaping, committer-side, non-parquet formats, or merge-on-read which the split plan never intercepts). 3. **Metrics modes**: inverted into a single allowlist rule — `full` and `truncate(N)` parse, anything else (including `truncat(16)` typos and future modes) falls back, with a test. The redundant `bloom-filter-max-bytes` and `stats-enabled.column.*` rules are gone; the property allowlist subsumes them. 4. **format-version**: the `None` case now falls back with "could not determine the table format-version". 5. **getOuterSparkWrite**: a missing `this$0` now returns `None`, so the reported reason is "could not unwrap SparkWrite". 6. **Compression**: moved to the accepted-divergences list (zstd default levels, LZ4 framing — byte-identical output isn't achievable even for a default table), and the "must match exactly" closing no longer claims compression. 7. **Registration**: the `CometExecRule` arm is now guarded by the config flag, so the default configuration tags nothing at all — no spurious fall-back reason on Comet's own operator. A test pins that no reason is recorded when the flag is off. I kept the wiring (rather than deferring to part 3) so the explain plumbing is exercised when users opt in. 8. **Test writes**: `captureWriteExec` is strict by default and fails the test if the INSERT throws; only the fixtures that deliberately write to unreachable locations or set broken classes (hdfs/s3/gs/oss/memory schemes, encryption, custom location provider / FileIO) tolerate failure. 9. **Coverage**: added tests for fanout, target-file-size, gs/oss/memory and explicit `file://` schemes, the disabled-flag behavior, a stubbed non-Iceberg `BatchWrite`, the metrics typo, and `write.parquet.shred-variants`. The "could not unwrap SparkWrite" branch isn't directly constructible (it needs a real `SparkWrite$` inner class without `this$0`), so it's covered only by the code path being fail-closed. Your version-drift concern was on the money: running the suite on all four profiles locally caught two real issues the 3.4/3.5-only verification missed, and both are what CI was failing on. Iceberg 1.11 on Spark 4.x unconditionally resolves `write.parquet.shred-variants=false` into every parquet write's `writeProperties` (now vetted, with `true` gated since shredding changes the parquet schema), and Hadoop 3.4's `core-default.xml` ships `io.compression.codec.zstd.level`, which made gating codec-level keys via the Hadoop conf a false positive on every Spark 4 write — those gates are removed and folded into the accepted compressed-bytes divergence (they can only shift compressed bytes). The suite (40 tests) is green locally on spark-3.4/1.5.2, 3.5/1.8.1, 4.0/1.10, and 4.1/1.11. Would you mind approving CI once more? The doc-link push reset the previous approval. -- 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]
