andygrove commented on code in PR #5039: URL: https://github.com/apache/datafusion-comet/pull/5039#discussion_r3659501474
########## native/spark-expr/src/datetime_funcs/make_interval.rs: ########## @@ -0,0 +1,82 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::arithmetic_overflow_error; +use arrow::array::Array; +use arrow::datatypes::DataType; +use datafusion::common::Result; +use datafusion::logical_expr::{ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl, Signature}; +use datafusion_spark::function::datetime::make_interval::SparkMakeInterval as DataFusionMakeInterval; + +#[derive(Debug, PartialEq, Eq, Hash)] +pub struct SparkMakeInterval { + inner: DataFusionMakeInterval, + fail_on_error: bool, +} + +impl SparkMakeInterval { + pub fn new(fail_on_error: bool) -> Self { + Self { + inner: DataFusionMakeInterval::new(), + fail_on_error, + } + } +} + +impl Default for SparkMakeInterval { + fn default() -> Self { + Self::new(false) + } +} + +impl ScalarUDFImpl for SparkMakeInterval { + fn name(&self) -> &str { + self.inner.name() + } + + fn signature(&self) -> &Signature { + self.inner.signature() + } + + fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { + self.inner.return_type(arg_types) + } + + fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> { + let inputs = if self.fail_on_error && !args.args.is_empty() { + Some(ColumnarValue::values_to_arrays(&args.args)?) + } else { + None + }; + let result = self.inner.invoke_with_args(args)?; Review Comment: There's a compatibility concern I'd like to flag with the underlying DataFusion kernel. Two related issues: **Nanosecond vs microsecond overflow.** Spark's `IntervalUtils.makeInterval` stores time components as `int64` **microseconds** via `secs.toUnscaledLong`, so a `Decimal(18, 6)` seconds value fits comfortably (max ≈ 1e18 micros, well under `Long.MaxValue`). DataFusion's kernel accumulates in nanoseconds, so it overflows at roughly `secs > 9_223_372_036` (~292 years). Any Decimal(18, 6) seconds value beyond that boundary silently returns null under this PR (or throws under ANSI) while Spark returns a valid interval. Spark's own `sql-tests/inputs/interval.sql` exercises exactly this range: ```sql select make_interval(1, 2, 3, 4, 0, 0, 123456789012.123456); ``` **Float64 coercion loses microsecond precision.** DataFusion's `SparkMakeInterval` signature coerces `secs` to `Float64`, but Spark's `MakeInterval.inputTypes` is `Decimal(18, 6)` and preserves microseconds exactly. For `secs = 999999999.999999`, the Float64 round-trip yields `frac * 1e9 ≈ 999999046` instead of `999999000` — a ~46 ns drift that translates into a wrong microsecond count on the JVM side. The small values currently in the fixture (`7.123456`, `100.000001`, `-1.5`) happen to be exactly representable so they don't expose this. Given both, would it make sense to mark this expression `Incompatible(Some("..."))` in `getSupportLevel`, and add a `getIncompatibleReasons()` string so the auto-generated compat page warns users? Marking it `Native` in `expressions.md` with no caveat currently overstates the compatibility. ########## native/spark-expr/src/datetime_funcs/make_interval.rs: ########## @@ -0,0 +1,82 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +use crate::arithmetic_overflow_error; +use arrow::array::Array; +use arrow::datatypes::DataType; +use datafusion::common::Result; +use datafusion::logical_expr::{ColumnarValue, ScalarFunctionArgs, ScalarUDFImpl, Signature}; +use datafusion_spark::function::datetime::make_interval::SparkMakeInterval as DataFusionMakeInterval; + +#[derive(Debug, PartialEq, Eq, Hash)] +pub struct SparkMakeInterval { + inner: DataFusionMakeInterval, + fail_on_error: bool, +} + +impl SparkMakeInterval { + pub fn new(fail_on_error: bool) -> Self { + Self { + inner: DataFusionMakeInterval::new(), + fail_on_error, + } + } +} + +impl Default for SparkMakeInterval { + fn default() -> Self { + Self::new(false) + } +} + +impl ScalarUDFImpl for SparkMakeInterval { + fn name(&self) -> &str { + self.inner.name() + } + + fn signature(&self) -> &Signature { + self.inner.signature() + } + + fn return_type(&self, arg_types: &[DataType]) -> Result<DataType> { + self.inner.return_type(arg_types) + } + + fn invoke_with_args(&self, args: ScalarFunctionArgs) -> Result<ColumnarValue> { + let inputs = if self.fail_on_error && !args.args.is_empty() { + Some(ColumnarValue::values_to_arrays(&args.args)?) Review Comment: Small perf nit for the ANSI path. `ColumnarValue::values_to_arrays` runs here, and then again inside DataFusion's `make_scalar_function` when `self.inner.invoke_with_args(args)` is called. Scalar inputs get expanded to length-N arrays twice. Also, on line 69–70 the overflow check iterates `0..values.len()` unconditionally. For batches with `null_count() == 0`, this scans the whole range even though there's nothing to detect. A cheap improvement would be to short-circuit when `values.null_count() == 0` and, otherwise, walk `values.nulls()` positions directly. All ANSI-only, so blast radius is small — happy to defer if you'd rather keep the code simple. -- 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]
