fix: guard against silent fail_on_error loss in scalar wiring (#5074) - #5359
fix: guard against silent fail_on_error loss in scalar wiring (#5074)#5359sam-1112 wants to merge 2 commits into
Conversation
…#5074) Fail closed when fail_on_error=true reaches a registry-only UDF, reject ANSI-sensitive expressions from plain CometScalarFunction, and document the name-based ANSI/try wiring constraint.
sunchao
left a comment
sunchao
left a comment
There was a problem hiding this comment.
Three verified P1 issues: mandatory CI suite registration, Scala 2.12 source compatibility, and existing Spark 4.1+ make_time native planning.
| copy(child = newChildren.head) | ||
| } | ||
|
|
||
| class CometScalarFunctionSuite extends CometTestBase { |
There was a problem hiding this comment.
[P1] Register the new suite in both CI workflow matrices
CometScalarFunctionSuite is not listed in either .github/workflows/pr_build_linux.yml or .github/workflows/pr_build_macos.yml. The mandatory preflight runs python3 dev/ci/check-suites.py, which requires every *Suite.scala in both workflows; on this head it exits 255 with Suite not found in workflow .github/workflows/pr_build_linux.yml: org.apache.comet.serde.CometScalarFunctionSuite. Please add the suite to the expressions bucket in both workflows; otherwise all downstream CI jobs are blocked.
There was a problem hiding this comment.
Fixed. I registered org.apache.comet.serde.CometScalarFunctionSuite in the expressions bucket of both .github/workflows/pr_build_linux.yml and .github/workflows/pr_build_macos.yml.
Verified locally with python3.12 dev/ci/check-suites.py; the suite is now found in both workflow matrices.
| private[serde] def isAnsiSensitive(expr: Expression): Boolean = { | ||
| expr match { | ||
| case p: Product => | ||
| p.productElementNames.exists(AnsiSensitiveFields.contains) |
There was a problem hiding this comment.
[P1] Keep ANSI-field detection compatible with Scala 2.12
scala.Product.productElementNames exists in Scala 2.13 but not Scala 2.12. The supported Spark 3.4 and 3.5 profiles include Scala 2.12, so their lint/build matrix entries cannot compile this source: value productElementNames is not a member of Product. I reproduced the failure with Scala 2.12.18 and confirmed the same expression compiles with Scala 2.13.16. Please use a Scala-2.12-compatible way to inspect these fields, or introduce version-specific shims.
There was a problem hiding this comment.
Fixed. I removed Product.productElementNames and now detect ANSI-sensitive fields with Java reflection (Class.getDeclaredFields, walking superclasses). That compiles on both Scala 2.12 and 2.13. Verified with ./mvnw -pl spark -am compile -DskipTests -Pspark-3.4 using JDK 11.
| make_comet_scalar_udf!("levenshtein", func, without data_type) | ||
| } | ||
| // Registry UDFs (including datafusion-spark) cannot receive fail_on_error. | ||
| _ if fail_on_error => Err(DataFusionError::Execution(format!( |
There was a problem hiding this comment.
[P1] Preserve existing make_time registry dispatch before rejecting fail_on_error=true
The Spark 4.1+ shim already calls scalarFunctionExprToProtoWithReturnType("make_time", s.dataType, true, ...), but SparkMakeTime is registered only in all_scalar_functions() and has no dedicated match arm. This branch therefore rejects every nonconstant make_time query during native planning, including valid inputs. SparkMakeTime already implements Spark's always-throw semantics correctly, so please add an explicit "make_time" match arm or a narrowly safe exemption, and cover create_comet_physical_fun("make_time", ..., Some(true)) in the regression test.
There was a problem hiding this comment.
Fixed. I added an explicit "make_time" match arm before the registry fail-closed branch. SparkMakeTime already implements always-throw semantics, so the arm accepts fail_on_error=true without taking the flag as a constructor argument.
I also added regression coverage for create_comet_physical_fun("make_time", ..., Some(true)). Verified with cargo test -p datafusion-comet-spark-expr --test test_udf_registration; all 5 tests pass.
|
|
||
| /** Product field names that indicate ANSI / eval-mode sensitive Spark expressions. */ | ||
| private val AnsiSensitiveFields: Set[String] = | ||
| Set("failOnError", "evalMode", "nullOnOverflow") |
There was a problem hiding this comment.
[P2] Recognize ansiEnabled and Spark 4.1+ evalContext
Could we include ansiEnabled and evalContext in AnsiSensitiveFields? Spark Round, BRound, and Conv store ANSI behavior in ansiEnabled, while Spark 4.1+ Add, Subtract, Multiply, Divide, and related arithmetic expressions store NumericEvalContext as evalContext and expose evalMode only through an inherited method. Neither marker is present in this set, so both isAnsiSensitive(expr) and isAnsiSensitive(clazz) return false. A plain CometScalarFunction registration can therefore silently serialize fail_on_error=false and still pass the registration audit. The current handlers for these expressions are specialized, so this is a gap in the proposed safeguard rather than a regression in existing registrations. Could we recognize both fields and add regression coverage using real Spark Round and Spark 4.1+ Add expressions?
There was a problem hiding this comment.
Yeah, good catch. Both fields are in AnsiSensitiveFields now. Round / BRound / Conv keep ANSI in ansiEnabled, and Spark 4.1+ arithmetic uses evalContext instead of an evalMode field, so the old set would miss them and the audit would still pass. Coverage is a real Spark Round in CometScalarFunctionSuite, and a real Spark 4.1+ Add(..., NumericEvalContext(...)) in CometDecimalArithmeticViewSuite (that constructor only exists on 4.1+).
Register CometScalarFunctionSuite in CI, use Scala 2.12-compatible ANSI field detection, keep make_time dispatch for fail_on_error=true, and recognize ansiEnabled/evalContext.
03f1f93 to
5bbdf7b
Compare
2 participants
Which issue does this PR close?
Closes #5074
Rationale for this change
Registry-resolved UDFs (including datafusion-spark) previously ignored
fail_on_error=true, which could silently change ANSI semantics. PlainCometScalarFunctionalso always serializedfail_on_error=false, so ANSI-sensitive Spark expressions could be miswired without failing at planning time.What changes are included in this PR?
create_comet_physical_fun_with_eval_modefails closed whenfail_on_error=truewould fall through to registry lookup.CometScalarFunction.convertrejects expressions withfailOnError,evalMode, ornullOnOverflowfields; addsisAnsiSensitivehelpers for registration audits.fail_on_errorwiring constraints in the contributor guide.test_udf_registration; ScalaCometScalarFunctionSuitefor serde guards and audit.How are these changes tested?
cargo test -p datafusion-comet-spark-expr --test test_udf_registration -- --nocapture./mvnw test -Dtest=none -Dsuites="org.apache.comet.serde.CometScalarFunctionSuite" -Pspark-4.1