[MINOR][VL] Re-enable stale ignored atan2 test in MathFunctionsValidateSuite

[brijrajk]

What changes are proposed in this pull request?

The atan2 test in MathFunctionsValidateSuite was marked ignore in 2023 after a result mismatch — Velox's implementation was using std::atan2 directly, which handles -0 inputs differently from Spark's Math.atan2(y + 0.0, x + 0.0).

That was fixed upstream in oap-project/velox#263, which added sparksql::Atan2Function that normalises -0 inputs to match Spark's exact behaviour. The ignore annotation was never removed after the fix landed, leaving a valid test permanently skipped.

This PR re-enables the test by changing ignore to test.

Also promotes MathFunctionsValidateSuite from abstract class to class — a leftover from PR #11756 (RAS removal) which deleted the only concrete subclasses but missed this suite (the same fix was correctly applied to DateFunctionsValidateSuite in that PR). ANSI mode is disabled for Spark 4 compatibility, consistent with the approach taken in PR #12158 which adds further tests to this same suite.

How was this patch tested?

The re-enabled test runs SELECT atan2(double_field1, 0) from datatab limit 1 and verifies native execution via checkGlutenPlan[ProjectExecTransformer], comparing results against vanilla Spark.

Was this patch authored or co-authored using generative AI tooling?

Generated-by: Claude (Anthropic)

[brijrajk]

@philo-he — tagging you as you reviewed a related change in PR #12158 which also touches MathFunctionsValidateSuite.

This PR is independent of #12158 and can be merged directly without waiting for it. The three changes here (abstract→class, ANSI disable, atan2 un-ignore) are all self-contained correctness fixes with no external dependencies.

PR #12158 (currently draft, pending facebookincubator/velox#17648) will be rebased on top of this once it merges, so the duplicate abstract→class and ANSI fixes are resolved cleanly at that point.

[philo-he]

Thanks for the PR. Could you verify whether the following environment variable setting is sufficient to make ANSI default to false?

gluten/.github/workflows/velox_backend_x86.yml

Line 51 in ce6e16f

SPARK_ANSI_SQL_MODE: false

[brijrajk]

Thanks for digging into this. I investigated the full flow:

SPARK_ANSI_SQL_MODE: false is set as a GitHub Actions env variable in velox_backend_x86.yml but it never reaches Maven tests — the Spark 4.0 test jobs pass only -Dspark.test.home=... in DargLine, and the scalatest-maven-plugin config in pom.xml only forwards log4j.configurationFile as a system property. No Scala test code in the sparkConf chain reads the env var either.

So the env var is effectively documentation-only today, and spark.sql.ansi.enabled can only be controlled via the sparkConf override in test code.

The right fix would be to wire SPARK_ANSI_SQL_MODE into FunctionsValidateSuite.sparkConf:

val ansiEnabled = sys.env.getOrElse("SPARK_ANSI_SQL_MODE", "false")
super.sparkConf.set(SQLConf.ANSI_ENABLED.key, ansiEnabled)

This propagates to all subclasses (no per-suite overrides needed), while MathFunctionsValidateSuiteAnsiOn can still override it back to true for ANSI-specific tests.

Would it make sense to raise a separate issue for this infrastructure fix? Happy to take it on as a follow-up. For this PR, should I keep the per-suite override as a self-contained fix, or would you prefer I address the root cause here directly?

[philo-he]

@brijrajk, my understanding is that the following Spark code reads this environment variable to determine the default value of the ANSI config. It only influences the default — if the user or test code sets the ANSI config explicitly, that setting takes precedence. Could you double-check if we still need an explicit setting? Thank you.

https://github.com/apache/spark/blob/224231815105f6c0f333e866ef50c48810720f37/sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala#L5180

[brijrajk]

Oh you are right @philo-he, turns out till this time I was testing it in an unintended way. Thanks for clearing that up and pointing me to how the ANSI config is passed — I had only looked at the Maven/Java property passing side and missed that Spark reads SPARK_ANSI_SQL_MODE directly in SQLConf to set the default. I have tested it now with SPARK_ANSI_SQL_MODE=false and the atan2 test passes. Removed the explicit config set in the Scala file.

[philo-he]

LGTM. Could you confirm if the CI failure is related? Thanks.

[brijrajk]

@philo-he — yes, the CI failure is related to our change, though indirectly. Here's the full analysis:

Before this PR, MathFunctionsValidateSuite was abstract class, so no tests from this suite ever ran on any Spark version. Our promotion to class made all tests run for the first time — and on Spark 3.3, the test "decimal arithmetic respects allowPrecisionLoss captured at view analysis time" failed with a precision mismatch:

Expected: [0.10000000000, 2469135780246913578024690.14691357802, ...]
Actual:   [0.01000000000,  246913578024691357802469.01469135780, ...]

This test was added in PR #12110 as a regression test for GLUTEN-11917 — which fixed a Spark 4.1-specific behavior where DecimalArithmeticExpression captures allowPrecisionLoss in its evalContext at analysis time. The test is specifically designed to verify that behavior, but it was never guarded with testWithMinSparkVersion("...", "4.1"). Since the class was abstract, the missing guard went unnoticed.

The fix is straightforward — add the version guard so the test only runs on Spark 4.1+:

testWithMinSparkVersion("decimal arithmetic respects allowPrecisionLoss captured at view analysis time", "4.1") {

Honestly, your question about the CI failure sent me down quite a rabbit hole — but a worthwhile one! It turned out our change was exposing a pre-existing gap in test scoping from PR #12110.

Would it make sense to file a separate issue for the missing version guard and reference it here with a Fixes link? Or is this small enough to just fix inline without a dedicated issue?

[philo-he]

Would it make sense to file a separate issue for the missing version guard and reference it here with a Fixes link? Or is this small enough to just fix inline without a dedicated issue?

@brijrajk, If it's just a one-line change, fixing it here seems fine. Thanks.

[brijrajk]

Thanks @philo-he — agreed, fixing it inline is the right call. Fixed by wrapping the testWithMinSparkVersion call across multiple lines to stay within the 100-character limit. Both this PR and #12158 have been rebased onto current main and force-pushed.

[philo-he]

LGTM. Thanks.