Skip to content

move Log2, Log10, Ln to datafusion-functions #9869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
alamb merged 4 commits into from
Mar 30, 2024
Merged

move Log2, Log10, Ln to datafusion-functions #9869

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5483fbf
move log2
tinfoil-knight Mar 29, 2024
ca70fb6
move log10, ln
tinfoil-knight Mar 29, 2024
862d663
refactor log_b functions to use macro
tinfoil-knight Mar 29, 2024
99f81aa
update proto
tinfoil-knight Mar 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 0 additions & 21 deletions datafusion/expr/src/built_in_function.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,14 +71,8 @@ pub enum BuiltinScalarFunction {
Lcm,
/// iszero
Iszero,
/// ln, Natural logarithm
Ln,
/// log, same as log10
Log,
/// log10
Log10,
/// log2
Log2,
/// nanvl
Nanvl,
/// pi
Expand Down Expand Up @@ -187,10 +181,7 @@ impl BuiltinScalarFunction {
BuiltinScalarFunction::Gcd => Volatility::Immutable,
BuiltinScalarFunction::Iszero => Volatility::Immutable,
BuiltinScalarFunction::Lcm => Volatility::Immutable,
BuiltinScalarFunction::Ln => Volatility::Immutable,
BuiltinScalarFunction::Log => Volatility::Immutable,
BuiltinScalarFunction::Log10 => Volatility::Immutable,
BuiltinScalarFunction::Log2 => Volatility::Immutable,
BuiltinScalarFunction::Nanvl => Volatility::Immutable,
BuiltinScalarFunction::Pi => Volatility::Immutable,
BuiltinScalarFunction::Power => Volatility::Immutable,
Expand Down Expand Up @@ -292,9 +283,6 @@ impl BuiltinScalarFunction {
| BuiltinScalarFunction::Degrees
| BuiltinScalarFunction::Exp
| BuiltinScalarFunction::Floor
| BuiltinScalarFunction::Ln
| BuiltinScalarFunction::Log10
| BuiltinScalarFunction::Log2
| BuiltinScalarFunction::Radians
| BuiltinScalarFunction::Round
| BuiltinScalarFunction::Signum
Expand Down Expand Up @@ -412,9 +400,6 @@ impl BuiltinScalarFunction {
| BuiltinScalarFunction::Degrees
| BuiltinScalarFunction::Exp
| BuiltinScalarFunction::Floor
| BuiltinScalarFunction::Ln
| BuiltinScalarFunction::Log10
| BuiltinScalarFunction::Log2
| BuiltinScalarFunction::Radians
| BuiltinScalarFunction::Signum
| BuiltinScalarFunction::Sin
Expand Down Expand Up @@ -450,9 +435,6 @@ impl BuiltinScalarFunction {
| BuiltinScalarFunction::Exp
| BuiltinScalarFunction::Factorial
| BuiltinScalarFunction::Floor
| BuiltinScalarFunction::Ln
| BuiltinScalarFunction::Log10
| BuiltinScalarFunction::Log2
| BuiltinScalarFunction::Radians
| BuiltinScalarFunction::Round
| BuiltinScalarFunction::Signum
Expand Down Expand Up @@ -490,10 +472,7 @@ impl BuiltinScalarFunction {
BuiltinScalarFunction::Gcd => &["gcd"],
BuiltinScalarFunction::Iszero => &["iszero"],
BuiltinScalarFunction::Lcm => &["lcm"],
BuiltinScalarFunction::Ln => &["ln"],
BuiltinScalarFunction::Log => &["log"],
BuiltinScalarFunction::Log10 => &["log10"],
BuiltinScalarFunction::Log2 => &["log2"],
BuiltinScalarFunction::Nanvl => &["nanvl"],
BuiltinScalarFunction::Pi => &["pi"],
BuiltinScalarFunction::Power => &["power", "pow"],
Expand Down
6 changes: 0 additions & 6 deletions datafusion/expr/src/expr_fn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -570,9 +570,6 @@ scalar_expr!(Signum, signum, num, "sign of the argument (-1, 0, +1) ");
scalar_expr!(Exp, exp, num, "exponential");
scalar_expr!(Gcd, gcd, arg_1 arg_2, "greatest common divisor");
scalar_expr!(Lcm, lcm, arg_1 arg_2, "least common multiple");
scalar_expr!(Log2, log2, num, "base 2 logarithm of number");
scalar_expr!(Log10, log10, num, "base 10 logarithm of number");
scalar_expr!(Ln, ln, num, "natural logarithm (base e) of number");
scalar_expr!(Power, power, base exponent, "`base` raised to the power of `exponent`");
scalar_expr!(Atan2, atan2, y x, "inverse tangent of a division given in the argument");
scalar_expr!(Log, log, base x, "logarithm of a `x` for a particular `base`");
Expand Down Expand Up @@ -1001,9 +998,6 @@ mod test {
test_nary_scalar_expr!(Trunc, trunc, num, precision);
test_unary_scalar_expr!(Signum, signum);
test_unary_scalar_expr!(Exp, exp);
test_unary_scalar_expr!(Log2, log2);
test_unary_scalar_expr!(Log10, log10);
test_unary_scalar_expr!(Ln, ln);
test_scalar_expr!(Atan2, atan2, y, x);
test_scalar_expr!(Nanvl, nanvl, x, y);
test_scalar_expr!(Iszero, iszero, input);
Expand Down
10 changes: 8 additions & 2 deletions datafusion/functions/src/macros.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -157,14 +157,16 @@ macro_rules! downcast_arg {
/// $NAME: the name of the function
/// $UNARY_FUNC: the unary function to apply to the argument
macro_rules! make_math_unary_udf {
($UDF:ident, $GNAME:ident, $NAME:ident, $UNARY_FUNC:ident) => {
($UDF:ident, $GNAME:ident, $NAME:ident, $UNARY_FUNC:ident, $MONOTONICITY:expr) => {
make_udf_function!($NAME::$UDF, $GNAME, $NAME);

mod $NAME {
use arrow::array::{ArrayRef, Float32Array, Float64Array};
use arrow::datatypes::DataType;
use datafusion_common::{exec_err, DataFusionError, Result};
use datafusion_expr::{ColumnarValue, ScalarUDFImpl, Signature, Volatility};
use datafusion_expr::{
ColumnarValue, FuncMonotonicity, ScalarUDFImpl, Signature, Volatility,
};
use std::any::Any;
use std::sync::Arc;

Expand Down Expand Up @@ -208,6 +210,10 @@ macro_rules! make_math_unary_udf {
}
}

fn monotonicity(&self) -> Result<Option<FuncMonotonicity>> {
Ok($MONOTONICITY)
}

fn invoke(&self, args: &[ColumnarValue]) -> Result<ColumnarValue> {
let args = ColumnarValue::values_to_arrays(args)?;

Expand Down
15 changes: 11 additions & 4 deletions datafusion/functions/src/math/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,14 @@ mod nans;
make_udf_function!(nans::IsNanFunc, ISNAN, isnan);
make_udf_function!(abs::AbsFunc, ABS, abs);

make_math_unary_udf!(TanhFunc, TANH, tanh, tanh);
make_math_unary_udf!(AcosFunc, ACOS, acos, acos);
make_math_unary_udf!(AsinFunc, ASIN, asin, asin);
make_math_unary_udf!(TanFunc, TAN, tan, tan);
make_math_unary_udf!(Log2Func, LOG2, log2, log2, Some(vec![Some(true)]));
Copy link
Contributor

@alamb alamb Mar 30, 2024

Choose a reason for hiding this comment

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

I wonder if we could make a constant or something that would make this easier to read

For example

/// Single monotonic argument
const MONOTONIC_ONE: Option<Vec<bool>> = Some(vec![Some(true)])
const NON_MONOTONIC: Option<Vec<bool>> = None;

And then

Suggested change
make_math_unary_udf!(Log2Func, LOG2, log2, log2, Some(vec![Some(true)]));
make_math_unary_udf!(Log2Func, LOG2, log2, log2, MONOTONIC_ONE);

Perhaps as a follow on PR

Copy link
Contributor Author

@tinfoil-knight tinfoil-knight Mar 30, 2024

Choose a reason for hiding this comment

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

@alamb
I tried this readability fix but Rust doesn't allow allocation (which vec![] needs) in const or static items. Global let is also not allowed.

The compiler suggested this with static:

consider wrapping this expression in `Lazy::new(|| ...)` from the `once_cell` crate: https://crates.io/crates/once_cell

We use once_cell in the physical_plan crate but introducing it to datafusion functions just for this doesn't seem right to me.

Copy link
Contributor

@alamb alamb Mar 30, 2024

Choose a reason for hiding this comment

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

We use once_cell in the physical_plan crate but introducing it to datafusion functions just for this doesn't seem right to me.

That makes sense to me. We would probably have to throw in a clone() or something

I thought about this and wrote up another API that would be much nicer #9879 (both for this callsite and montonicity in general)

make_math_unary_udf!(Log10Func, LOG10, log10, log10, Some(vec![Some(true)]));
make_math_unary_udf!(LnFunc, LN, ln, ln, Some(vec![Some(true)]));

make_math_unary_udf!(TanhFunc, TANH, tanh, tanh, None);
make_math_unary_udf!(AcosFunc, ACOS, acos, acos, None);
make_math_unary_udf!(AsinFunc, ASIN, asin, asin, None);
make_math_unary_udf!(TanFunc, TAN, tan, tan, None);

// Export the functions out of this package, both as expr_fn as well as a list of functions
export_functions!(
Expand All @@ -37,6 +41,9 @@ export_functions!(
"returns true if a given number is +NaN or -NaN otherwise returns false"
),
(abs, num, "returns the absolute value of a given number"),
(log2, num, "base 2 logarithm of a number"),
(log10, num, "base 10 logarithm of a number"),
(ln, num, "natural logarithm (base e) of a number"),
(
acos,
num,
Expand Down
3 changes: 0 additions & 3 deletions datafusion/physical-expr/src/functions.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,9 +221,6 @@ pub fn create_physical_fun(
BuiltinScalarFunction::Lcm => {
Arc::new(|args| make_scalar_function_inner(math_expressions::lcm)(args))
}
BuiltinScalarFunction::Ln => Arc::new(math_expressions::ln),
BuiltinScalarFunction::Log10 => Arc::new(math_expressions::log10),
BuiltinScalarFunction::Log2 => Arc::new(math_expressions::log2),
BuiltinScalarFunction::Nanvl => {
Arc::new(|args| make_scalar_function_inner(math_expressions::nanvl)(args))
}
Expand Down
6 changes: 3 additions & 3 deletions datafusion/proto/proto/datafusion.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,10 +551,10 @@ enum ScalarFunction {
// 7 was Digest
Exp = 8;
Floor = 9;
Ln = 10;
// 10 was Ln
Log = 11;
Log10 = 12;
Log2 = 13;
// 12 was Log10
// 13 was Log2
Round = 14;
Signum = 15;
Sin = 16;
Expand Down
9 changes: 0 additions & 9 deletions datafusion/proto/src/generated/pbjson.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 3 additions & 9 deletions datafusion/proto/src/generated/prost.rs
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 1 addition & 9 deletions datafusion/proto/src/logical_plan/from_proto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use datafusion_expr::{
acosh, asinh, atan, atan2, atanh, cbrt, ceil, coalesce, concat_expr, concat_ws_expr,
cos, cosh, cot, degrees, ends_with, exp,
expr::{self, InList, Sort, WindowFunction},
factorial, find_in_set, floor, gcd, initcap, iszero, lcm, ln, log, log10, log2,
factorial, find_in_set, floor, gcd, initcap, iszero, lcm, log,
logical_plan::{PlanType, StringifiedPlan},
nanvl, pi, power, radians, random, round, signum, sin, sinh, sqrt, substr_index,
translate, trunc, AggregateFunction, Between, BinaryExpr, BuiltInWindowFunction,
Expand Down Expand Up @@ -437,8 +437,6 @@ impl From<&protobuf::ScalarFunction> for BuiltinScalarFunction {
ScalarFunction::Atanh => Self::Atanh,
ScalarFunction::Exp => Self::Exp,
ScalarFunction::Log => Self::Log,
ScalarFunction::Ln => Self::Ln,
ScalarFunction::Log10 => Self::Log10,
ScalarFunction::Degrees => Self::Degrees,
ScalarFunction::Radians => Self::Radians,
ScalarFunction::Factorial => Self::Factorial,
Expand All @@ -449,7 +447,6 @@ impl From<&protobuf::ScalarFunction> for BuiltinScalarFunction {
ScalarFunction::Round => Self::Round,
ScalarFunction::Trunc => Self::Trunc,
ScalarFunction::Concat => Self::Concat,
ScalarFunction::Log2 => Self::Log2,
ScalarFunction::Signum => Self::Signum,
ScalarFunction::ConcatWithSeparator => Self::ConcatWithSeparator,
ScalarFunction::EndsWith => Self::EndsWith,
Expand Down Expand Up @@ -1348,11 +1345,6 @@ pub fn parse_expr(
ScalarFunction::Radians => {
Ok(radians(parse_expr(&args[0], registry, codec)?))
}
ScalarFunction::Log2 => Ok(log2(parse_expr(&args[0], registry, codec)?)),
ScalarFunction::Ln => Ok(ln(parse_expr(&args[0], registry, codec)?)),
ScalarFunction::Log10 => {
Ok(log10(parse_expr(&args[0], registry, codec)?))
}
ScalarFunction::Floor => {
Ok(floor(parse_expr(&args[0], registry, codec)?))
}
Expand Down
3 changes: 0 additions & 3 deletions datafusion/proto/src/logical_plan/to_proto.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1431,16 +1431,13 @@ impl TryFrom<&BuiltinScalarFunction> for protobuf::ScalarFunction {
BuiltinScalarFunction::Gcd => Self::Gcd,
BuiltinScalarFunction::Lcm => Self::Lcm,
BuiltinScalarFunction::Log => Self::Log,
BuiltinScalarFunction::Ln => Self::Ln,
BuiltinScalarFunction::Log10 => Self::Log10,
BuiltinScalarFunction::Degrees => Self::Degrees,
BuiltinScalarFunction::Radians => Self::Radians,
BuiltinScalarFunction::Floor => Self::Floor,
BuiltinScalarFunction::Ceil => Self::Ceil,
BuiltinScalarFunction::Round => Self::Round,
BuiltinScalarFunction::Trunc => Self::Trunc,
BuiltinScalarFunction::Concat => Self::Concat,
BuiltinScalarFunction::Log2 => Self::Log2,
BuiltinScalarFunction::Signum => Self::Signum,
BuiltinScalarFunction::ConcatWithSeparator => Self::ConcatWithSeparator,
BuiltinScalarFunction::EndsWith => Self::EndsWith,
Expand Down