andygrove commented on PR #4799:
URL:
https://github.com/apache/datafusion-comet/pull/4799#issuecomment-5049064757
Nice to see the redesign land. A couple of correctness questions before this
ships. (Review drafted with LLM assistance.)
**1. `timeParserPolicy=LEGACY` currently produces no fallback and no
per-expression check.**
The new test in `CometSparkSessionExtensionsSuite` asserts that
`spark.sql.legacy.timeParserPolicy=LEGACY` must NOT trigger the session-wide
fallback, on the stated reasoning that it's handled per-expression. But
grepping the JVM source shows no serde actually consults `timeParserPolicy`. So
under `timeParserPolicy=LEGACY`, Comet stays enabled and runs the native
datetime parsers with the default (CORRECTED) semantics, and results silently
diverge from Spark for any query that parses a date/timestamp string. The
comment on that test case says the same thing as the description, but neither
seems to match the code.
Either the flag needs to go into
`ShimLegacyConfFallback.legacyConfDefaults`, or the affected datetime
expressions need to gate on it (mirroring how `CometArrayInsert` and
`CometCast` do). Same question for
`spark.sql.legacy.castComplexTypesToString.enabled` when
`spark.comet.expr.Cast.allowIncompatible=true` — the native path runs but
doesn't honor the legacy formatting. That's within the documented
`Incompatible` contract, so it's arguably fine, just worth being intentional
about.
**2. Setting the primary parquet key instead of the legacy alias silently
bypasses the fallback.**
The fallback keys off
`conf.contains("spark.sql.legacy.parquet.datetimeRebaseModeInRead")` etc., but
Spark treats these as aliases for `spark.sql.parquet.datetimeRebaseModeInRead`.
A user who sets the primary (non-legacy) name to `LEGACY` gets zero fallback
signal, and the native Parquet reader doesn't honor it either — so the write in
the file gets applied as-is without rebasing. That's a real divergence for any
Parquet file written by Spark 2.x. Could the check either look at the primary
key (via the resolved `SQLConf` entry) or explicitly document that only the
legacy alias is monitored?
**3. Description bullet mentions `timeParserPolicy` in the curated
session-wide list.**
The description's session-wide bullet reads `parquet datetime/int96 rebase
modes, charVarcharAsString, timeParserPolicy, ...`, but `timeParserPolicy` is
deliberately NOT in `legacyConfDefaults`. Whichever way (1) is resolved, the
description should match.
--
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]
