You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@datafusion.apache.org by "buraksenn (via GitHub)" <gi...@apache.org> on 2024/10/31 13:26:09 UTC
[PR] Convert nth_value builtIn function to UDWF [datafusion]
buraksenn opened a new pull request, #13201:
URL: https://github.com/apache/datafusion/pull/13201
## Which issue does this PR close?
Closes #12649
## Rationale for this change
Context: #8709
## What changes are included in this PR?
- cleanup BuiltinWindowFunctions
- move builtin nth_value function to udwf
### It got pretty out of hand because of nth_value and BuiltinWindowFunction references across the repo so I wanted to open a draft PR to get feedback. I should remove the parts in the PR right?
## Are these changes tested?
- #### will comment out testing part and fix them
## Are there any user-facing changes?
no
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836726411
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -119,13 +119,36 @@ regr_slope(c11, '2') over () as min1
from aggregate_test_100
order by c9
-# WindowFunction with BuiltInWindowFunction wrong signature
-statement error DataFusion error: Error during planning: No function matches the given name and argument types 'NTH_VALUE\(Int32, Int64, Int64\)'\. You might need to add explicit type casts\.\n\tCandidate functions:\n\tNTH_VALUE\(Any, Any\)
+# WindowFunction wrong signature
+statement error
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
from aggregate_test_100
order by c9
+----
+DataFusion error: Error during planning: Error during planning: Coercion from [Int32, Int64, Int64] to the signature OneOf([Any(0), Any(1), Any(2)]) failed. No function matches the given name and argument types 'nth_value(Int32, Int64, Int64)'. You might need to add explicit type casts.
+ Candidate functions:
+ nth_value()
+ nth_value(Any)
+ nth_value(Any, Any)
Review Comment:
Just trying to fix that. Thanks so much @jcsherin
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2464819831
Working on about @jcsherin. I will commit some changes today so marking this PR draft until it becomes ready again
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836542435
##########
datafusion/proto/src/physical_plan/to_proto.rs:
##########
@@ -101,39 +101,10 @@ pub fn serialize_physical_window_expr(
codec: &dyn PhysicalExtensionCodec,
) -> Result<protobuf::PhysicalWindowExprNode> {
let expr = window_expr.as_any();
- let mut args = window_expr.expressions().to_vec();
+ let args = window_expr.expressions().to_vec();
let window_frame = window_expr.get_window_frame();
- let (window_function, fun_definition) = if let Some(built_in_window_expr) =
- expr.downcast_ref::<BuiltInWindowExpr>()
- {
- let expr = built_in_window_expr.get_built_in_func_expr();
- let built_in_fn_expr = expr.as_any();
-
- let builtin_fn =
- if let Some(nth_value_expr) = built_in_fn_expr.downcast_ref::<NthValue>() {
- match nth_value_expr.get_kind() {
- NthValueKind::First => protobuf::BuiltInWindowFunction::FirstValue,
- NthValueKind::Last => protobuf::BuiltInWindowFunction::LastValue,
- NthValueKind::Nth(n) => {
- args.insert(
- 1,
- Arc::new(Literal::new(
- datafusion_common::ScalarValue::Int64(Some(n)),
- )),
- );
- protobuf::BuiltInWindowFunction::NthValue
- }
- }
- } else {
- return not_impl_err!("BuiltIn function not supported: {expr:?}");
- };
-
- (
- physical_window_expr_node::WindowFunction::BuiltInFunction(builtin_fn as i32),
- None,
- )
- } else if let Some(plain_aggr_window_expr) =
Review Comment:
this part is causing the issue since we can't convert UDWF to anything 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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1835785085
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -272,31 +269,34 @@ fn roundtrip_window() -> Result<()> {
let field_b = Field::new("b", DataType::Int64, false);
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
- let window_frame = WindowFrame::new_bounds(
+ let _window_frame = WindowFrame::new_bounds(
datafusion_expr::WindowFrameUnits::Range,
WindowFrameBound::Preceding(ScalarValue::Int64(None)),
WindowFrameBound::CurrentRow,
);
- let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
- Arc::new(NthValue::first(
- "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
- col("a", &schema)?,
- DataType::Int64,
- false,
- )),
- &[col("b", &schema)?],
- &LexOrdering{
- inner: vec![PhysicalSortExpr {
- expr: col("a", &schema)?,
- options: SortOptions {
- descending: false,
- nulls_first: false,
- },
- }]
- },
- Arc::new(window_frame),
- ));
+ // let udwf_window_expr = Expr::WindowFunction(WindowFunction::new(
+ // WindowFunctionDefinition::WindowUDF(first_value_udwf()),
+
+ // let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
+ // Arc::new(NthValue::first(
+ // "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
+ // col("a", &schema)?,
+ // DataType::Int64,
+ // false,
+ // )),
+ // &[col("b", &schema)?],
+ // &LexOrdering{
+ // inner: vec![PhysicalSortExpr {
Review Comment:
I could not make this expression work with the first_value_udwf? I looked to the similar things but still could not build the required expression. Can I get some help please @jcsherin @berkaysynnada ?
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2456154778
> I think it would be better to update WindowUDFImpl in a followup PR for enhancement right? I can skip this test case in the scope of this PR. Correct me if I'm wrong please
Sure, we can improve the API in another PR.
Here is a workaround that fixes the failing test:
```rust
// In datafusion/physical-plan/src/windows/mod.rs
fn create_udwf_window_expr(
fun: &Arc<WindowUDF>,
args: &[Arc<dyn PhysicalExpr>],
input_schema: &Schema,
name: String,
ignore_nulls: bool,
) -> Result<Arc<dyn BuiltInWindowFunctionExpr>> {
// need to get the types into an owned vec for some reason
let input_types: Vec<_> = args
.iter()
.map(|arg| arg.data_type(input_schema))
.collect::<Result<_>>()?;
let udwf_expr =
Arc::new(WindowUDFExpr {
fun: Arc::clone(fun),
args: args.to_vec(),
input_types,
name,
is_reversed: false,
ignore_nulls,
});
/// Early validation of input expressions
///
/// We create a partition evaluator because in the user-defined window
/// implementation this is where code for parsing input expressions
/// exist. The benefits are:
/// - If any of the input expressions are invalid we catch them early
/// in the planning phase, rather than during execution.
/// - Maintains compatibility with built-in (now removed) window
/// functions validation behavior.
/// - Predictable and reliable error handling.
///
/// See discussion here:
/// https://github.com/apache/datafusion/pull/13201#issuecomment-2454209975
let _ = udwf_expr.create_evaluator()?;
Ok(udwf_expr)
}
```
I verified that this works in your branch.
```sql
DataFusion CLI v42.2.0
> CREATE TABLE t1(v1 BIGINT);
0 row(s) fetched.
Elapsed 0.019 seconds.
> EXPLAIN SELECT NTH_VALUE('+Inf'::Double, v1) OVER (PARTITION BY v1) FROM t1;
+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type | plan |
+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| logical_plan | Projection: nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING |
| | WindowAggr: windowExpr=[[nth_value(Float64(inf), t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING AS nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]] |
| | TableScan: t1 projection=[v1] |
+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.018 seconds.
> SELECT NTH_VALUE('+Inf'::Double, v1) OVER (PARTITION BY v1) FROM t1;
This feature is not implemented: There is only support Literal types for field at idx: 1 in Window Function
```
This workaround may not be ideal, but at least we do not need to skip a test here. Also please feel free to update the code/comments as you see fit.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1835784521
##########
datafusion/expr/src/built_in_window_function.rs:
##########
@@ -17,115 +17,12 @@
//! Built-in functions module contains all the built-in functions definitions.
-use std::fmt;
-use std::str::FromStr;
-
-use crate::type_coercion::functions::data_types;
-use crate::utils;
-use crate::{Signature, Volatility};
-use datafusion_common::{plan_datafusion_err, plan_err, DataFusionError, Result};
-
-use arrow::datatypes::DataType;
-
use strum_macros::EnumIter;
-impl fmt::Display for BuiltInWindowFunction {
- fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- write!(f, "{}", self.name())
- }
-}
-
/// A [window function] built in to DataFusion
///
/// [Window Function]: https://en.wikipedia.org/wiki/Window_function_(SQL)
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash, EnumIter)]
pub enum BuiltInWindowFunction {
- /// returns value evaluated at the row that is the first row of the window frame
- FirstValue,
- /// Returns value evaluated at the row that is the last row of the window frame
- LastValue,
- /// Returns value evaluated at the row that is the nth row of the window frame (counting from 1); returns null if no such row
- NthValue,
-}
-
-impl BuiltInWindowFunction {
- pub fn name(&self) -> &str {
- use BuiltInWindowFunction::*;
- match self {
- FirstValue => "first_value",
- LastValue => "last_value",
- NthValue => "NTH_VALUE",
- }
- }
-}
-
-impl FromStr for BuiltInWindowFunction {
- type Err = DataFusionError;
- fn from_str(name: &str) -> Result<BuiltInWindowFunction> {
- Ok(match name.to_uppercase().as_str() {
- "FIRST_VALUE" => BuiltInWindowFunction::FirstValue,
- "LAST_VALUE" => BuiltInWindowFunction::LastValue,
- "NTH_VALUE" => BuiltInWindowFunction::NthValue,
- _ => return plan_err!("There is no built-in window function named {name}"),
- })
- }
-}
-
-/// Returns the datatype of the built-in window function
-impl BuiltInWindowFunction {
- pub fn return_type(&self, input_expr_types: &[DataType]) -> Result<DataType> {
- // Note that this function *must* return the same type that the respective physical expression returns
- // or the execution panics.
-
- // Verify that this is a valid set of data types for this function
- data_types(input_expr_types, &self.signature())
- // Original errors are all related to wrong function signature
- // Aggregate them for better error message
- .map_err(|_| {
- plan_datafusion_err!(
- "{}",
- utils::generate_signature_error_msg(
- &format!("{self}"),
- self.signature(),
- input_expr_types,
- )
- )
- })?;
-
- match self {
- BuiltInWindowFunction::FirstValue
- | BuiltInWindowFunction::LastValue
- | BuiltInWindowFunction::NthValue => Ok(input_expr_types[0].clone()),
- }
- }
-
- /// The signatures supported by the built-in window function `fun`.
- pub fn signature(&self) -> Signature {
- // Note: The physical expression must accept the type returned by this function or the execution panics.
- match self {
- BuiltInWindowFunction::FirstValue | BuiltInWindowFunction::LastValue => {
- Signature::any(1, Volatility::Immutable)
- }
- BuiltInWindowFunction::NthValue => Signature::any(2, Volatility::Immutable),
- }
- }
-}
-
-#[cfg(test)]
-mod tests {
- use super::*;
- use strum::IntoEnumIterator;
- #[test]
- // Test for BuiltInWindowFunction's Display and from_str() implementations.
- // For each variant in BuiltInWindowFunction, it converts the variant to a string
- // and then back to a variant. The test asserts that the original variant and
- // the reconstructed variant are the same. This assertion is also necessary for
- // function suggestion. See https://github.com/apache/datafusion/issues/8082
- fn test_display_and_from_str() {
- for func_original in BuiltInWindowFunction::iter() {
- let func_name = func_original.to_string();
- let func_from_str = BuiltInWindowFunction::from_str(&func_name).unwrap();
- assert_eq!(func_from_str, func_original);
- }
- }
Review Comment:
added
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836594502
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -0,0 +1,555 @@
+// 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.
+
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
+
+use std::any::Any;
+use std::cmp::Ordering;
+use std::fmt::Debug;
+use std::ops::Range;
+use std::sync::OnceLock;
+
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_datafusion_err, exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
+use datafusion_expr::window_state::WindowAggState;
+use datafusion_expr::{
+ Documentation, Literal, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+get_or_init_udwf!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+get_or_init_udwf!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+get_or_init_udwf!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
+
+/// Create an expression to represent the `first_value` window function
+///
+pub fn first_value(arg: datafusion_expr::Expr) -> datafusion_expr::Expr {
+ first_value_udwf().call(vec![arg])
+}
+
+/// Create an expression to represent the `last_value` window function
+///
+pub fn last_value(arg: datafusion_expr::Expr) -> datafusion_expr::Expr {
+ last_value_udwf().call(vec![arg])
+}
+
+/// Create an expression to represent the `nth_value` window function
+///
+pub fn nth_value(arg: datafusion_expr::Expr, n: Option<i64>) -> datafusion_expr::Expr {
Review Comment:
Also add a roundtrip logical plan test for this API here: https://github.com/apache/datafusion/blob/ac79ef3442e65f6197c7234da9fad964895b9101/datafusion/proto/tests/cases/roundtrip_logical_plan.rs#L943
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829377262
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -120,7 +120,7 @@ from aggregate_test_100
order by c9
# WindowFunction with BuiltInWindowFunction wrong signature
-statement error DataFusion error: Error during planning: No function matches the given name and argument types 'NTH_VALUE\(Int32, Int64, Int64\)'\. You might need to add explicit type casts\.\n\tCandidate functions:\n\tNTH_VALUE\(Any, Any\)
+statement error Coercion
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
Review Comment:
I deleted `Coercion` so that test will fail. Then I used the `--complete` flag to auto-generate the multiline error statement automatically.
`RUST_BACKTRACE=1 cargo test -p datafusion-sqllogictest --test sqllogictests errors.slt -- --complete`
```suggestion
statement error
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
from aggregate_test_100
order by c9
----
DataFusion error: Error during planning: Error during planning: Coercion from [Int32, Int64, Int64] to the signature OneOf([Any(0), Any(1), Any(2)]) failed. No function matches the given name and argument types 'nth_value(Int32, Int64, Int64)'. You might need to add explicit type casts.
Candidate functions:
nth_value()
nth_value(Any)
nth_value(Any, Any)
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2465538026
Let us know if you need help
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2466856184
> Let us know if you need help
Sorry @alamb this dragged on much longer than I anticipated. I could not find time for the recent days and by opening draft PR I kinda blocked other people working on it sorry about that. Will open more complete PRs and try not to block further on. I'm currently working on it and hopefully make it ready in a few hours
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "berkaysynnada (via GitHub)" <gi...@apache.org>.
berkaysynnada commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836070735
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -272,31 +269,34 @@ fn roundtrip_window() -> Result<()> {
let field_b = Field::new("b", DataType::Int64, false);
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
- let window_frame = WindowFrame::new_bounds(
+ let _window_frame = WindowFrame::new_bounds(
datafusion_expr::WindowFrameUnits::Range,
WindowFrameBound::Preceding(ScalarValue::Int64(None)),
WindowFrameBound::CurrentRow,
);
- let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
- Arc::new(NthValue::first(
- "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
- col("a", &schema)?,
- DataType::Int64,
- false,
- )),
- &[col("b", &schema)?],
- &LexOrdering{
- inner: vec![PhysicalSortExpr {
- expr: col("a", &schema)?,
- options: SortOptions {
- descending: false,
- nulls_first: false,
- },
- }]
- },
- Arc::new(window_frame),
- ));
+ // let udwf_window_expr = Expr::WindowFunction(WindowFunction::new(
+ // WindowFunctionDefinition::WindowUDF(first_value_udwf()),
+
+ // let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
+ // Arc::new(NthValue::first(
+ // "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
+ // col("a", &schema)?,
+ // DataType::Int64,
+ // false,
+ // )),
+ // &[col("b", &schema)?],
+ // &LexOrdering{
+ // inner: vec![PhysicalSortExpr {
Review Comment:
I've sent a commit, I believe it will unblock you. Perhaps we can even remove this expression since `SlidingAggregateWindowExpr` has already been added in the test (and `PlainAggregateWindowExpr` has also), but keep it 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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836112390
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1500,129 +1494,6 @@ mod tests {
Ok(source)
}
- // Tests NTH_VALUE(negative index) with memoize feature.
- // To be able to trigger memoize feature for NTH_VALUE we need to
- // - feed BoundedWindowAggExec with batch stream data.
- // - Window frame should contain UNBOUNDED PRECEDING.
- // It hard to ensure these conditions are met, from the sql query.
- #[tokio::test]
- async fn test_window_nth_value_bounded_memoize() -> Result<()> {
Review Comment:
I reused `create_udwf_window_expr` by changing the visibility to current module to build the physical expressions required for this test.
You may apply this patch as-is and the test will pass.
```patch
diff --git a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
index 0a898bd85..a031253e4 100644
--- a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
+++ b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
@@ -1163,7 +1163,9 @@ mod tests {
use crate::expressions::PhysicalSortExpr;
use crate::projection::ProjectionExec;
use crate::streaming::{PartitionStream, StreamingTableExec};
- use crate::windows::{create_window_expr, BoundedWindowAggExec, InputOrderMode};
+ use crate::windows::{
+ create_udwf_window_expr, create_window_expr, BoundedWindowAggExec, InputOrderMode,
+ };
use crate::{execute_stream, get_plan_string, ExecutionPlan};
use arrow_array::builder::{Int64Builder, UInt64Builder};
@@ -1180,13 +1182,14 @@ mod tests {
WindowFrame, WindowFrameBound, WindowFrameUnits, WindowFunctionDefinition,
};
use datafusion_functions_aggregate::count::count_udaf;
- use datafusion_functions_window::nth_value::first_value_udwf;
use datafusion_functions_window::nth_value::last_value_udwf;
use datafusion_functions_window::nth_value::nth_value_udwf;
- use datafusion_physical_expr::expressions::{col, lit, Column};
+ use datafusion_physical_expr::expressions::{col, Column, Literal};
use datafusion_physical_expr::{LexOrdering, PhysicalExpr};
use crate::memory::MemoryExec;
+ use crate::common::collect;
+ use datafusion_physical_expr::window::BuiltInWindowExpr;
use futures::future::Shared;
use futures::{pin_mut, ready, FutureExt, Stream, StreamExt};
use itertools::Itertools;
@@ -1524,36 +1527,35 @@ mod tests {
)
.map(|e| Arc::new(e) as Arc<dyn ExecutionPlan>)?;
let col_a = col("a", &schema)?;
- let nth_value_func1 = NthValue::nth(
- "nth_value(-1)",
- Arc::clone(&col_a),
- DataType::Int32,
- 1,
+ let nth_value_func1 = create_udwf_window_expr(
+ &nth_value_udwf(),
+ &[Arc::clone(&col_a), Arc::new(Literal::new(ScalarValue::Int32(Some(1))))],
+ &schema,
+ "nth_value(-1)".to_string(),
false,
)?
.reverse_expr()
.unwrap();
- let nth_value_func2 = NthValue::nth(
- "nth_value(-2)",
- Arc::clone(&col_a),
- DataType::Int32,
- 2,
+ let nth_value_func2 = create_udwf_window_expr(
+ &nth_value_udwf(),
+ &[Arc::clone(&col_a), Arc::new(Literal::new(ScalarValue::Int32(Some(2))))],
+ &schema,
+ "nth_value(-2)".to_string(),
false,
- )?
- .reverse_expr()
- .unwrap();
- let last_value_func = Arc::new(NthValue::last(
- "last",
- Arc::clone(&col_a),
- DataType::Int32,
+ )?.reverse_expr().unwrap();
+ let last_value_func = create_udwf_window_expr(
+ &last_value_udwf(),
+ &[Arc::clone(&col_a)],
+ &schema,
+ "last".to_string(),
false,
- )) as _;
+ )?;
let window_exprs = vec![
// LAST_VALUE(a)
Arc::new(BuiltInWindowExpr::new(
last_value_func,
&[],
- &LexOrdering::default(),
+ &LexOrdering::default(),
Arc::new(WindowFrame::new_bounds(
WindowFrameUnits::Rows,
WindowFrameBound::Preceding(ScalarValue::UInt64(None)),
diff --git a/datafusion/physical-plan/src/windows/mod.rs b/datafusion/physical-plan/src/windows/mod.rs
index aee0de94e..a4630c4f7 100644
--- a/datafusion/physical-plan/src/windows/mod.rs
+++ b/datafusion/physical-plan/src/windows/mod.rs
@@ -158,7 +158,7 @@ fn window_expr_from_aggregate_expr(
}
/// Creates a `BuiltInWindowFunctionExpr` suitable for a user defined window function
-fn create_udwf_window_expr(
+pub(self) fn create_udwf_window_expr(
fun: &Arc<WindowUDF>,
args: &[Arc<dyn PhysicalExpr>],
input_schema: &Schema,
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829198945
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -2172,31 +2171,6 @@ mod tests {
Ok(())
}
- #[tokio::test]
- async fn select_with_window_exprs() -> Result<()> {
- // build plan using Table API
- let t = test_table().await?;
- let first_row = Expr::WindowFunction(WindowFunction::new(
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::FirstValue,
- ),
Review Comment:
This test should be fixed to use `first_value_udwf()` and not deleted.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829272956
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -507,17 +507,6 @@ message ScalarUDFExprNode {
enum BuiltInWindowFunction {
UNSPECIFIED = 0; // https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum
- // ROW_NUMBER = 0;
- // RANK = 1;
- // DENSE_RANK = 2;
- // PERCENT_RANK = 3;
- // CUME_DIST = 4;
- // NTILE = 5;
- // LAG = 6;
- // LEAD = 7;
- FIRST_VALUE = 8;
- LAST_VALUE = 9;
- NTH_VALUE = 10;
Review Comment:
Instead of removing everything just comment out the enums. After the change you will need to run `regen.sh` again.
This too we can tackle in a cleanup PR.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2468432128
@buraksenn and @berkaysynnada Thanks!
@alamb This PR is ready.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836112390
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1500,129 +1494,6 @@ mod tests {
Ok(source)
}
- // Tests NTH_VALUE(negative index) with memoize feature.
- // To be able to trigger memoize feature for NTH_VALUE we need to
- // - feed BoundedWindowAggExec with batch stream data.
- // - Window frame should contain UNBOUNDED PRECEDING.
- // It hard to ensure these conditions are met, from the sql query.
- #[tokio::test]
- async fn test_window_nth_value_bounded_memoize() -> Result<()> {
Review Comment:
~~I reused `create_udwf_window_expr` by changing the visibility to current module to build the physical expressions required for this test.~~
~~You may apply this patch as-is and the test will pass.~~
In [5c4e0f6](https://github.com/apache/datafusion/pull/13201/commits/5c4e0f6ec67945b0e6f70662b5fd75513f77af65) the visibility of `create_udwf_window_expr` is changed to `pub` so that it can be reused in physical plan roundtrip test.
Updated patch:
```patch
diff --git a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
index 0a898bd85..4b69773cf 100644
--- a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
+++ b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
@@ -1163,7 +1163,9 @@ mod tests {
use crate::expressions::PhysicalSortExpr;
use crate::projection::ProjectionExec;
use crate::streaming::{PartitionStream, StreamingTableExec};
- use crate::windows::{create_window_expr, BoundedWindowAggExec, InputOrderMode};
+ use crate::windows::{
+ create_udwf_window_expr, create_window_expr, BoundedWindowAggExec, InputOrderMode,
+ };
use crate::{execute_stream, get_plan_string, ExecutionPlan};
use arrow_array::builder::{Int64Builder, UInt64Builder};
@@ -1180,13 +1182,14 @@ mod tests {
WindowFrame, WindowFrameBound, WindowFrameUnits, WindowFunctionDefinition,
};
use datafusion_functions_aggregate::count::count_udaf;
- use datafusion_functions_window::nth_value::first_value_udwf;
use datafusion_functions_window::nth_value::last_value_udwf;
use datafusion_functions_window::nth_value::nth_value_udwf;
- use datafusion_physical_expr::expressions::{col, lit, Column};
+ use datafusion_physical_expr::expressions::{col, Column, Literal};
use datafusion_physical_expr::{LexOrdering, PhysicalExpr};
+ use crate::common::collect;
use crate::memory::MemoryExec;
+ use datafusion_physical_expr::window::BuiltInWindowExpr;
use futures::future::Shared;
use futures::{pin_mut, ready, FutureExt, Stream, StreamExt};
use itertools::Itertools;
@@ -1524,30 +1527,37 @@ mod tests {
)
.map(|e| Arc::new(e) as Arc<dyn ExecutionPlan>)?;
let col_a = col("a", &schema)?;
- let nth_value_func1 = NthValue::nth(
- "nth_value(-1)",
- Arc::clone(&col_a),
- DataType::Int32,
- 1,
+ let nth_value_func1 = create_udwf_window_expr(
+ &nth_value_udwf(),
+ &[
+ Arc::clone(&col_a),
+ Arc::new(Literal::new(ScalarValue::Int32(Some(1)))),
+ ],
+ &schema,
+ "nth_value(-1)".to_string(),
false,
)?
.reverse_expr()
.unwrap();
- let nth_value_func2 = NthValue::nth(
- "nth_value(-2)",
- Arc::clone(&col_a),
- DataType::Int32,
- 2,
+ let nth_value_func2 = create_udwf_window_expr(
+ &nth_value_udwf(),
+ &[
+ Arc::clone(&col_a),
+ Arc::new(Literal::new(ScalarValue::Int32(Some(2)))),
+ ],
+ &schema,
+ "nth_value(-2)".to_string(),
false,
)?
.reverse_expr()
.unwrap();
- let last_value_func = Arc::new(NthValue::last(
- "last",
- Arc::clone(&col_a),
- DataType::Int32,
+ let last_value_func = create_udwf_window_expr(
+ &last_value_udwf(),
+ &[Arc::clone(&col_a)],
+ &schema,
+ "last".to_string(),
false,
- )) as _;
+ )?;
let window_exprs = vec![
// LAST_VALUE(a)
Arc::new(BuiltInWindowExpr::new(
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836603721
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -272,31 +269,34 @@ fn roundtrip_window() -> Result<()> {
let field_b = Field::new("b", DataType::Int64, false);
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
- let window_frame = WindowFrame::new_bounds(
+ let _window_frame = WindowFrame::new_bounds(
datafusion_expr::WindowFrameUnits::Range,
WindowFrameBound::Preceding(ScalarValue::Int64(None)),
WindowFrameBound::CurrentRow,
);
- let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
- Arc::new(NthValue::first(
- "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
- col("a", &schema)?,
- DataType::Int64,
- false,
- )),
- &[col("b", &schema)?],
- &LexOrdering{
- inner: vec![PhysicalSortExpr {
- expr: col("a", &schema)?,
- options: SortOptions {
- descending: false,
- nulls_first: false,
- },
- }]
- },
- Arc::new(window_frame),
- ));
+ // let udwf_window_expr = Expr::WindowFunction(WindowFunction::new(
+ // WindowFunctionDefinition::WindowUDF(first_value_udwf()),
+
+ // let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
+ // Arc::new(NthValue::first(
+ // "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
+ // col("a", &schema)?,
+ // DataType::Int64,
+ // false,
+ // )),
+ // &[col("b", &schema)?],
+ // &LexOrdering{
+ // inner: vec![PhysicalSortExpr {
Review Comment:
For this to work we need to add serialization for user-defined window functions.
I think we can remove `builtin_window_expr` from the round trip test
(but keep the `PlainAggregateWindowExpr` and `SlidingAggregateWindowExpr` tests intact).
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1835767976
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -120,7 +120,7 @@ from aggregate_test_100
order by c9
# WindowFunction with BuiltInWindowFunction wrong signature
-statement error DataFusion error: Error during planning: No function matches the given name and argument types 'NTH_VALUE\(Int32, Int64, Int64\)'\. You might need to add explicit type casts\.\n\tCandidate functions:\n\tNTH_VALUE\(Any, Any\)
+statement error Coercion
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
Review Comment:
I applied this suggestion but now sure why it remained like this. Resolving it. Thanks
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829236883
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -15,143 +15,254 @@
// specific language governing permissions and limitations
// under the License.
-//! Defines physical expressions for `FIRST_VALUE`, `LAST_VALUE`, and `NTH_VALUE`
-//! functions that can be evaluated at run time during query execution.
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
use std::any::Any;
use std::cmp::Ordering;
+use std::fmt::Debug;
use std::ops::Range;
-use std::sync::Arc;
-
-use crate::window::window_expr::{NthValueKind, NthValueState};
-use crate::window::BuiltInWindowFunctionExpr;
-use crate::PhysicalExpr;
+use std::sync::OnceLock;
-use arrow::array::{Array, ArrayRef};
-use arrow::datatypes::{DataType, Field};
-use datafusion_common::Result;
-use datafusion_common::ScalarValue;
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
use datafusion_expr::window_state::WindowAggState;
-use datafusion_expr::PartitionEvaluator;
+use datafusion_expr::{
+ Documentation, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+define_udwf_and_expr!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+define_udwf_and_expr!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+define_udwf_and_expr!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
Review Comment:
Once you have a working `nth_value` expression API please add a roundtrip logical plan test here:
https://github.com/apache/datafusion/blob/ac79ef3442e65f6197c7234da9fad964895b9101/datafusion/proto/tests/cases/roundtrip_logical_plan.rs#L943-L945
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2454209975
> > External error: query failed: DataFusion error: Arrow error: Invalid argument error: It is not possible to concatenate arrays of different data types.
>
> In the built-in (older) version the output field is defined like:
>
> ```rust
> fn field(&self) -> Result<Field> {
> let nullable = true;
> Ok(Field::new(&self.name, self.data_type.clone(), nullable))
> }
> ```
>
> In the current code, the data type of the field is hard-coded as `DataType::UInt64`:
>
> ```rust
> fn field(&self, field_args: WindowUDFFieldArgs) -> Result<Field> {
> let nullable = true;
>
> Ok(Field::new(field_args.name(), DataType::UInt64, nullable))
> }
> ```
>
> To fix this use `field_args` to get the data type of the input expression rather than hard-coding `DataType::UInt64`.
Thanks, @jcsherin that was the fix. I've fixed that issue but encountered another one. I return Error from partition evaluator but I think it is not honored.
```
External error: query is expected to fail, but actually succeed:
[SQL] SELECT NTH_VALUE('+Inf'::Double, v1) OVER (PARTITION BY v1) FROM t1;
at test_files/window.slt:4895
Error: Execution("1 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`
Caused by:
process didn't exit successfully: `/Users/buraksen/d/datafusion/target/debug/deps/sqllogictests-267639e9b963ccc2 window` (exit status: 1)
```
But it should not succeed since:
```
let n =
match get_scalar_value_from_args(partition_evaluator_args.input_exprs(), 1)?
.map(get_signed_integer)
{
Some(Ok(n)) => {
if partition_evaluator_args.is_reversed() {
-n
} else {
n
}
}
_ => {
return exec_err!(
"Expected a signed integer literal for the second argument of nth_value"
)
}
};
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829223868
##########
datafusion/expr/src/expr.rs:
##########
@@ -783,9 +764,11 @@ impl From<Arc<WindowUDF>> for WindowFunctionDefinition {
/// ```
/// # use datafusion_expr::{Expr, BuiltInWindowFunction, col, ExprFunctionExt};
/// # use datafusion_expr::expr::WindowFunction;
-/// // Create FIRST_VALUE(a) OVER (PARTITION BY b ORDER BY c)
+/// use datafusion_expr::test::function_stub::count_udaf;
+/// use datafusion_expr::WindowFunctionDefinition::{AggregateUDF};
+/// // Create COUNT(a) OVER (PARTITION BY b ORDER BY c)
/// let expr = Expr::WindowFunction(
-/// WindowFunction::new(BuiltInWindowFunction::FirstValue, vec![col("a")])
+/// WindowFunction::new(AggregateUDF(count_udaf()), vec![col("a")])
Review Comment:
The equivalent here is `first_value_udwf()`.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836121534
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -120,12 +120,35 @@ from aggregate_test_100
order by c9
# WindowFunction with BuiltInWindowFunction wrong signature
-statement error DataFusion error: Error during planning: No function matches the given name and argument types 'NTH_VALUE\(Int32, Int64, Int64\)'\. You might need to add explicit type casts\.\n\tCandidate functions:\n\tNTH_VALUE\(Any, Any\)
+statement error
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
from aggregate_test_100
order by c9
+----
+DataFusion error: Error during planning: Error during planning: Coercion from [Int32, Int64, Int64] to the signature OneOf([Any(0), Any(1), Any(2)]) failed. No function matches the given name and argument types 'nth_value(Int32, Int64, Int64)'. You might need to add explicit type casts.
+ Candidate functions:
+ nth_value()
+ nth_value(Any)
+ nth_value(Any, Any)
+from aggregate_test_100
+order by c9
+
+
+statement error DataFusion error: Error during planning: Invalid function 'nth_vlue'.\nDid you mean 'nth_value'?
Review Comment:
👍
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1835766292
##########
datafusion/proto/proto/datafusion.proto:
##########
@@ -507,17 +507,6 @@ message ScalarUDFExprNode {
enum BuiltInWindowFunction {
UNSPECIFIED = 0; // https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum
- // ROW_NUMBER = 0;
- // RANK = 1;
- // DENSE_RANK = 2;
- // PERCENT_RANK = 3;
- // CUME_DIST = 4;
- // NTILE = 5;
- // LAG = 6;
- // LEAD = 7;
- FIRST_VALUE = 8;
- LAST_VALUE = 9;
- NTH_VALUE = 10;
Review Comment:
commented out the enums
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836543691
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -272,31 +269,34 @@ fn roundtrip_window() -> Result<()> {
let field_b = Field::new("b", DataType::Int64, false);
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
- let window_frame = WindowFrame::new_bounds(
+ let _window_frame = WindowFrame::new_bounds(
datafusion_expr::WindowFrameUnits::Range,
WindowFrameBound::Preceding(ScalarValue::Int64(None)),
WindowFrameBound::CurrentRow,
);
- let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
- Arc::new(NthValue::first(
- "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
- col("a", &schema)?,
- DataType::Int64,
- false,
- )),
- &[col("b", &schema)?],
- &LexOrdering{
- inner: vec![PhysicalSortExpr {
- expr: col("a", &schema)?,
- options: SortOptions {
- descending: false,
- nulls_first: false,
- },
- }]
- },
- Arc::new(window_frame),
- ));
+ // let udwf_window_expr = Expr::WindowFunction(WindowFunction::new(
+ // WindowFunctionDefinition::WindowUDF(first_value_udwf()),
+
+ // let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
+ // Arc::new(NthValue::first(
+ // "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
+ // col("a", &schema)?,
+ // DataType::Int64,
+ // false,
+ // )),
+ // &[col("b", &schema)?],
+ // &LexOrdering{
+ // inner: vec![PhysicalSortExpr {
Review Comment:
This does not work because we can't convert to proto. The issue is that we have the following definition:
```
/// Nested message and enum types in `PhysicalWindowExprNode`.
pub mod physical_window_expr_node {
#[derive(Clone, PartialEq, ::prost::Oneof)]
pub enum WindowFunction {
#[prost(enumeration = "super::BuiltInWindowFunction", tag = "2")]
BuiltInFunction(i32),
#[prost(string, tag = "3")]
UserDefinedAggrFunction(::prost::alloc::string::String),
}
}
```
It can be UDAF or BuiltinFunction. This deletion https://github.com/apache/datafusion/pull/13201/files#r1836542435, causes the issue. What should I do here? I think deleting BuiltinWindowExpr was ok but there is nothing to replace here AFAIS
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2451704154
> > It got pretty out of hand because of nth_value and BuiltinWindowFunction references across the repo so I wanted to open a draft PR to get feedback.
>
> I personally think it would be fine to leave `BuiltInWindowFunction` initially while we port over the function, and them remove it as a follow on PR
>
> Perhaps you can leave a stub in like
>
> ```rust
> enum BuiltInWindowFunction {
> // Never created, will be removed in a follow on PR
> Stub
> };
> ```
>
> Then we can focus this PR on making sure that `nth_value` is working as a user defined function and then move on in the next PR to mechanically ripping out the old BuiltInWindowFunction
Thanks @alamb will continue with what you've said
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836143061
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1500,129 +1494,6 @@ mod tests {
Ok(source)
}
- // Tests NTH_VALUE(negative index) with memoize feature.
- // To be able to trigger memoize feature for NTH_VALUE we need to
- // - feed BoundedWindowAggExec with batch stream data.
- // - Window frame should contain UNBOUNDED PRECEDING.
- // It hard to ensure these conditions are met, from the sql query.
- #[tokio::test]
- async fn test_window_nth_value_bounded_memoize() -> Result<()> {
Review Comment:
Thanks a lot @jcsherin. I will apply this changes and try to wrap up 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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2465537813
Thanks @buraksenn -- I eagerly await this PR (as it will let us close out two projects and claim success). Very excited
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1835784248
##########
datafusion/core/tests/fuzz_cases/window_fuzz.rs:
##########
@@ -415,36 +414,6 @@ fn get_random_function(
),
);
}
- window_fn_map.insert(
- "first_value",
- (
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::FirstValue,
- ),
- vec![arg.clone()],
- ),
- );
- window_fn_map.insert(
- "last_value",
- (
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::LastValue,
- ),
- vec![arg.clone()],
- ),
- );
- window_fn_map.insert(
- "nth_value",
- (
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::NthValue,
- ),
- vec![
- arg.clone(),
- lit(ScalarValue::Int64(Some(rng.gen_range(1..10)))),
- ],
- ),
- );
Review Comment:
done thanks
##########
datafusion/core/src/dataframe/mod.rs:
##########
@@ -2172,31 +2171,6 @@ mod tests {
Ok(())
}
- #[tokio::test]
- async fn select_with_window_exprs() -> Result<()> {
- // build plan using Table API
- let t = test_table().await?;
- let first_row = Expr::WindowFunction(WindowFunction::new(
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::FirstValue,
- ),
Review Comment:
done thanks
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2457564487
Thank you @jcsherin and @buraksenn -- this looks so close 🙏
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836689833
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -0,0 +1,555 @@
+// 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.
+
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
+
+use std::any::Any;
+use std::cmp::Ordering;
+use std::fmt::Debug;
+use std::ops::Range;
+use std::sync::OnceLock;
+
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_datafusion_err, exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
+use datafusion_expr::window_state::WindowAggState;
+use datafusion_expr::{
+ Documentation, Literal, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+get_or_init_udwf!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+get_or_init_udwf!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+get_or_init_udwf!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
+
+/// Create an expression to represent the `first_value` window function
+///
+pub fn first_value(arg: datafusion_expr::Expr) -> datafusion_expr::Expr {
+ first_value_udwf().call(vec![arg])
+}
+
+/// Create an expression to represent the `last_value` window function
+///
+pub fn last_value(arg: datafusion_expr::Expr) -> datafusion_expr::Expr {
+ last_value_udwf().call(vec![arg])
+}
+
+/// Create an expression to represent the `nth_value` window function
+///
+pub fn nth_value(arg: datafusion_expr::Expr, n: Option<i64>) -> datafusion_expr::Expr {
Review Comment:
Fixed and added test
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2468239357
@buraksenn Tremendous effort 🙌. These changes look good to me.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2472615041
Awesome -- thank you so much. I will review this PR hopefully later today
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829221878
##########
datafusion/expr/src/expr.rs:
##########
@@ -693,9 +689,6 @@ impl AggregateFunction {
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash)]
/// Defines which implementation of an aggregate function DataFusion should call.
pub enum WindowFunctionDefinition {
- /// A built in aggregate function that leverages an aggregate function
- /// A a built-in window function
- BuiltInWindowFunction(BuiltInWindowFunction),
Review Comment:
Let us keep this PR limited to converting `nth_value`. We can handle cleanup of `BuiltInWindowFunction` in a follow-up PR.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1835785304
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1500,129 +1494,6 @@ mod tests {
Ok(source)
}
- // Tests NTH_VALUE(negative index) with memoize feature.
- // To be able to trigger memoize feature for NTH_VALUE we need to
- // - feed BoundedWindowAggExec with batch stream data.
- // - Window frame should contain UNBOUNDED PRECEDING.
- // It hard to ensure these conditions are met, from the sql query.
- #[tokio::test]
- async fn test_window_nth_value_bounded_memoize() -> Result<()> {
Review Comment:
same as: https://github.com/apache/datafusion/pull/13201#discussion_r1835785085
I could not build a valid physical layer window expression that would work. Can I get help on this too if possible @jcsherin @berkaysynnada ? An example would be great
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2455620360
@jcsherin thanks for the very detailed explanation. In this case, I think it would be better to update WindowUDFImpl in a followup PR for enhancement right? I can skip this test case in the scope of this PR. Correct me if I'm wrong please
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2455422680
TL;DR
For invalid input expressions, built-in window functions fail early when converting logical plan to physical plan. But user-defined window functions will complete planning, and fail only during physical execution.
Validation of input expressions in user-defined window runs only during physical execution.
Is it not better for udwf to fail early when converting to physical plan?
That would require updating `WindowUDFImpl` so that we parse input expressions when creating the window expressions (similar to built-in window functions). And hook it here so argument parsing happens earlier here:
https://github.com/apache/datafusion/blob/b61b2fc7163f8a219de092d54a3712dd16725820/datafusion/physical-plan/src/windows/mod.rs#L158-L164
### Edge Case: Empty Table
```sql
DataFusion CLI v42.2.0
> CREATE TABLE t1(v1 BIGINT);
0 row(s) fetched.
Elapsed 0.020 seconds.
```
There are currently no rows in `t1` so an early return happens in `WindowAggStream::compute_aggregates`.
https://github.com/apache/datafusion/blob/89e96b404f07900469fee31740f43edd8a410a10/datafusion/physical-plan/src/windows/window_agg_exec.rs#L319-L321
The `nth_value` window function is never executed when this query is run on the empty table `t1`.
```sql
> SELECT NTH_VALUE('+Inf'::Double, v1) OVER (PARTITION BY v1) FROM t1;
+-------------------------------------------------------------------------------------------------------------+
| nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING |
+-------------------------------------------------------------------------------------------------------------+
+-------------------------------------------------------------------------------------------------------------+
0 row(s) fetched.
Elapsed 0.018 seconds.
```
After we insert a few values into `t1` the `nth_value` aggregation will be computed. Only then do we see the expected error message,
```sql
> insert into t1 values (123), (456);
+-------+
| count |
+-------+
| 2 |
+-------+
1 row(s) fetched.
Elapsed 0.007 seconds.
> SELECT NTH_VALUE('+Inf'::Double, v1) OVER (PARTITION BY v1) FROM t1;
This feature is not implemented: There is only support Literal types for field at idx: 1 in Window Function
```
### Planning divergence between built-in & user-defined window functions
In `main` branch where `nth_value` is a `BuiltInWindowFunction` the argument parsing fails early when mapping logical to physical plan. In explain output there is no physical plan.
```sql
> EXPLAIN SELECT NTH_VALUE('+Inf'::Double, v1) OVER (PARTITION BY v1) FROM t1;
+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type | plan |
+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| logical_plan | Projection: NTH_VALUE(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING |
| | WindowAggr: windowExpr=[[NTH_VALUE(Float64(inf), t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING AS NTH_VALUE(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]] |
| | TableScan: t1 projection=[v1] |
+--------------+----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row(s) fetched.
Elapsed 0.009 seconds.
```
But this is not the case for user-defined window functions. In this branch we instead see that a complete plan is built and failure is happening only when the query executes,
```sql
> EXPLAIN SELECT NTH_VALUE('+Inf'::Double, v1) OVER (PARTITION BY v1) FROM t1;
+---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| plan_type | plan |
+---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| logical_plan | Projection: nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING |
| | WindowAggr: windowExpr=[[nth_value(Float64(inf), t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING AS nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING]] |
| | TableScan: t1 projection=[v1] |
| physical_plan | ProjectionExec: expr=[nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING@1 as nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING] |
| | WindowAggExec: wdw=[nth_value(Utf8("+Inf"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING: Ok(Field { name: "nth_value(Utf8(\"+Inf\"),t1.v1) PARTITION BY [t1.v1] ROWS BETWEEN UNBOUNDED PRECEDING AND UNBOUNDED FOLLOWING", data_type: Float64, nullable: true, dict_id: 0, dict_is_ordered: false, metadata: {} }), frame: WindowFrame { units: Rows, start_bound: Preceding(UInt64(NULL)), end_bound: Following(UInt64(NULL)), is_causal: false }] |
| | SortExec: expr=[v1@0 ASC NULLS LAST], preserve_partitioning=[false] |
| | MemoryExec: partitions=1, partition_sizes=[0] |
| | |
+---------------+-----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
2 row(s) fetched.
Elapsed 0.019 seconds.
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829269713
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1500,129 +1494,6 @@ mod tests {
Ok(source)
}
- // Tests NTH_VALUE(negative index) with memoize feature.
- // To be able to trigger memoize feature for NTH_VALUE we need to
- // - feed BoundedWindowAggExec with batch stream data.
- // - Window frame should contain UNBOUNDED PRECEDING.
- // It hard to ensure these conditions are met, from the sql query.
- #[tokio::test]
- async fn test_window_nth_value_bounded_memoize() -> Result<()> {
Review Comment:
This test should not be removed either.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836593884
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -0,0 +1,555 @@
+// 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.
+
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
+
+use std::any::Any;
+use std::cmp::Ordering;
+use std::fmt::Debug;
+use std::ops::Range;
+use std::sync::OnceLock;
+
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_datafusion_err, exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
+use datafusion_expr::window_state::WindowAggState;
+use datafusion_expr::{
+ Documentation, Literal, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+get_or_init_udwf!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+get_or_init_udwf!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+get_or_init_udwf!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
+
+/// Create an expression to represent the `first_value` window function
+///
+pub fn first_value(arg: datafusion_expr::Expr) -> datafusion_expr::Expr {
+ first_value_udwf().call(vec![arg])
+}
+
+/// Create an expression to represent the `last_value` window function
+///
+pub fn last_value(arg: datafusion_expr::Expr) -> datafusion_expr::Expr {
+ last_value_udwf().call(vec![arg])
+}
+
+/// Create an expression to represent the `nth_value` window function
+///
+pub fn nth_value(arg: datafusion_expr::Expr, n: Option<i64>) -> datafusion_expr::Expr {
Review Comment:
The type of `n` is `i64`, not `Option<i64>`.
See the rust docs: [https://docs.rs/datafusion/latest/datafusion/logical_expr/window_function/fn.nth_value.html](https://docs.rs/datafusion/latest/datafusion/logical_expr/window_function/fn.nth_value.html)
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836490951
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1500,129 +1494,6 @@ mod tests {
Ok(source)
}
- // Tests NTH_VALUE(negative index) with memoize feature.
- // To be able to trigger memoize feature for NTH_VALUE we need to
- // - feed BoundedWindowAggExec with batch stream data.
- // - Window frame should contain UNBOUNDED PRECEDING.
- // It hard to ensure these conditions are met, from the sql query.
- #[tokio::test]
- async fn test_window_nth_value_bounded_memoize() -> Result<()> {
Review Comment:
This test passes 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] Convert nth_value builtIn function to User Defined Window Function [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1840604599
##########
datafusion/physical-plan/Cargo.toml:
##########
@@ -70,6 +70,7 @@ tokio = { workspace = true }
[dev-dependencies]
criterion = { version = "0.5", features = ["async_futures"] }
datafusion-functions-aggregate = { workspace = true }
+datafusion-functions-window = { workspace = true }
Review Comment:
some day I hope we can remove these dependencies (so we can make testing `physical-plan` faster, but not a part of this PR
##########
datafusion/physical-plan/src/windows/mod.rs:
##########
@@ -241,14 +171,29 @@ fn create_udwf_window_expr(
.map(|arg| arg.data_type(input_schema))
.collect::<Result<_>>()?;
- Ok(Arc::new(WindowUDFExpr {
+ let udwf_expr = Arc::new(WindowUDFExpr {
fun: Arc::clone(fun),
args: args.to_vec(),
input_types,
name,
is_reversed: false,
ignore_nulls,
- }))
+ });
+
+ // Early validation of input expressions
+ // We create a partition evaluator because in the user-defined window
+ // implementation this is where code for parsing input expressions
+ // exist. The benefits are:
+ // - If any of the input expressions are invalid we catch them early
Review Comment:
💯 for these comments that explain the rationale
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829235397
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -15,143 +15,254 @@
// specific language governing permissions and limitations
// under the License.
-//! Defines physical expressions for `FIRST_VALUE`, `LAST_VALUE`, and `NTH_VALUE`
-//! functions that can be evaluated at run time during query execution.
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
use std::any::Any;
use std::cmp::Ordering;
+use std::fmt::Debug;
use std::ops::Range;
-use std::sync::Arc;
-
-use crate::window::window_expr::{NthValueKind, NthValueState};
-use crate::window::BuiltInWindowFunctionExpr;
-use crate::PhysicalExpr;
+use std::sync::OnceLock;
-use arrow::array::{Array, ArrayRef};
-use arrow::datatypes::{DataType, Field};
-use datafusion_common::Result;
-use datafusion_common::ScalarValue;
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
use datafusion_expr::window_state::WindowAggState;
-use datafusion_expr::PartitionEvaluator;
+use datafusion_expr::{
+ Documentation, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+define_udwf_and_expr!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+define_udwf_and_expr!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+define_udwf_and_expr!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
Review Comment:
The macro generated expression API does not match what we need.
This is the removed expression API for `nth_value`. Also there doesn't seem to be separate APIs for `first_value` and `last_value`. So you do not need to create expression APIs for them.
```
/// Create an expression to represent the `nth_value` window function
pub fn nth_value(arg: Expr, n: i64) -> Expr {
Expr::WindowFunction(WindowFunction::new(
BuiltInWindowFunction::NthValue,
vec![arg, n.lit()],
))
}
```
This is the macro generated expression API:
```rust
#[doc = " Create a [`WindowFunction`](datafusion_expr::Expr::WindowFunction) expression for"]
#[doc = " `NthValue` user-defined window function."]
#[doc = ""]
#[doc = " returns the nth value in the window frame"]
pub fn nth_value() -> datafusion_expr::Expr {
nth_value_udwf().call(vec![])
}
```
The `define_udwf_and_expr` does not support generating expression functions which accepts arguments to code complexity low.
You can take a look at the code in `lead`/`lag` to see how to apply it here as well:
https://github.com/apache/datafusion/blob/9e636d8fe920340409e527da36cff3d2c25aef5f/datafusion/functions-window/src/lead_lag.rs#L40-L93
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2450656951
> It got pretty out of hand because of nth_value and BuiltinWindowFunction references across the repo so I wanted to open a draft PR to get feedback.
I personally think it would be fine to leave `BuiltInWindowFunction` initially while we port over the function, and them remove it as a follow on PR
Perhaps you can leave a stub in like
```rust
enum BuiltInWindowFunction {
// Never created, will be removed in a follow on PR
Stub
};
```
Then we can focus this PR on making sure that `nth_value` is working as a user defined function and then move on in the next PR to mechanically ripping out the old BuiltInWindowFunction
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2453934188
> External error: query failed: DataFusion error: Arrow error: Invalid argument error: It is not possible to concatenate arrays of different data types.
In the built-in (older) version the output field is defined like:
```rust
fn field(&self) -> Result<Field> {
let nullable = true;
Ok(Field::new(&self.name, self.data_type.clone(), nullable))
}
```
In the current code the implementation of field the data type of the field is hard-coded as `DataType::UInt64`:
```rust
fn field(&self, field_args: WindowUDFFieldArgs) -> Result<Field> {
let nullable = true;
Ok(Field::new(field_args.name(), DataType::UInt64, nullable))
}
```
To fix this use `field_args` to get the data type of the input expression rather than hard-coding `DataType::UInt64`.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829212628
##########
datafusion/expr/src/built_in_window_function.rs:
##########
@@ -17,115 +17,12 @@
//! Built-in functions module contains all the built-in functions definitions.
-use std::fmt;
-use std::str::FromStr;
-
-use crate::type_coercion::functions::data_types;
-use crate::utils;
-use crate::{Signature, Volatility};
-use datafusion_common::{plan_datafusion_err, plan_err, DataFusionError, Result};
-
-use arrow::datatypes::DataType;
-
use strum_macros::EnumIter;
-impl fmt::Display for BuiltInWindowFunction {
- fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
- write!(f, "{}", self.name())
- }
-}
-
/// A [window function] built in to DataFusion
///
/// [Window Function]: https://en.wikipedia.org/wiki/Window_function_(SQL)
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Hash, EnumIter)]
pub enum BuiltInWindowFunction {
- /// returns value evaluated at the row that is the first row of the window frame
- FirstValue,
- /// Returns value evaluated at the row that is the last row of the window frame
- LastValue,
- /// Returns value evaluated at the row that is the nth row of the window frame (counting from 1); returns null if no such row
- NthValue,
-}
-
-impl BuiltInWindowFunction {
- pub fn name(&self) -> &str {
- use BuiltInWindowFunction::*;
- match self {
- FirstValue => "first_value",
- LastValue => "last_value",
- NthValue => "NTH_VALUE",
- }
- }
-}
-
-impl FromStr for BuiltInWindowFunction {
- type Err = DataFusionError;
- fn from_str(name: &str) -> Result<BuiltInWindowFunction> {
- Ok(match name.to_uppercase().as_str() {
- "FIRST_VALUE" => BuiltInWindowFunction::FirstValue,
- "LAST_VALUE" => BuiltInWindowFunction::LastValue,
- "NTH_VALUE" => BuiltInWindowFunction::NthValue,
- _ => return plan_err!("There is no built-in window function named {name}"),
- })
- }
-}
-
-/// Returns the datatype of the built-in window function
-impl BuiltInWindowFunction {
- pub fn return_type(&self, input_expr_types: &[DataType]) -> Result<DataType> {
- // Note that this function *must* return the same type that the respective physical expression returns
- // or the execution panics.
-
- // Verify that this is a valid set of data types for this function
- data_types(input_expr_types, &self.signature())
- // Original errors are all related to wrong function signature
- // Aggregate them for better error message
- .map_err(|_| {
- plan_datafusion_err!(
- "{}",
- utils::generate_signature_error_msg(
- &format!("{self}"),
- self.signature(),
- input_expr_types,
- )
- )
- })?;
-
- match self {
- BuiltInWindowFunction::FirstValue
- | BuiltInWindowFunction::LastValue
- | BuiltInWindowFunction::NthValue => Ok(input_expr_types[0].clone()),
- }
- }
-
- /// The signatures supported by the built-in window function `fun`.
- pub fn signature(&self) -> Signature {
- // Note: The physical expression must accept the type returned by this function or the execution panics.
- match self {
- BuiltInWindowFunction::FirstValue | BuiltInWindowFunction::LastValue => {
- Signature::any(1, Volatility::Immutable)
- }
- BuiltInWindowFunction::NthValue => Signature::any(2, Volatility::Immutable),
- }
- }
-}
-
-#[cfg(test)]
-mod tests {
- use super::*;
- use strum::IntoEnumIterator;
- #[test]
- // Test for BuiltInWindowFunction's Display and from_str() implementations.
- // For each variant in BuiltInWindowFunction, it converts the variant to a string
- // and then back to a variant. The test asserts that the original variant and
- // the reconstructed variant are the same. This assertion is also necessary for
- // function suggestion. See https://github.com/apache/datafusion/issues/8082
- fn test_display_and_from_str() {
- for func_original in BuiltInWindowFunction::iter() {
- let func_name = func_original.to_string();
- let func_from_str = BuiltInWindowFunction::from_str(&func_name).unwrap();
- assert_eq!(func_from_str, func_original);
- }
- }
Review Comment:
It will be a good to convert this to a sqllogicttest now. For e.g.,
```sql
> SELECT
NTH_VLUE(c4, 2) OVER()
FROM aggregate_test_100
ORDER BY c9
LIMIT 5;
Error during planning: Invalid function 'nth_vlue'.
Did you mean 'nth_value'?
> SELECT
FRST_VALUE(c4, 2) OVER()
FROM aggregate_test_100
ORDER BY c9
LIMIT 5;
Error during planning: Invalid function 'frst_value'.
Did you mean 'first_value'?
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829235397
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -15,143 +15,254 @@
// specific language governing permissions and limitations
// under the License.
-//! Defines physical expressions for `FIRST_VALUE`, `LAST_VALUE`, and `NTH_VALUE`
-//! functions that can be evaluated at run time during query execution.
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
use std::any::Any;
use std::cmp::Ordering;
+use std::fmt::Debug;
use std::ops::Range;
-use std::sync::Arc;
-
-use crate::window::window_expr::{NthValueKind, NthValueState};
-use crate::window::BuiltInWindowFunctionExpr;
-use crate::PhysicalExpr;
+use std::sync::OnceLock;
-use arrow::array::{Array, ArrayRef};
-use arrow::datatypes::{DataType, Field};
-use datafusion_common::Result;
-use datafusion_common::ScalarValue;
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
use datafusion_expr::window_state::WindowAggState;
-use datafusion_expr::PartitionEvaluator;
+use datafusion_expr::{
+ Documentation, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+define_udwf_and_expr!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+define_udwf_and_expr!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+define_udwf_and_expr!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
Review Comment:
The macro generated expression API does not match what we need.
This is the removed expression API for `nth_value`.
```
/// Create an expression to represent the `nth_value` window function
pub fn nth_value(arg: Expr, n: i64) -> Expr {
Expr::WindowFunction(WindowFunction::new(
BuiltInWindowFunction::NthValue,
vec![arg, n.lit()],
))
}
```
This is the macro generated expression API:
```rust
#[doc = " Create a [`WindowFunction`](datafusion_expr::Expr::WindowFunction) expression for"]
#[doc = " `NthValue` user-defined window function."]
#[doc = ""]
#[doc = " returns the nth value in the window frame"]
pub fn nth_value() -> datafusion_expr::Expr {
nth_value_udwf().call(vec![])
}
```
The `define_udwf_and_expr` does not support generating expression functions which accepts arguments (to keep code complexity low).
You can take a look at the code in `lead`/`lag` to see how to apply it here as well:
https://github.com/apache/datafusion/blob/9e636d8fe920340409e527da36cff3d2c25aef5f/datafusion/functions-window/src/lead_lag.rs#L40-L93
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829389055
##########
datafusion/sqllogictest/test_files/window.slt:
##########
@@ -4892,7 +4892,7 @@ DROP TABLE t1;
statement ok
CREATE TABLE t1(v1 BIGINT);
-query error DataFusion error: Execution error: Expected a signed integer literal for the second argument of nth_value, got v1@0
+query error DataFusion error: This feature is not implemented: There is only support Literal types for field at idx: 1 in Window Function
Review Comment:
The original error message is more specific. You can transform it by using `map_err` when invalid input is provided.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1835784628
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -15,143 +15,254 @@
// specific language governing permissions and limitations
// under the License.
-//! Defines physical expressions for `FIRST_VALUE`, `LAST_VALUE`, and `NTH_VALUE`
-//! functions that can be evaluated at run time during query execution.
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
use std::any::Any;
use std::cmp::Ordering;
+use std::fmt::Debug;
use std::ops::Range;
-use std::sync::Arc;
-
-use crate::window::window_expr::{NthValueKind, NthValueState};
-use crate::window::BuiltInWindowFunctionExpr;
-use crate::PhysicalExpr;
+use std::sync::OnceLock;
-use arrow::array::{Array, ArrayRef};
-use arrow::datatypes::{DataType, Field};
-use datafusion_common::Result;
-use datafusion_common::ScalarValue;
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
use datafusion_expr::window_state::WindowAggState;
-use datafusion_expr::PartitionEvaluator;
+use datafusion_expr::{
+ Documentation, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+define_udwf_and_expr!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+define_udwf_and_expr!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+define_udwf_and_expr!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
Review Comment:
I think this is fixed now without detailed explanation which I will add if it all works
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836112390
##########
datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs:
##########
@@ -1500,129 +1494,6 @@ mod tests {
Ok(source)
}
- // Tests NTH_VALUE(negative index) with memoize feature.
- // To be able to trigger memoize feature for NTH_VALUE we need to
- // - feed BoundedWindowAggExec with batch stream data.
- // - Window frame should contain UNBOUNDED PRECEDING.
- // It hard to ensure these conditions are met, from the sql query.
- #[tokio::test]
- async fn test_window_nth_value_bounded_memoize() -> Result<()> {
Review Comment:
~~I reused `create_udwf_window_expr` by changing the visibility to current module to build the physical expressions required for this test.~~
~~You may apply this patch as-is and the test will pass.~~
In [5c4e0f6](https://github.com/apache/datafusion/pull/13201/commits/5c4e0f6ec67945b0e6f70662b5fd75513f77af65) the visibility of `create_udwf_window_expr` is changed to `pub` so that it can be reused in physical plan roundtrip test.
Updated patch:
```patch
diff --git a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
index 0a898bd85..a031253e4 100644
--- a/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
+++ b/datafusion/physical-plan/src/windows/bounded_window_agg_exec.rs
@@ -1163,7 +1163,9 @@ mod tests {
use crate::expressions::PhysicalSortExpr;
use crate::projection::ProjectionExec;
use crate::streaming::{PartitionStream, StreamingTableExec};
- use crate::windows::{create_window_expr, BoundedWindowAggExec, InputOrderMode};
+ use crate::windows::{
+ create_udwf_window_expr, create_window_expr, BoundedWindowAggExec, InputOrderMode,
+ };
use crate::{execute_stream, get_plan_string, ExecutionPlan};
use arrow_array::builder::{Int64Builder, UInt64Builder};
@@ -1180,13 +1182,14 @@ mod tests {
WindowFrame, WindowFrameBound, WindowFrameUnits, WindowFunctionDefinition,
};
use datafusion_functions_aggregate::count::count_udaf;
- use datafusion_functions_window::nth_value::first_value_udwf;
use datafusion_functions_window::nth_value::last_value_udwf;
use datafusion_functions_window::nth_value::nth_value_udwf;
- use datafusion_physical_expr::expressions::{col, lit, Column};
+ use datafusion_physical_expr::expressions::{col, Column, Literal};
use datafusion_physical_expr::{LexOrdering, PhysicalExpr};
use crate::memory::MemoryExec;
+ use crate::common::collect;
+ use datafusion_physical_expr::window::BuiltInWindowExpr;
use futures::future::Shared;
use futures::{pin_mut, ready, FutureExt, Stream, StreamExt};
use itertools::Itertools;
@@ -1524,36 +1527,35 @@ mod tests {
)
.map(|e| Arc::new(e) as Arc<dyn ExecutionPlan>)?;
let col_a = col("a", &schema)?;
- let nth_value_func1 = NthValue::nth(
- "nth_value(-1)",
- Arc::clone(&col_a),
- DataType::Int32,
- 1,
+ let nth_value_func1 = create_udwf_window_expr(
+ &nth_value_udwf(),
+ &[Arc::clone(&col_a), Arc::new(Literal::new(ScalarValue::Int32(Some(1))))],
+ &schema,
+ "nth_value(-1)".to_string(),
false,
)?
.reverse_expr()
.unwrap();
- let nth_value_func2 = NthValue::nth(
- "nth_value(-2)",
- Arc::clone(&col_a),
- DataType::Int32,
- 2,
+ let nth_value_func2 = create_udwf_window_expr(
+ &nth_value_udwf(),
+ &[Arc::clone(&col_a), Arc::new(Literal::new(ScalarValue::Int32(Some(2))))],
+ &schema,
+ "nth_value(-2)".to_string(),
false,
- )?
- .reverse_expr()
- .unwrap();
- let last_value_func = Arc::new(NthValue::last(
- "last",
- Arc::clone(&col_a),
- DataType::Int32,
+ )?.reverse_expr().unwrap();
+ let last_value_func = create_udwf_window_expr(
+ &last_value_udwf(),
+ &[Arc::clone(&col_a)],
+ &schema,
+ "last".to_string(),
false,
- )) as _;
+ )?;
let window_exprs = vec![
// LAST_VALUE(a)
Arc::new(BuiltInWindowExpr::new(
last_value_func,
&[],
- &LexOrdering::default(),
+ &LexOrdering::default(),
Arc::new(WindowFrame::new_bounds(
WindowFrameUnits::Rows,
WindowFrameBound::Preceding(ScalarValue::UInt64(None)),
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836710920
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -911,6 +912,9 @@ async fn roundtrip_expr_api() -> Result<()> {
count_distinct(lit(1)),
first_value(lit(1), None),
first_value(lit(1), Some(vec![lit(2).sort(true, true)])),
+ functions_window::nth_value::first_value(lit(1)),
+ functions_window::nth_value::last_value(lit(1)),
Review Comment:
Yes also existing nth_value was an aggregate function not window 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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836725370
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -119,13 +119,36 @@ regr_slope(c11, '2') over () as min1
from aggregate_test_100
order by c9
-# WindowFunction with BuiltInWindowFunction wrong signature
-statement error DataFusion error: Error during planning: No function matches the given name and argument types 'NTH_VALUE\(Int32, Int64, Int64\)'\. You might need to add explicit type casts\.\n\tCandidate functions:\n\tNTH_VALUE\(Any, Any\)
+# WindowFunction wrong signature
+statement error
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
from aggregate_test_100
order by c9
+----
+DataFusion error: Error during planning: Error during planning: Coercion from [Int32, Int64, Int64] to the signature OneOf([Any(0), Any(1), Any(2)]) failed. No function matches the given name and argument types 'nth_value(Int32, Int64, Int64)'. You might need to add explicit type casts.
+ Candidate functions:
+ nth_value()
+ nth_value(Any)
+ nth_value(Any, Any)
Review Comment:
This multiline format worked (for me) in local but it is failing in CI. The following change works (again for me) in local. I guess it will pass in CI as well.
```suggestion
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int32, Int64, Int64\] to the signature OneOf\(\[Any\(0\), Any\(1\), Any\(2\)\]\) failed
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
from aggregate_test_100
order by c9
```
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2450651884
THis is so exciting. FYI @jonathanc-n and @Omega359
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836137334
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -272,31 +269,34 @@ fn roundtrip_window() -> Result<()> {
let field_b = Field::new("b", DataType::Int64, false);
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
- let window_frame = WindowFrame::new_bounds(
+ let _window_frame = WindowFrame::new_bounds(
datafusion_expr::WindowFrameUnits::Range,
WindowFrameBound::Preceding(ScalarValue::Int64(None)),
WindowFrameBound::CurrentRow,
);
- let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
- Arc::new(NthValue::first(
- "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
- col("a", &schema)?,
- DataType::Int64,
- false,
- )),
- &[col("b", &schema)?],
- &LexOrdering{
- inner: vec![PhysicalSortExpr {
- expr: col("a", &schema)?,
- options: SortOptions {
- descending: false,
- nulls_first: false,
- },
- }]
- },
- Arc::new(window_frame),
- ));
+ // let udwf_window_expr = Expr::WindowFunction(WindowFunction::new(
+ // WindowFunctionDefinition::WindowUDF(first_value_udwf()),
+
+ // let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
+ // Arc::new(NthValue::first(
+ // "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
+ // col("a", &schema)?,
+ // DataType::Int64,
+ // false,
+ // )),
+ // &[col("b", &schema)?],
+ // &LexOrdering{
+ // inner: vec![PhysicalSortExpr {
Review Comment:
That one fails with:
```
to proto: NotImplemented("WindowExpr not supported: BuiltInWindowExpr { expr: WindowUDFExpr { fun: WindowUDF { inner: NthValue { signature: Signature { type_signature: OneOf([Any(0), Any(1), Any(2)]), volatility: Immutable }, kind: First } }, args: [Column { name: \"a\", index: 0 }], name: \"FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW\", input_types: [Int64], is_reversed: false, ignore_nulls: false }, partition_by: [Column { name: \"b\", index: 1 }], order_by: LexOrdering { inner: [PhysicalSortExpr { expr: Column { name: \"a\", index: 0 }, options: SortOptions { descending: false, nulls_first: false } }] }, window_frame: WindowFrame { units: Range, start_bound: Preceding(Int64(NULL)), end_bound: CurrentRow, is_causal: false } }")
```
Checking if I can fix it
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829235397
##########
datafusion/functions-window/src/nth_value.rs:
##########
@@ -15,143 +15,254 @@
// specific language governing permissions and limitations
// under the License.
-//! Defines physical expressions for `FIRST_VALUE`, `LAST_VALUE`, and `NTH_VALUE`
-//! functions that can be evaluated at run time during query execution.
+//! `nth_value` window function implementation
+
+use crate::utils::{get_scalar_value_from_args, get_signed_integer};
use std::any::Any;
use std::cmp::Ordering;
+use std::fmt::Debug;
use std::ops::Range;
-use std::sync::Arc;
-
-use crate::window::window_expr::{NthValueKind, NthValueState};
-use crate::window::BuiltInWindowFunctionExpr;
-use crate::PhysicalExpr;
+use std::sync::OnceLock;
-use arrow::array::{Array, ArrayRef};
-use arrow::datatypes::{DataType, Field};
-use datafusion_common::Result;
-use datafusion_common::ScalarValue;
+use datafusion_common::arrow::array::ArrayRef;
+use datafusion_common::arrow::datatypes::{DataType, Field};
+use datafusion_common::{exec_err, Result, ScalarValue};
+use datafusion_expr::window_doc_sections::DOC_SECTION_ANALYTICAL;
use datafusion_expr::window_state::WindowAggState;
-use datafusion_expr::PartitionEvaluator;
+use datafusion_expr::{
+ Documentation, PartitionEvaluator, ReversedUDWF, Signature, TypeSignature,
+ Volatility, WindowUDFImpl,
+};
+use datafusion_functions_window_common::field;
+use datafusion_functions_window_common::partition::PartitionEvaluatorArgs;
+use field::WindowUDFFieldArgs;
+
+define_udwf_and_expr!(
+ First,
+ first_value,
+ "returns the first value in the window frame",
+ NthValue::first
+);
+define_udwf_and_expr!(
+ Last,
+ last_value,
+ "returns the last value in the window frame",
+ NthValue::last
+);
+define_udwf_and_expr!(
+ NthValue,
+ nth_value,
+ "returns the nth value in the window frame",
+ NthValue::nth
+);
Review Comment:
The macro generated expression API does not match what we need.
This is the removed expression API for `nth_value`.
```
/// Create an expression to represent the `nth_value` window function
pub fn nth_value(arg: Expr, n: i64) -> Expr {
Expr::WindowFunction(WindowFunction::new(
BuiltInWindowFunction::NthValue,
vec![arg, n.lit()],
))
}
```
This is the macro generated expression API:
```rust
#[doc = " Create a [`WindowFunction`](datafusion_expr::Expr::WindowFunction) expression for"]
#[doc = " `NthValue` user-defined window function."]
#[doc = ""]
#[doc = " returns the nth value in the window frame"]
pub fn nth_value() -> datafusion_expr::Expr {
nth_value_udwf().call(vec![])
}
```
The `define_udwf_and_expr` does not support generating expression functions which accepts arguments to code complexity low.
You can take a look at the code in `lead`/`lag` to see how to apply it here as well:
https://github.com/apache/datafusion/blob/9e636d8fe920340409e527da36cff3d2c25aef5f/datafusion/functions-window/src/lead_lag.rs#L40-L93
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1829200222
##########
datafusion/core/tests/fuzz_cases/window_fuzz.rs:
##########
@@ -415,36 +414,6 @@ fn get_random_function(
),
);
}
- window_fn_map.insert(
- "first_value",
- (
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::FirstValue,
- ),
- vec![arg.clone()],
- ),
- );
- window_fn_map.insert(
- "last_value",
- (
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::LastValue,
- ),
- vec![arg.clone()],
- ),
- );
- window_fn_map.insert(
- "nth_value",
- (
- WindowFunctionDefinition::BuiltInWindowFunction(
- BuiltInWindowFunction::NthValue,
- ),
- vec![
- arg.clone(),
- lit(ScalarValue::Int64(Some(rng.gen_range(1..10)))),
- ],
- ),
- );
Review Comment:
Same applies here. Fix the test and not delete it.
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2453567445
Wanted to update here. I think I'm almost finished but probably encountered a side effect. This query fails in slt file:
```
External error: query failed: DataFusion error: Arrow error: Invalid argument error: It is not possible to concatenate arrays of different data types.
[SQL] select
c9,
sum(c5) over (order by c9) as sum1,
avg(c5) over (order by c9) as avg1,
count(c5) over (order by c9) as count1,
max(c5) over (order by c9) as max1,
min(c5) over (order by c9) as min1,
first_value(c5) over (order by c9) as fv1,
last_value(c5) over (order by c9) as lv1,
nth_value(c5, 2) over (order by c9) as nv1
from aggregate_test_100
order by c9
limit 5
at test_files/window.slt:97
Error: Execution("1 failures")
error: test failed, to rerun pass `-p datafusion-sqllogictest --test sqllogictests`
```
I hope to fix this and make this ready tomorrow
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "buraksenn (via GitHub)" <gi...@apache.org>.
buraksenn commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836665856
##########
datafusion/proto/tests/cases/roundtrip_physical_plan.rs:
##########
@@ -272,31 +269,34 @@ fn roundtrip_window() -> Result<()> {
let field_b = Field::new("b", DataType::Int64, false);
let schema = Arc::new(Schema::new(vec![field_a, field_b]));
- let window_frame = WindowFrame::new_bounds(
+ let _window_frame = WindowFrame::new_bounds(
datafusion_expr::WindowFrameUnits::Range,
WindowFrameBound::Preceding(ScalarValue::Int64(None)),
WindowFrameBound::CurrentRow,
);
- let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
- Arc::new(NthValue::first(
- "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
- col("a", &schema)?,
- DataType::Int64,
- false,
- )),
- &[col("b", &schema)?],
- &LexOrdering{
- inner: vec![PhysicalSortExpr {
- expr: col("a", &schema)?,
- options: SortOptions {
- descending: false,
- nulls_first: false,
- },
- }]
- },
- Arc::new(window_frame),
- ));
+ // let udwf_window_expr = Expr::WindowFunction(WindowFunction::new(
+ // WindowFunctionDefinition::WindowUDF(first_value_udwf()),
+
+ // let builtin_window_expr = Arc::new(BuiltInWindowExpr::new(
+ // Arc::new(NthValue::first(
+ // "FIRST_VALUE(a) PARTITION BY [b] ORDER BY [a ASC NULLS LAST] RANGE BETWEEN UNBOUNDED PRECEDING AND CURRENT ROW",
+ // col("a", &schema)?,
+ // DataType::Int64,
+ // false,
+ // )),
+ // &[col("b", &schema)?],
+ // &LexOrdering{
+ // inner: vec![PhysicalSortExpr {
Review Comment:
Yes, that's what I've been saying actually. Then I'm deleting builtin part from test
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836709044
##########
datafusion/proto/tests/cases/roundtrip_logical_plan.rs:
##########
@@ -911,6 +912,9 @@ async fn roundtrip_expr_api() -> Result<()> {
count_distinct(lit(1)),
first_value(lit(1), None),
first_value(lit(1), Some(vec![lit(2).sort(true, true)])),
+ functions_window::nth_value::first_value(lit(1)),
+ functions_window::nth_value::last_value(lit(1)),
Review Comment:
These expression functions (`first_value` and `last_value`) did not exist before. There is only `nth_value` here:
https://docs.rs/datafusion/latest/datafusion/logical_expr/window_function/index.html#functions
I guess it is alright.
Thanks for adding them 👍
--
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] Convert nth_value builtIn function to UDWF [datafusion]
Posted by "jcsherin (via GitHub)" <gi...@apache.org>.
jcsherin commented on code in PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#discussion_r1836725370
##########
datafusion/sqllogictest/test_files/errors.slt:
##########
@@ -119,13 +119,36 @@ regr_slope(c11, '2') over () as min1
from aggregate_test_100
order by c9
-# WindowFunction with BuiltInWindowFunction wrong signature
-statement error DataFusion error: Error during planning: No function matches the given name and argument types 'NTH_VALUE\(Int32, Int64, Int64\)'\. You might need to add explicit type casts\.\n\tCandidate functions:\n\tNTH_VALUE\(Any, Any\)
+# WindowFunction wrong signature
+statement error
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
from aggregate_test_100
order by c9
+----
+DataFusion error: Error during planning: Error during planning: Coercion from [Int32, Int64, Int64] to the signature OneOf([Any(0), Any(1), Any(2)]) failed. No function matches the given name and argument types 'nth_value(Int32, Int64, Int64)'. You might need to add explicit type casts.
+ Candidate functions:
+ nth_value()
+ nth_value(Any)
+ nth_value(Any, Any)
Review Comment:
This format worked (for me) in local but it is failing in CI. The following change works (again for me) in local. I guess it will pass in CI as well.
```suggestion
statement error DataFusion error: Error during planning: Error during planning: Coercion from \[Int32, Int64, Int64\] to the signature OneOf\(\[Any\(0\), Any\(1\), Any\(2\)\]\) failed
select
c9,
nth_value(c5, 2, 3) over (order by c9) as nv1
from aggregate_test_100
order by c9
```
--
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] Convert `nth_value` builtIn function to User Defined Window Function [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #13201:
URL: https://github.com/apache/datafusion/pull/13201
--
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] Convert `nth_value` builtIn function to User Defined Window Function [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2473979162
I also took the liberty of merging up from main to make sure we haven't hit any logical conflicts with this PR
--
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] Convert `nth_value` builtIn function to User Defined Window Function [datafusion]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #13201:
URL: https://github.com/apache/datafusion/pull/13201#issuecomment-2474063240
I don't think there is any reason to wait around for this PR -- people know it is coming, so let's get this in 🚀
--
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