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 GitH
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 GitH
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
-# Window
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
-# WindowF
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
-# WindowF
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)),
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 GitH
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)),
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 lice
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", Dat
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", Data
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 licen
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 licen
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", Dat
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,
) ->
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(n
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(n
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", Dat
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 wro
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(ne
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(ne
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(ne
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",
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(
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(n
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", Dat
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.
-
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.
-/
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 wron
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://p
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
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 specifi
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.
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
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 t
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 err
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
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.
-//
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.
-//
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://pr
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(ne
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.
-//
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.
-//
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> for WindowFunctionDefinition {
/// ```
/// # use datafusion_expr::{Expr, BuiltInWindowFunctio
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 imple
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.
-u
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(
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
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
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 plann
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 ca
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-i
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 (olde
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: Ar
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
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
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 ab
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
-
58 matches
Mail list logo
