Skip to content

[GLUTEN-12157][VL] Register sin, tan, tanh, radians in Velox sparksql function registry#12158

Open
brijrajk wants to merge 1 commit into
apache:mainfrom
brijrajk:feat/register-sparksql-math-functions
Open

[GLUTEN-12157][VL] Register sin, tan, tanh, radians in Velox sparksql function registry#12158
brijrajk wants to merge 1 commit into
apache:mainfrom
brijrajk:feat/register-sparksql-math-functions

Conversation

@brijrajk
Copy link
Copy Markdown

@brijrajk brijrajk commented May 27, 2026 •
edited
Loading

What changes are proposed in this pull request?

Five math scalar functions (sin, tan, tanh, radians) are mapped in
ExpressionMappings.scala to their Substrait counterparts but were missing from
Gluten's Velox C++ function registry (RegistrationAllFunctions.cc). Queries using
these functions silently fell back to vanilla Spark instead of running natively in Velox.

This PR registers them in registerFunctionOverwrite() using the existing Velox prestosql
implementations from velox/functions/prestosql/Arithmetic.h, which match Spark's expected
semantics for these functions.

Also fixes a leftover from PR #11756 (RAS removal): MathFunctionsValidateSuite and
ScalarFunctionsValidateSuite were left as abstract class after their only concrete
subclasses were deleted, causing all tests in those suites to silently not run. Both are
promoted to class here, consistent with the fix already applied to
DateFunctionsValidateSuite in that same PR.

Fixes #12157

How was this patch tested?

Added tests in MathFunctionsValidateSuite for each of the five functions using
runQueryAndCompare with checkGlutenPlan[ProjectExecTransformer] to verify native
execution in Velox without fallback.

MathFunctionsValidateSuite now explicitly disables ANSI mode (enabled by default in
Spark 4), which wraps arithmetic expressions in ANSI check nodes and shifts the top-level
plan node away from ProjectExecTransformer. ANSI-specific behavior is covered by the
existing MathFunctionsValidateSuiteAnsiOn.

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

Generated-by: Claude (Anthropic)

@github-actions github-actions Bot added the VELOX label May 27, 2026
@n0r0shi
Copy link
Copy Markdown

n0r0shi commented May 28, 2026 •
edited
Loading

Hi @brijrajk, apologies for the parallel work, I didn't see your PR was already up when I started. I had an upstream Velox fix in flight at the same time: facebookincubator/velox#17648.

Heads up on ln, independent of which path lands: prestosql::LnFunction calls std::log and returns -Infinity for ln(0) / NaN for ln(-1). Spark's UnaryLogExpression returns null in both cases (Hive heritage, check yAsymptote: in mathExpressions.scala).

So a change on Velox is indeed inevitable, the Velox PR adds a sparksql::LnFunction mirroring the existing sparksql::Log2Function for that reason. The other four(sin/tan/tanh/radians) are fine with the prestosql impls — no domain restriction, same as sinh/cos/cosh/ degrees already.

Your test-framework fixes (abstract class → class, ANSI disable) are independently valuable and should land regardless — those suites really are silently not running today.

Happy to coordinate whichever direction maintainers prefer.

@brijrajk brijrajk changed the title [GLUTEN-12157][VL] Register sin, tan, tanh, radians, ln in Velox sparksql function registry [GLUTEN-12157][VL] Register sin, tan, tanh, radians in Velox sparksql function registry May 28, 2026
@brijrajk
Copy link
Copy Markdown
Author

brijrajk commented May 28, 2026 •
edited
Loading

Hi @brijrajk, apologies for the parallel work, I didn't see your PR was already up when I started. I had an upstream Velox fix in flight at the same time: facebookincubator/velox#17648.

Heads up on ln, independent of which path lands: prestosql::LnFunction calls std::log and returns -Infinity for ln(0) / NaN for ln(-1). Spark's UnaryLogExpression returns null in both cases (Hive heritage, check yAsymptote: in mathExpressions.scala).

So a change on Velox is indeed inevitable, the Velox PR adds a sparksql::LnFunction mirroring the existing sparksql::Log2Function for that reason. The other four(sin/tan/tanh/radians) are fine with the prestosql impls — no domain restriction, same as sinh/cos/cosh/ degrees already.

Your test-framework fixes (abstract class → class, ANSI disable) are independently valuable and should land regardless — those suites really are silently not running today.

Happy to coordinate whichever direction maintainers prefer.

Thanks for the detailed explanation and for flagging the parallel work — no worries at all. You're right on ln; I've dropped it from this PR so only the four semantically safe functions (sin, tan, tanh, radians) land here, along with the test-framework fixes.

Once your Velox PR (facebookincubator/velox#17648) lands upstream and Gluten picks up the new sparksql::LnFunction, ln can follow in a separate PR.

@brijrajk brijrajk force-pushed the feat/register-sparksql-math-functions branch from 6270b01 to 9aa5e1b Compare May 28, 2026 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

VELOX

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[VL] Register missing math scalar functions in Velox sparksql registry (sin, tan, tanh, radians, ln)

2 participants

@brijrajk @n0r0shi