feat: Initial support for Window function #599

huaxingao · 2024-06-27T17:31:58Z

Co-authored-by: comphead [email protected]
Co-authored-by: Huaxin Gao [email protected]

Which issue does this PR close?

Closes #.

Rationale for this change

What changes are included in this PR?

This PR has the initial changes for window function. Currently, only Count, Max and Min are supported. In the following PRs, we will add all the other aggregate functions support, built-in window function support, ignoreNulls support, ect.

How are these changes tested?

Co-authored-by: comphead <[email protected]>

kazuyukitanimura

I am not sure about all the corner cases, but looks good

kazuyukitanimura · 2024-06-27T21:59:19Z

core/src/execution/datafusion/planner.rs

+                    WindowFrameBound::Preceding(ScalarValue::UInt64(None))
+                }
+                LowerFrameBoundStruct::Preceding(offset) => {
+                    let offset_value = if offset.offset < 0 {


Perhaps we can use abs()?

Changed. Thanks!

kazuyukitanimura · 2024-06-27T22:00:26Z

spark/src/main/scala/org/apache/comet/CometSparkSessionExtensions.scala

@@ -541,6 +542,17 @@ class CometSparkSessionExtensions
          withInfo(s, Seq(info1, info2).flatten.mkString(","))
          s

+        case w: WindowExec =>


Should we add isCometOperatorEnabled() to enable/disable the feature?

Yes. I saw some of the isCometOperatorEnabled are checked in CometSparkSessionExtensions, but some of them are in QueryPlanSerde. I followed HashAggregateExec and added the check in QueryPlanSerde

kazuyukitanimura · 2024-06-27T22:03:16Z

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

+        withTable("t1") {
+          val numRows = 10
+          spark
+            .range(numRows)
+            .selectExpr("id AS a", s"$numRows - id AS b") // Todo: Test Nulls
+            .repartition(3) // Force repartition to test data will come to single partition
+            .write
+            .saveAsTable("t1")


This can be simplified a little with withParquetTable()

Changed to withParquetTable(). Thanks

comphead · 2024-06-28T16:35:54Z

core/src/execution/datafusion/planner.rs

+            sort_exprs,
+            window_frame.into(),
+            &input_schema,
+            false, // TODO: Ignore nulls


I'll create a follow up PR for this

comphead · 2024-06-28T16:39:52Z

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

+          val aggregateFunctions =
+            List("COUNT(_1)", "MAX(_1)", "MIN(_1)") // TODO: Test all the aggregates
+
+          aggregateFunctions.foreach { function =>


Thanks for making tests generic

comphead

lgtm thanks @huaxingao

comphead · 2024-06-28T16:41:25Z

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

+      withSQLConf(
+        CometConf.COMET_EXEC_SHUFFLE_ENABLED.key -> "true",
+        SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> aqeEnabled) {
+        withParquetTable((0 until 10).map(i => (i, 10 - i)), "t1") { // TODO: test nulls


File a ticket to support test nulls for window functions

comphead · 2024-06-28T16:43:12Z

spark/src/test/scala/org/apache/comet/exec/CometExecSuite.scala

+        SQLConf.ADAPTIVE_EXECUTION_ENABLED.key -> aqeEnabled) {
+        withParquetTable((0 until 10).map(i => (i, 10 - i)), "t1") { // TODO: test nulls
+          val aggregateFunctions =
+            List("COUNT(_1)", "MAX(_1)", "MIN(_1)") // TODO: Test all the aggregates


Followup to test all the aggregates functions for window clause

Still have problems with other aggregates functions. I will look one by one.
For example:

sum: Error from DataFusion: Internal error: Builtin Sum will be removed avg: org.apache.comet.CometNativeException: AvgAccumulator for (Int32 --> Float64)

huaxingao · 2024-06-28T17:40:51Z

also cc @andygrove @viirya

andygrove · 2024-07-01T23:37:43Z

spark/src/main/scala/org/apache/comet/serde/QueryPlanSerde.scala

+        case agg: AggregateExpression =>
+          Some(agg)


It would be better to fall back to Spark for aggregate expressions that we do not support, rather than fail during query execution.

Suggested change

case agg: AggregateExpression =>

Some(agg)

case agg: AggregateExpression => agg.aggregateFunction match {

case _: Min | _: Max | _: Count => Some(agg)

case _ =>

withInfo(windowExpr, "Unsupported aggregate", expr)

None

}

Fixed. Thanks!

andygrove

LGTM. Thanks @huaxingao

andygrove · 2024-07-02T15:13:33Z

@huaxingao We should also update the user guide to show that we now support window functions

huaxingao · 2024-07-02T15:27:50Z

I will update the user doc to add window function support.

huaxingao · 2024-07-02T15:28:25Z

Thanks everyone!

* feat: initial support for Window function Co-authored-by: comphead <[email protected]> * fix style * fix style * address comments * abs()->unsigned_abs() * address comments --------- Co-authored-by: comphead <[email protected]>

feat: initial support for Window function

e575d9c

Co-authored-by: comphead <[email protected]>

huaxingao force-pushed the window_1 branch from b244a83 to e575d9c Compare June 27, 2024 17:39

huaxingao added 2 commits June 27, 2024 11:52

fix style

1b174a0

fix style

ebd1964

kazuyukitanimura approved these changes Jun 27, 2024

View reviewed changes

huaxingao added 2 commits June 28, 2024 09:13

address comments

4011198

abs()->unsigned_abs()

4a9f605

comphead reviewed Jun 28, 2024

View reviewed changes

comphead mentioned this pull request Jun 28, 2024

[Windows] Add IGNORE NULLS support for windows #606

Open

comphead reviewed Jun 28, 2024

View reviewed changes

comphead approved these changes Jun 28, 2024

View reviewed changes

comphead mentioned this pull request Jun 28, 2024

[Windows] Support test nulls for window functions #607

Open

comphead reviewed Jun 28, 2024

View reviewed changes

This was referenced Jun 28, 2024

[Windows] Test all the aggregates functions for window clause #608

Open

[EPIC] Native execution for Windows functions #22

Open

andygrove reviewed Jul 1, 2024

View reviewed changes

address comments

eb13de2

andygrove approved these changes Jul 2, 2024

View reviewed changes

andygrove merged commit 0d2fcbc into apache:main Jul 2, 2024
66 checks passed

huaxingao deleted the window_1 branch July 2, 2024 15:28

comphead mentioned this pull request Jul 12, 2024

feat: Comet windows functions support #200

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Initial support for Window function #599

feat: Initial support for Window function #599

huaxingao commented Jun 27, 2024 •

edited

Loading

kazuyukitanimura left a comment

kazuyukitanimura Jun 27, 2024

huaxingao Jun 28, 2024

kazuyukitanimura Jun 27, 2024

huaxingao Jun 28, 2024

kazuyukitanimura Jun 27, 2024

huaxingao Jun 28, 2024

comphead Jun 28, 2024

comphead Jun 28, 2024

comphead Jun 28, 2024

comphead left a comment

comphead Jun 28, 2024

comphead Jun 28, 2024

comphead Jun 28, 2024

comphead Jun 28, 2024

huaxingao Jun 28, 2024

huaxingao commented Jun 28, 2024

andygrove Jul 1, 2024

huaxingao Jul 2, 2024

andygrove left a comment

andygrove commented Jul 2, 2024

huaxingao commented Jul 2, 2024

huaxingao commented Jul 2, 2024

-        case agg: AggregateExpression =>
-          Some(agg)
+        case agg: AggregateExpression => agg.aggregateFunction match {
+          case _: Min | _: Max | _: Count => Some(agg)
+          case _ =>
+            withInfo(windowExpr, "Unsupported aggregate", expr)
+            None
+        }

feat: Initial support for Window function #599

feat: Initial support for Window function #599

Conversation

huaxingao commented Jun 27, 2024 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

kazuyukitanimura left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

comphead left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huaxingao commented Jun 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andygrove left a comment

Choose a reason for hiding this comment

andygrove commented Jul 2, 2024

huaxingao commented Jul 2, 2024

huaxingao commented Jul 2, 2024

huaxingao commented Jun 27, 2024 •

edited

Loading