anshulsingh-py commented on PR #19853:
URL: https://github.com/apache/hudi/pull/19853#issuecomment-5615303860
CI caught two real Spark 4.2-specific bugs in the registry fallback
(`test-spark-java17-scala-other-tests`, scala-2.13/spark4.2 - looks like this
is the only lane that actually exercises this test file against 4.2, since the
3.5/4.0/4.1 variants of that job are commented out). Reproduced both locally
against real Spark 4.2.0 (`mvn -Pjava17 -Dscala-2.13 -Dspark4.2`) rather than
guessing from the CI log, since the original failure gave no stack trace
(caught by the existing `Try`/fallback) - added a temporary debug print to get
the real exception, then reverted it:
- `lookupBuiltin`'s `FunctionIdentifier(funcName)` asserted on 4.2: a
session-level function registry clone now requires builtins to be looked up by
the fully qualified `system.builtin.<name>` key, unlike the builtin singleton
itself which still auto-qualifies a bare name. Fixed with a version-gated
identifier - checked Spark's own `normalizeFuncName` source across
3.5.5/4.1.1/4.2.0 first, since always passing a 3-part identifier would instead
break lookups pre-4.2 (it keeps whatever database is passed in rather than
dropping it).
- `finalizeRegistryResolution`'s `RuntimeReplaceable` unwrap didn't handle a
`With(child, defs)` common-subexpression wrapper reachable through the unwrap
(`nullif` -> `NullIf` -> `With(...)`, since Spark 4.0) - `With` is
`Unevaluable` like any other holder, so it was being rejected outright instead
of inlined. `With`/`CommonExpressionDef`/`CommonExpressionRef` don't exist
before 4.0, so the inlining goes through reflection rather than a direct import
to keep this file compiling across the same 3.3-4.2 range as the rest of it.
Also folded in two smaller fixes from self-review while in there:
`isUsableOutsideQueryPlan`'s eval-safety probe was too broad and could catch a
genuine data error (`regexp_replace('a', '[', 'x')`) from an all-literal
foldable call and misreport it as unresolvable - narrowed to only treat a bare
`SparkException` (`Unevaluable.eval()`'s own marker) as the structural signal;
and `lookupBuiltin` now widens each argument before the lookup itself (`sqrt(ts
+ 1)` needed the same pre-lookup widening `nvl(ts, 0)` already had), not just
the call's result afterward.
Verified against real Spark 4.2.0/scala-2.13 locally: full `procedure`
package, 271/271 passing (matches the default-profile baseline exactly - 271
succeeded, 1 canceled, 2 ignored, both places). Rebased onto current master.
--
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]
