andygrove commented on issue #4180: URL: https://github.com/apache/datafusion-comet/issues/4180#issuecomment-5152048117
## Re-audit against current `apache/main` The [earlier audit](https://github.com/apache/datafusion-comet/issues/4180#issuecomment-5063793313) ran against `0761e549a`. Main has moved since then, so I re-ran it against `ba21f02de` (default Maven profile, Spark 4.1.2, macOS aarch64, debug build) and widened the sweep from the inventory in the description to the whole `spark.sql.legacy.*` family. Scope this time: all 104 `spark.sql.legacy.*` keys declared in Spark 4.1.1's `SQLConf` were triaged, and every one that can reach a code path Comet executes was tested differentially at both values, plus the rebase-mode family and the list in #5087. Method unchanged: run the same query with `spark.comet.enabled` false and then true under the config, over Parquet-backed columns rather than literals, and compare results, exceptions and plans. ### Result: two categories still diverge, and both already have open issues | Category | Configs | Evidence on `ba21f02de` | Issue | |---|---|---|---| | Parquet legacy-calendar rebase | `spark.sql.parquet.datetimeRebaseModeInRead`, `spark.sql.parquet.int96RebaseModeInRead`, and their `spark.sql.legacy.parquet.*` aliases | `1000-01-01` reads back as `1000-01-06`; `count(*) ... WHERE d = date'1000-01-01'` returns 0 against Spark's 1. Identical in **all three** read-mode values and at defaults, so the config is simply not consulted. INT96 behaves the same | #5010 | | Negative-scale decimal | `spark.sql.legacy.allowNegativeScaleOfDecimal` | Native panic, still reproducing on four paths: cast into a negative-scale decimal and decimal-to-string (`core/src/num/mod.rs:475`, multiply overflow), decimal arithmetic (`planner.rs:917`, subtract overflow), and the same cast inside a `Filter` | #5013 | Since both categories are already filed and assigned, filing per-config issues would only create duplicates. One genuinely separate item was carved out of #5010 instead: * **#5195**: `spark.comet.exceptionOnDatetimeRebase` is dead code. It is declared, non-internal, published to the configs page, and read nowhere. Its intended destination (`SparkParquetOptions::use_legacy_date_timestamp_or_ntz`) is only ever initialized to `false` and never consulted. A user who sets it precisely to be protected from the limitation above gets silence and wrong results. It has a self-contained fix that does not depend on rebase support landing, which is why it is now its own issue. ### Everything else matched No divergence was found for any of the following. Each was run at both values with Comet enabled and disabled. **Verified respected by design** (Comet reads the config or the serde routes around it): `castComplexTypesToString.enabled`, `negativeIndexInArrayInsert`, `nullInEmptyListBehavior`, `truncateForEmptyRegexSplit`, `sizeOfNull`, `parquet.returnNullStructIfAllFieldsMissing`. **Verified matching, not read by Comet** (no reader in the tree, but parity holds because the affected path is Spark's or the values coincide): `timeParserPolicy` at `LEGACY` and `CORRECTED` across `to_date`, `to_timestamp`, `unix_timestamp`, `date_format` and `cast(string as date)`; `followThreeValuedLogicInArrayExists`; `lpadRpadAlwaysReturnString`; `allowHashOnMapType` (both `hash` and `xxhash64` over maps); `collationAwareHashFunctions` (hash, group-by and shuffle join on a `UTF8_LCASE` column); `disableMapKeyNormalization` (`map`, `map_from_arrays` and `str_to_map`, with genuine `-0.0` and `NaN` keys read from Parquet); `allowNullComparisonResultInArraySort` (including a comparator that returns `NULL`); `javaCharsets`; `codingErrorAction`; `allowZeroIndexInFormatString`; `raiseErrorWithoutErrorClass`; `json.allowEmptyString.enabled`; `json.enableDateTimeParsingFallback`; `csv.enableDateTimeParsingFallback`; `nullValueWrittenAsQuotedEmptyStringCsv`; `charVarcharAsString`; `noCharPaddingInPredicate`; `readSideCharPadding`; `integerGroupingId`; `groupingIdWithAppendedUserGroupBy`; `statisticalAggregate`; `percentileDiscCalculation`; `histogramNumericPropagateInputType`; `decimal.retainFractionDigitsOnTruncate`; `literal.pickMinimumPrecision`; `bucketedTableScan.outputOrdering`; `exponentLiteralAsDecimal.enabled`; `interval.enabled`; `fromDayTimeString.enabled`; `viewSchemaCompensation`; `storeAnalyzedPlanForView`; `useCurrentConfigsForView`; `readFileSourceTableCacheIgnoreOptions`; `doLooseUpcast`; `duplicateBetweenInput`; `inSubqueryNullability`; `scalarSubqueryCountBugBehavior`; `setopsPrecedence.enabled`; `allowParameterlessCount`; `typeCoercion.datetimeToString.enabled`; `createEmptyCollectionUsingStringType`; `compareDateTimestampInTimestamp`. The per-config verdicts for the #5087 subset are posted on that issue. ### Two limits on the above, stated plainly * `spark.sql.legacy.parquet.nanosAsLong` could not be tested this way. Spark cannot write nanosecond-precision Parquet, so producing an input requires a non-Spark writer. Comet has no reader for the config, so it is unverified rather than verified-safe. * My "did Comet actually execute this" probe walks `executedPlan` and does not descend into AQE query stages. For aggregate and subquery shapes it under-reports Comet operators. The parity verdicts are unaffected, but for those shapes I can confirm Spark and Comet agree without confirming which engine ran the expression. Anything reported above as respected-by-design was confirmed on a non-AQE plan with the Comet operator visible. ### What remains open on this issue Unchanged from the previous audit, and still the substantive work: this comment provides decisions, not tests. Nothing under `spark/src/test` exercises the `spark.sql.legacy.*` family, so every verdict above is a point-in-time observation that no regression test defends. That matters most for the configs in the second list. `timeParserPolicy` holds 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 in scope for #4936 and #4942, would silently break the config with no test to catch it. Linked issues: #5010, #5013, #5195, #5087, and #4799 for the per-expression and per-scan handling. -- 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]
