Jefffrey commented on code in PR #18968:
URL: https://github.com/apache/datafusion/pull/18968#discussion_r2568294919
##########
datafusion/functions/src/math/power.rs:
##########
@@ -312,25 +294,28 @@ impl ScalarUDFImpl for PowerFunc {
/// 3. Power(a, Log(a, b)) ===> b
fn simplify(
&self,
- mut args: Vec<Expr>,
+ args: Vec<Expr>,
info: &dyn SimplifyInfo,
) -> Result<ExprSimplifyResult> {
- let exponent = args.pop().ok_or_else(|| {
- plan_datafusion_err!("Expected power to have 2 arguments, got 0")
- })?;
- let base = args.pop().ok_or_else(|| {
- plan_datafusion_err!("Expected power to have 2 arguments, got 1")
- })?;
-
+ let [base, exponent] = take_function_args("power", args)?;
+ let base_type = info.get_data_type(&base)?;
let exponent_type = info.get_data_type(&exponent)?;
+
+ // Null propagation
+ if base_type.is_null() || exponent_type.is_null() {
+ let return_type = self.return_type(&[base_type, exponent_type])?;
+ return Ok(ExprSimplifyResult::Simplified(lit(
+ ScalarValue::Null.cast_to(&return_type)?
+ )));
+ }
Review Comment:
Similar to what we do for log:
https://github.com/apache/datafusion/blob/c6f73636b771ddb827c2c0b4c5c690707867e9a0/datafusion/functions/src/math/log.rs#L278-L283
##########
datafusion/functions/src/math/power.rs:
##########
@@ -358,241 +343,6 @@ fn is_log(func: &ScalarUDF) -> bool {
#[cfg(test)]
mod tests {
use super::*;
- use arrow::array::{Array, Decimal128Array, Float64Array, Int64Array};
- use arrow::datatypes::{Field, DECIMAL128_MAX_SCALE};
- use arrow_buffer::NullBuffer;
- use datafusion_common::cast::{
- as_decimal128_array, as_float64_array, as_int64_array,
- };
- use datafusion_common::config::ConfigOptions;
- use std::sync::Arc;
-
- #[cfg(test)]
- #[ctor::ctor]
- fn init() {
- // Enable RUST_LOG logging configuration for test
- let _ = env_logger::try_init();
- }
-
- #[test]
- fn test_power_f64() {
Review Comment:
Moved these to SLTs
##########
datafusion/sqllogictest/test_files/scalar.slt:
##########
@@ -1775,7 +1775,7 @@ CREATE TABLE test(
(-14, -14, -14.5, -14.5),
(NULL, NULL, NULL, NULL);
-query IIRRIR rowsort
+query IRRRIR rowsort
Review Comment:
This is a minor difference; previously we were able to cast floats to
integer, but with the new signature we fallback to computing as float if either
base or exponent is float.
##########
datafusion/functions/src/math/power.rs:
##########
@@ -162,49 +182,20 @@ impl ScalarUDFImpl for PowerFunc {
}
fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> {
- Ok(arg_types[0].clone())
+ if arg_types[0].is_null() {
+ Ok(DataType::Int64)
Review Comment:
Maintaining existing behaviour for null types
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
