Skip to content

perf: Optimize scalar fast path of atan2#20336

Open
kumarUjjawal wants to merge 1 commit intoapache:mainfrom
kumarUjjawal:perf/atan2
Open

perf: Optimize scalar fast path of atan2#20336
kumarUjjawal wants to merge 1 commit intoapache:mainfrom
kumarUjjawal:perf/atan2

Conversation

@kumarUjjawal
Copy link
Contributor

@kumarUjjawal kumarUjjawal commented Feb 13, 2026

Which issue does this PR close?

Rationale for this change

atan2 scalar calls were still going through array conversion in the math binary UDF macro, adding unnecessary overhead for constant folding and scalar evaluation. This PR adds a direct scalar fast path so scalar inputs no longer pay that cost

What changes are included in this PR?

  • Add a scalar fast path to the binary math UDF macro (used by atan2)
  • Add a new atan2 benchmark
Type Before After Speedup
atan2_f32_scalar 233.71 ns 53.39 ns 4.4x
atan2_f64_scalar 225.02 ns 56.57 ns 4.0x

Note: This change targets the make_math_binary_udf macro, which is currently only used by atan2, so the impact in this PR is limited to atan2 only.

Are these changes tested?

Yes

Are there any user-facing changes?

No

@github-actions github-actions bot added the functions Changes to functions implementation label Feb 13, 2026
Copy link
Contributor

@Jefffrey Jefffrey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's worth adding a note in the PR title/body for what other math UDFs this change would impact

ColumnarValue::Scalar(y_scalar),
ColumnarValue::Scalar(x_scalar),
) => {
if y_scalar.is_null() || x_scalar.is_null() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be a bit cleaner to move this into the match below, since we're already matching on (y_scalar, x_scalar).

|y, x| f64::$BINARY_FUNC(y, x),
)?;
Arc::new(result) as _
let return_type = args.return_type().clone();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we refactor this to avoid doing the clone() unconditionally, even for non-NULL inputs?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

functions Changes to functions implementation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants