andygrove commented on issue #4180: URL: https://github.com/apache/datafusion-comet/issues/4180#issuecomment-5063793313
## Audit results I worked through the config inventory above against `apache/main` @ `0761e549a` (default Maven profile, Spark 4.1.2, macOS aarch64, debug build). Method was a differential harness rather than code reading: each case runs the same query with `spark.comet.enabled` false and then true under the config, and records the result, the exception, and the executed plan. Two caveats on the method, because both bit me on the first pass and would bite anyone repeating this: * Queries over literals get constant-folded, so Comet never executes and the case passes vacuously. About thirty cases in my first round were silently meaningless until I moved everything onto Parquet-backed columns and started asserting that Comet operators actually appear in the plan. * Binary columns compared through `Row.toString` produce `[B@<identity hash>`, so `binaryAsString`, `concatBinaryAsString` and `eltOutputAsString` all looked like mismatches until I compared them via `hex()`. They are fine. ### Tier 1 | Config | Verdict | |---|---| | `spark.sql.parquet.datetimeRebaseModeInRead` | **Divergence.** #5010 | | `spark.sql.parquet.int96RebaseModeInRead` | **Divergence.** Same root cause, folded into #5010 (#5011 closed as duplicate) | | `spark.sql.parquet.datetimeRebaseModeInWrite`, `int96RebaseModeInWrite` | Not applicable in the default configuration: `spark.comet.parquet.write.enabled` defaults to `false` and is marked experimental | | `spark.sql.legacy.timeParserPolicy` | Respected, but incidentally. See the note below | | `spark.sql.parquet.binaryAsString` | Respected. Verified on projection, `\|\|`, `length` and `upper` | ### Tier 2 | Config | Verdict | |---|---| | `spark.sql.legacy.sizeOfNull` | Respected by design. `CometSize` reads `expr.legacySizeOfNull` and emits the matching `CaseWhen` else-branch | | `spark.sql.legacy.allowNegativeScaleOfDecimal` | **Divergence.** #5013, a native panic | | `spark.sql.legacy.typeCoercion.datetimeToString.enabled` | Safe to ignore. Resolved in the analyzer before Comet sees the plan | | `spark.sql.legacy.allowCastNumericToTimestamp` | Safe to ignore. Analyzer-side | | `spark.sql.legacy.followThreeValuedLogicInArrayExists` | Respected, but incidentally | | `spark.sql.function.concatBinaryAsString` | Safe to ignore. Analyzer-side coercion | | `spark.sql.function.eltOutputAsString` | Safe to ignore. Analyzer-side coercion | | `spark.sql.legacy.integerGroupingIdEnabled` | Respected. Verified through `CometExpand` + `CometHashAggregate` with `CUBE` | | `spark.sql.legacy.json.allowEmptyString.enabled` | Respected | ### Tier 3 | Config | Verdict | |---|---| | `spark.sql.legacy.charVarcharAsString` | Respected. Verified both write-side and read-side, with padding-sensitive values and a `c = 'ab'` predicate | | `spark.sql.readSideCharPadding` | Respected | | `spark.sql.legacy.createEmptyCollectionUsingStringType` | Safe to ignore. Analyzer-side | ### Tier 4 | Config | Verdict | |---|---| | `spark.sql.ansi.enabled` | **Divergence.** #5012. Everything else checked out: division and remainder by zero, int/long add/sub/mul overflow, unary minus, `abs`, array index and `element_at` out of bounds, map key not found, `cast` to every numeric target, decimal overflow, `add_months`, `make_date`, `conv`, `substring`, `round`, `date_add`, and `sum` overflow all raise correctly, and all the same expressions match with ANSI off | | `spark.sql.caseSensitiveAnalysis` | Respected, including nested struct field access in expressions | | `spark.sql.session.timeZone` | Respected. Verified across `date_format`, `hour`, `minute`, `date_trunc`, `from_utc_timestamp`, `to_utc_timestamp`, `cast(ts as date)`, `cast(ts as string)` and `unix_timestamp`, against two non-UTC zones | | `spark.sql.decimalOperationsAllowPrecisionLoss` | Respected for divide, multiply, add, `sum` and `avg`, under both values | ### Two things worth flagging beyond the pass/fail **"Respected, but incidentally" is not the same as respected.** `timeParserPolicy` is honoured only because `CometGetTimestamp` and `CometToUnixTimestamp` are `CometCodegenDispatch`, and `followThreeValuedLogicInArrayExists` only because `CometArrayExists` is. Nothing reads either config. Giving any of those expressions a native implementation, which is squarely in scope for #4936 and #4942, would silently break the config with no test to catch it. These are the cases where a regression test earns its keep. **ANSI error text.** Results are correct, but several messages are not: `bigint + bigint` overflow reports "integer overflow" where Spark says "long overflow", `abs` reports "Int32 overflow", several omit Spark's `try_add` / `try_multiply` suggestion, and decimal overflow surfaces as a bare `CometNativeException: Compute error: Arithmetic overflow` instead of `NUMERIC_VALUE_OUT_OF_RANGE`. Related to #4967. Not filed separately since these are error fidelity rather than correctness, but they would fail Spark SQL tests that match on message text. ## What is still outstanding The process in the description asks for "an explicit decision, a test, and documentation" per config. This comment provides the decision. The tests do not exist: my harness was a throwaway diagnostic and nothing under `spark/src/test` currently exercises `spark.sql.legacy.*` or the rebase configs. Given the "incidentally respected" cases above, that gap is the substantive remaining work, so I would suggest keeping this issue open until: 1. #4799 lands, covering the session-wide fallback for legacy configs Comet cannot honour per-expression; 2. a follow-up PR adds a permanent differential suite for the configs above that Comet does honour natively, asserting both parity and that Comet actually executed, plus the per-config decision table in the compatibility guide. The optional `spark.comet.strictConfigCompatibility` whitelist gate from the description is largely subsumed by #4799's `spark.comet.legacyConfFallback.enabled`. -- 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]
