fix: widen power(decimal, float) to Float64, fix bugs

[neilconway]

Which issue does this PR close?

• Closespower(decimal, float) should return float #22472.

Rationale for this change

This PR makes several improvements to the power UDF:

1. Previously, power(decimal, float) returned decimal, with the same precision as the first argument. This could result in silently truncating the result value and is inconsistent with the behavior of Postgres and DuckDB; for example, power(2.5::decimal(2, 1), 4.0) returned 39 instead of 39.0625. This PR changes power(decimal, float) to return a Float64 instead by removing the power(decimal, float) path entirely: type coercion will result in taking the (float, float) path.

2. simplify for power could sometimes have resulted in mismatches between the declared return type of the function and the simplified expression. Change this to insert casts instead.

3. Previously, power(decimal, int-array) converted its inputs to Float64, on the argument that this improved performance. Empirically, this does not seem to be the case (see benchmarks below), although perhaps it was true with older version of Arrow. Perhaps more importantly, converting exact numeric types to floating point is undesirable because it loses precision. The behavior here was also inconsistent with the behavior for power(decimal, int-scalar).

Benchmarks (Arm64):

case main (ns) branch (ns) Δ
---- --------- ----------- ---
array n=1024 exp=2 8546 4753 -44.3%
array n=1024 exp=4 8516 5839 -31.4%
array n=1024 exp=8 8458 6298 -25.5%
array n=8192 exp=2 65197 37159 -43.0%
array n=8192 exp=4 65136 44680 -31.4%
array n=8192 exp=8 65110 49479 -24.0%
scalar n=1024 exp=2 5281 5025 -4.8%
scalar n=1024 exp=4 6473 5972 -7.7%
scalar n=1024 exp=8 6700 6593 -1.6%
scalar n=8192 exp=2 40280 38481 -4.4%
scalar n=8192 exp=4 49497 45748 -7.5%
scalar n=8192 exp=8 51334 51450 +0.2%

What changes are included in this PR?

• Implement fixes/improvements described above.
• Various refactoring and code cleanup
• Add new benchmark
• Update SLT tests

Are these changes tested?

Yes; new tests added.

Are there any user-facing changes?

Yes; this commit changes the behavior of power with a decimal base. In most cases this is a clear improvement; one slight regression is that the decimal code path for power is more likely to needlessly overflow (#22480); I will fix that in a followup PR.

[Jefffrey]

cc @theirix since worked on the decimal part for this before

[Omega359]

# power scalar null result types
query TTT
select arrow_typeof(power(null, 64)),
arrow_typeof(power(null, null)),
arrow_typeof(power(2, null));
----
Float64 Float64 Float64

passes in main, fails in PR.

3. query result mismatch:
[SQL] select arrow_typeof(power(null, 64)),
arrow_typeof(power(null, null)),
arrow_typeof(power(2, null));
[Diff] (-expected|+actual)
- Float64 Float64 Float64
+ Null Null Float64
at /home/bruce/dev/datafusion/datafusion/sqllogictest/test_files/scalar.slt:870

[Omega359]

Please fix the slt tests that are failing - math.slt, expr.slt, push_down_filter_regression.slt

[neilconway]

@Omega359 thanks, fixed.

[theirix]

Thanks, this is a real regression. The main question is whether we should use a decimal or a float output type.
From a quick skim, analytical engines use float output types for the decimal inputs. The major exception is Postgres - it preserves decimal types, as we previously did for power. Also, with parse_float_as_decimal, we promote the second argument to decimal - would it work?

[neilconway]

The main question is whether we should use a decimal or a float output type.
From a quick skim, analytical engines use float output types for the decimal inputs. The major exception is Postgres - it preserves decimal types

numeric/decimal in Postgres is arbitrary precision and can represent exceptional values like NaN, so I think it's somewhat different (no risk of overflow, etc.)

We could follow DuckDB and others by always returning a float output type, but to me it seems unfortunate to lose exact precision for the power(decimal, integer) case.

Also, with parse_float_as_decimal, we promote the second argument to decimal - would it work?

AFAIK this PR should work fine in that scenario (I'm working on this in part because I want to flip parse_float_as_decimal to true by default in the future). But lmk if I misunderstood your question.

[theirix]

The main question is whether we should use a decimal or a float output type.
From a quick skim, analytical engines use float output types for the decimal inputs. The major exception is Postgres - it preserves decimal types

numeric/decimal in Postgres is arbitrary precision and can represent exceptional values like NaN, so I think it's somewhat different (no risk of overflow, etc.)

That's true indeed - Postgres is different even with this type semantics, let's stick with DuckDB/Spark.

We could follow DuckDB and others by always returning a float output type, but to me it seems unfortunate to lose exact precision for the power(decimal, integer) case.

I agree, let's leave (decimal+integer -> decimal) exact, if possible.

Also, with parse_float_as_decimal, we promote the second argument to decimal - would it work?

AFAIK this PR should work fine in that scenario (I'm working on this in part because I want to flip parse_float_as_decimal to true by default in the future). But lmk if I misunderstood your question.

Yes, I saw that ticket - thank you! My concern is that after the switch, both arguments become decimal values, so the floatness check on the exponent returns false. I remember I've tried to introduce some SLT test cases for both values of the flag, it could be useful to double-check.

[neilconway]

My concern is that after the switch, both arguments become decimal values, so the floatness check on the exponent returns false.

The function's signature doesn't allow it to be invoked with (Decimal, Decimal), so it will actually be invoked with (Decimal, Float64) -- so I think we're fine here.

[alamb]

👍

[alamb]

Thank yoy (again) @neilconway

[adriangb]

This seems ready so I sent it to merge @neilconway

[neilconway]

@theirix@Jefffrey@alamb@Omega359 Thanks for the reviews!

@adriangb Thanks!