You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "andygrove (via GitHub)" <gi...@apache.org> on 2025/01/31 20:04:01 UTC
[PR] Add spark expr crate [datafusion]
andygrove opened a new pull request, #14392:
URL: https://github.com/apache/datafusion/pull/14392
## Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements and this helps us generate change logs for our releases. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Closes #.
## Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
## What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
## Are these changes tested?
<!--
We typically require tests for all PRs in order to:
1. Prevent the code from being accidentally broken by subsequent changes
2. Serve as another way to document the expected behavior of the code
If tests are not included in your PR, please explain why (for example, are they covered by existing tests)?
-->
## Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938312603
##########
datafusion/spark/src/comet_scalar_funcs.rs:
##########
@@ -0,0 +1,192 @@
+// 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::hash_funcs::*;
+use crate::{
+ spark_ceil, spark_date_add, spark_date_sub, spark_decimal_div, spark_floor,
+ spark_hex, spark_isnan, spark_make_decimal, spark_read_side_padding, spark_round,
+ spark_unhex, spark_unscaled_value, SparkChrFunc,
+};
+use arrow_schema::DataType;
+use datafusion_common::{DataFusionError, Result as DataFusionResult};
+use datafusion_expr::registry::FunctionRegistry;
+use datafusion_expr::{
+ ColumnarValue, ScalarFunctionImplementation, ScalarUDF, ScalarUDFImpl, Signature,
+ Volatility,
+};
+use std::any::Any;
+use std::fmt::Debug;
+use std::sync::Arc;
+
+macro_rules! make_comet_scalar_udf {
Review Comment:
we would probably want to rename this eventually so it didn't refer to comet
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-functions-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2628460299
> I'm wondering how the user should choose what function to use. For instance `to_timestamp` function which may behave differently between non spark and spark env.
In the short term, the main goal is to allow downstream projects to add these functions to a DataFusion context, but users could manually configure their DataFusion context to choose which functions to use.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2701788755
> @andygrove Just checking in. I've been adding tons of new functions to the Sail repo but will port them over after this PR is merged!
Sounds good!
The consensus was that we cannot merge this PR until we have adequate testing in place. I created an issue to track the work at https://github.com/apache/datafusion-comet/issues/1366 but I don't think that anyone is working on this yet.
Would you have bandwidth to help with adding some initial tests in the Comet repo? I think if we have some examples then it will be easier for others to contribute.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "shehabgamin (via GitHub)" <gi...@apache.org>.
shehabgamin commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629161924
> cc @Blizzara and @shehabgamin as you have mentioned interest in helping here
>
Will catch up on this thread tonight. So much happening so fast, exciting!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629435886
> @comphead @andygrove @alamb It may be a good idea to prefix all function names with `spark_` to avoid confusion, conflicts, or unknown behavior between functions that share the same name.
For Comet, we need the function names to match Spark.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove closed pull request #14392: feat: Add `datafusion-spark` crate
URL: https://github.com/apache/datafusion/pull/14392
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-functions-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938062695
##########
Cargo.toml:
##########
@@ -32,6 +32,7 @@ members = [
"datafusion/functions-aggregate-common",
"datafusion/functions-table",
"datafusion/functions-nested",
+ "datafusion/functions-spark",
"datafusion/functions-window",
"datafusion/functions-window-common",
Review Comment:
🤔 you are right that it is kind of strange to have all spark functions in a single crate when we have the other functions in more fine grained crates
Maybe we should make one crate like `datafusion_spark` that holds all the different types of functions:
```diff
i "datafusion/functions-spark",
+ "datafusion/spark",
```
And then over time we can add things like `datafusion_spark_functions_nested` in `datafusion/spark/functions-nested` 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629290634
> It may be a good idea to prefix all function names with spark_ to avoid confusion, conflicts, or unknown behavior between functions that share the same name.
In optimizer, we rely on the name to do such optimization so if we rename it to name like 'spark_count' we might need to add the spark name to those optimize rules as well, which increase the maintainence cost. If we assume the datafusion native function and spark function is mutually exclusive (I guess we do so) then having consistent name for optimizer is preferred choice.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-functions-spark` crate [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2628334890
I'm wondering how the user should choose what function to use. For instance `to_timestamp` function which may behave differently between non spark and spark env.
So the developer implements spark compliant `to_timestamp` function in the crate but when he is running the query
``` select to_timestamp() from t1``` what exactly implementation will be picked up? Like we should introduce something like `FUNCTION CATALOG` or something similar to switch between implementations?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "shehabgamin (via GitHub)" <gi...@apache.org>.
shehabgamin commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629306233
> In optimizer, we rely on the name to do such optimization so if we rename it to name like 'spark_count' we might need to add the spark name to those optimize rules as well, which increase the maintainence cost. If we assume the datafusion native function and spark function is mutually exclusive (I guess we do so) then having consistent name for optimizer is preferred choice.
I'm glad you brought this up, @jayzhan211.
Some Spark functions behave identically to DataFusion functions but have different names. For example:
- Spark’s `startswith(str, substr)` corresponds to DataFusion’s `expr_fn::starts_with(str, substr)`
There are also cases where functions take input arguments in a different order. For example:
- Spark’s `position(substr, str)` corresponds to DataFusion’s `expr_fn::strpos(str, substr)`
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2637335904
> @andygrove @alamb maybe we can do a conditional compilation on function level instead of separate crate? Sort of introduce a feature spark and have different implementations
>
> ```
> #[cfg(spark)]
> fn to_timestamp() {}
>
> #![cfg(spark)]
> fn to_timestamp() {}
> ```
What I was thinking was an either-or type thing. For any particular `SessionContext` a user could choose one of the following dialects + functions:
1. Postgres++ (aka what DataFusion does today)
2. Spark
And then users would somehow pick between the two -- perhaps like
```rust
let ctx = SessionContext::new()
.with_spark_functions()
```
Or something
I don't think having functions like `spark_avg` is going to help anyone as any one system seems unlikely to want two different implementations of `avg`.
I think if we keep the entire function packages separate it would keep the implementations eaiser to understand as well (rather than feature gating each function)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "jayzhan211 (via GitHub)" <gi...@apache.org>.
jayzhan211 commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629310335
> > In optimizer, we rely on the name to do such optimization so if we rename it to name like 'spark_count' we might need to add the spark name to those optimize rules as well, which increase the maintainence cost. If we assume the datafusion native function and spark function is mutually exclusive (I guess we do so) then having consistent name for optimizer is preferred choice.
>
> I'm glad you brought this up, @jayzhan211.
>
> Some Spark functions behave identically to DataFusion functions but have different names. For example:
> - Spark’s `startswith(str, substr)` corresponds to DataFusion’s `expr_fn::starts_with(str, substr)`
>
> There are also cases where functions take input arguments in a different order. For example:
> - Spark’s `position(substr, str)` corresponds to DataFusion’s `expr_fn::strpos(str, substr)`
If the behavior is the same we don't have any reason to copy another one to spark crate, adding alias to the function is enough.
If the diff between the native and spark one is minor we can also add another flag to switch code logic instead of maintaining a copy in another crate so that the maintenance cost could be minimized.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629666721
I'm going to move this to draft for now. Let's keep the conversation going, though.
I plan on working with the community to start adding tests to the `datafusion-comet-spark-expr` crate that don't depend on Spark, and to add some examples, as those are two large areas of valid concern.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2703660394
> > Would you have bandwidth to help with adding some initial tests in the Comet repo? I think if we have some examples then it will be easier for others to contribute.
>
> Yeah forsure. I'm working on pushing out the next Sail release tomorrow, but after that I'll have some bandwidth.
What I recommend is:
1. Create a new PR with a new datafusion-functions-spark crate
2. Add one or two example functions to this crate, along with how the tests might look
Then once we get consensus on what the pattern looks like, we can gin up a project / spread the work to port over existing functions and tests from other projects / sources following that pattern
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-functions-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2628522279
> I'm wondering how the user should choose what function to use. For instance `to_timestamp` function which may behave differently between non spark and spark env.
>
> So the developer implements spark compliant `to_timestamp` function in the crate but when he is running the query ` select to_timestamp() from t1` what exactly implementation will be picked up? Like we should introduce something like `FUNCTION CATALOG` or something similar to switch between implementations?
What I was imagining was that just like today DataFusion comes with some set if "included" functions (mostly following the postgres semantics). Nothing new is added to the core
For users that want to use the spark functions, I am hoping we have something like
```rust
use datafusion_functions_spark::install_spark;
...
// by default this SessionContext has all the same functions as it does today
let ctx = SessionContext::new();
// for users who want spark compatible functions, install the
// spark compatibility functions, potentially overriding
// some/all "built in functions", rewrites, etc
install_spark(&mut ctx)?;
// running queries now use spark compatible syntax
ctx.sql(...)
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "shehabgamin (via GitHub)" <gi...@apache.org>.
shehabgamin commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629317132
> If the behavior is the same we don't have any reason to copy one to spark crate, adding alias to the function is enough.
@jayzhan211 I’ll need to verify this, but I recall encountering a case in DataFusion where some two function names, `A` and `B`, are mapped inversely in Spark. Specifically, some Spark's `B` corresponds to some DataFusion's `A`, and some Spark's `A` corresponds to some DataFusion's `B`.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "Blizzara (via GitHub)" <gi...@apache.org>.
Blizzara commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2630634253
I'd be very happy to see this go in :) Having the functions here in main datafusion repo makes dealing with any breaks and downstream upgrades easier (we'll only need to track/match DF & object_store, but no more Comet). And it maybe allows reusing some of the internal macros, making it easier to contribute new stuff, and reusing logic across the existing impls and the Spark impls. So, nice!
Re the conflicts between Spark and existing impls, not sure what the best there would be, but I think in general the people who care about Spark-compatibility likely don't care about the non-compatible functions, so it could be some kind of toggle on the context/state level. Maybe supplying either a DataFusion-(or Postgres-)FunctionRegistry (default) or a SparkFunctionRegistry, or something like that?
Also, this includes things that aren't strictly UD(A/W)Fs, like [cast](https://github.com/apache/datafusion/pull/14392/files#diff-dfa667ba378c7329ba68ff11097b90144fb2704f901849e70254188eacf820c2). We currently use a FunctionRewrite to convert all DF casts to use the Comet cast, but maybe there could be a better way for that as well?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938313126
##########
datafusion/spark/src/agg_funcs/avg.rs:
##########
@@ -0,0 +1,344 @@
+// 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 arrow::compute::sum;
+use arrow_array::{
+ builder::PrimitiveBuilder,
+ cast::AsArray,
+ types::{Float64Type, Int64Type},
+ Array, ArrayRef, ArrowNumericType, Int64Array, PrimitiveArray,
+};
+use arrow_schema::{DataType, Field};
+use datafusion::logical_expr::{
+ type_coercion::aggregates::avg_return_type, Accumulator, EmitTo, GroupsAccumulator,
+ Signature,
+};
+use datafusion_common::{not_impl_err, Result, ScalarValue};
+use datafusion_physical_expr::expressions::format_state_name;
+use std::{any::Any, sync::Arc};
+
+use arrow_array::ArrowNativeTypeOp;
+use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs};
+use datafusion_expr::Volatility::Immutable;
+use datafusion_expr::{AggregateUDFImpl, ReversedUDAF};
+use DataType::*;
+
+/// AVG aggregate expression
+#[derive(Debug, Clone)]
+pub struct Avg {
Review Comment:
Of course, we should really have some documentation for these expressions explaining what the differences are.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938312400
##########
datafusion/spark/src/agg_funcs/avg.rs:
##########
@@ -0,0 +1,344 @@
+// 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 arrow::compute::sum;
+use arrow_array::{
+ builder::PrimitiveBuilder,
+ cast::AsArray,
+ types::{Float64Type, Int64Type},
+ Array, ArrayRef, ArrowNumericType, Int64Array, PrimitiveArray,
+};
+use arrow_schema::{DataType, Field};
+use datafusion::logical_expr::{
+ type_coercion::aggregates::avg_return_type, Accumulator, EmitTo, GroupsAccumulator,
+ Signature,
+};
+use datafusion_common::{not_impl_err, Result, ScalarValue};
+use datafusion_physical_expr::expressions::format_state_name;
+use std::{any::Any, sync::Arc};
+
+use arrow_array::ArrowNativeTypeOp;
+use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs};
+use datafusion_expr::Volatility::Immutable;
+use datafusion_expr::{AggregateUDFImpl, ReversedUDAF};
+use DataType::*;
+
+/// AVG aggregate expression
+#[derive(Debug, Clone)]
+pub struct Avg {
Review Comment:
not needed for this PR but I am intrested why there is a separate AVG implementation
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "shehabgamin (via GitHub)" <gi...@apache.org>.
shehabgamin commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2708820627
> > > Would you have bandwidth to help with adding some initial tests in the Comet repo? I think if we have some examples then it will be easier for others to contribute.
> >
> >
> > Yeah forsure. I'm working on pushing out the next Sail release tomorrow, but after that I'll have some bandwidth.
>
> What I recommend is:
>
> 1. Create a new PR with a new datafusion-functions-spark crate
>
> 2. Add one or two example functions to this crate, along with how the tests might look
>
>
> Then once we get consensus on what the pattern looks like, we can gin up a project / spread the work to port over existing functions and tests from other projects / sources following that pattern
Will get to this sometime this week.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "shehabgamin (via GitHub)" <gi...@apache.org>.
shehabgamin commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2686793279
@andygrove Just checking in. I've been adding tons of new functions to the Sail repo but will port them over after this PR is merged!
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "shehabgamin (via GitHub)" <gi...@apache.org>.
shehabgamin commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2703064824
> Would you have bandwidth to help with adding some initial tests in the Comet repo? I think if we have some examples then it will be easier for others to contribute.
Yeah forsure. I'm working on pushing out the next Sail release tomorrow, but after that I'll have some bandwidth.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938313058
##########
datafusion/spark/src/agg_funcs/avg.rs:
##########
@@ -0,0 +1,344 @@
+// 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 arrow::compute::sum;
+use arrow_array::{
+ builder::PrimitiveBuilder,
+ cast::AsArray,
+ types::{Float64Type, Int64Type},
+ Array, ArrayRef, ArrowNumericType, Int64Array, PrimitiveArray,
+};
+use arrow_schema::{DataType, Field};
+use datafusion::logical_expr::{
+ type_coercion::aggregates::avg_return_type, Accumulator, EmitTo, GroupsAccumulator,
+ Signature,
+};
+use datafusion_common::{not_impl_err, Result, ScalarValue};
+use datafusion_physical_expr::expressions::format_state_name;
+use std::{any::Any, sync::Arc};
+
+use arrow_array::ArrowNativeTypeOp;
+use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs};
+use datafusion_expr::Volatility::Immutable;
+use datafusion_expr::{AggregateUDFImpl, ReversedUDAF};
+use DataType::*;
+
+/// AVG aggregate expression
+#[derive(Debug, Clone)]
+pub struct Avg {
Review Comment:
One difference is that this version uses an i64 for the count in the accumulator rather than a u64 (which makes sense because Java does not have unsigned ints).
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-functions-spark` crate [datafusion]
Posted by "viirya (via GitHub)" <gi...@apache.org>.
viirya commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938056200
##########
Cargo.toml:
##########
@@ -32,6 +32,7 @@ members = [
"datafusion/functions-aggregate-common",
"datafusion/functions-table",
"datafusion/functions-nested",
+ "datafusion/functions-spark",
"datafusion/functions-window",
"datafusion/functions-window-common",
Review Comment:
Hmm, it is good to add this crate. But I'm wondering that our other function crates like `functions-nested` should also follow Spark behavior, right? So it sounds like all these functions should be in `datafusion-functions-spark`?
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2637338434
> All functions that Sail ports over can be tested without the JVM or Spark Client. Because there is no JVM involvement when running a Sail server, it would be a relatively straightforward process to port function tests from Sail upstream.
@shehabgamin -- this sounds amazing.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2637613868
I filed an issue in Comet to do the necessary work (testing and examples) to prepare to move the crate into DataFusion repo.
https://github.com/apache/datafusion-comet/issues/1366
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-functions-spark` crate [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938062695
##########
Cargo.toml:
##########
@@ -32,6 +32,7 @@ members = [
"datafusion/functions-aggregate-common",
"datafusion/functions-table",
"datafusion/functions-nested",
+ "datafusion/functions-spark",
"datafusion/functions-window",
"datafusion/functions-window-common",
Review Comment:
🤔 you are right that it is kind of strange to have all spark functions in a single crate when we have the other functions in more fine grained crates
Maybe we should make one crate like `datafusion_spark` that holds all the different types of functions:
```suggestion
"datafusion/functions-table",
"datafusion/functions-nested",
"datafusion/spark",
"datafusion/functions-window",
"datafusion/functions-window-common",
````
And then over time we can add things like `datafusion_spark_functions_nested` in `datafusion/spark/functions-nested` 🤔
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-functions-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938073542
##########
Cargo.toml:
##########
@@ -32,6 +32,7 @@ members = [
"datafusion/functions-aggregate-common",
"datafusion/functions-table",
"datafusion/functions-nested",
+ "datafusion/functions-spark",
"datafusion/functions-window",
"datafusion/functions-window-common",
Review Comment:
Thanks @viirya. That is a good point. I renamed `functions-spark` to `spark` for now.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "shehabgamin (via GitHub)" <gi...@apache.org>.
shehabgamin commented on code in PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#discussion_r1938420167
##########
datafusion/spark/src/agg_funcs/avg.rs:
##########
@@ -0,0 +1,344 @@
+// 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 arrow::compute::sum;
+use arrow_array::{
+ builder::PrimitiveBuilder,
+ cast::AsArray,
+ types::{Float64Type, Int64Type},
+ Array, ArrayRef, ArrowNumericType, Int64Array, PrimitiveArray,
+};
+use arrow_schema::{DataType, Field};
+use datafusion::logical_expr::{
+ type_coercion::aggregates::avg_return_type, Accumulator, EmitTo, GroupsAccumulator,
+ Signature,
+};
+use datafusion_common::{not_impl_err, Result, ScalarValue};
+use datafusion_physical_expr::expressions::format_state_name;
+use std::{any::Any, sync::Arc};
+
+use arrow_array::ArrowNativeTypeOp;
+use datafusion_expr::function::{AccumulatorArgs, StateFieldsArgs};
+use datafusion_expr::Volatility::Immutable;
+use datafusion_expr::{AggregateUDFImpl, ReversedUDAF};
+use DataType::*;
+
+/// AVG aggregate expression
+#[derive(Debug, Clone)]
+pub struct Avg {
Review Comment:
Subtle differences (e.g., `i32` instead of `i64`, `nullable`instead of `non-nullable`, `microseconds` instead of `nanoseconds`, etc.) between the DataFusion "default" implementation and the Spark implementation are going to be a common theme with many of these UDFs.
@andygrove @alamb This is where having something like https://github.com/apache/datafusion/issues/14296#issuecomment-2614200125 would be useful. Otherwise we will always have to keep track of any changes made to the original DataFusion "default" implementation.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629456979
> For Comet, we need the function names to match Spark.
Actually, this isn't true. In Comet, we just need the Scala wrapper classes to have the same function names as Spark.
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org
Re: [PR] feat: Add `datafusion-spark` crate [datafusion]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #14392:
URL: https://github.com/apache/datafusion/pull/14392#issuecomment-2629508048
That is exactly what I was mentioning https://github.com/apache/datafusion/pull/14392#issuecomment-2628334890 not sure how to separate out functions with the same names. I think this crate and PR makes a lot of sense until the user wants a mixed implementation, another thing is the code is duplicated, so changes in base function may not be reflected in spark variant.
@andygrove @alamb maybe we can do a conditional compilation on function level instead of separate crate?
Sort of introduce a feature spark and have different implementations
```
#[cfg(spark)]
fn to_timestamp() {}
#![cfg(spark)]
fn to_timestamp() {}
```
--
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: github-unsubscribe@datafusion.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: github-unsubscribe@datafusion.apache.org
For additional commands, e-mail: github-help@datafusion.apache.org