Add Std dev samp for windowing [databricks] #3869

razajafri · 2021-10-20T18:23:45Z

This PR adds windowing support for stddev. Its built on top of @revans2 changes for adding support for GpuReplaceWindowFunction

Added changes to AggregateFunction.scala for the actual Expression changes. Had to add a CudfWindowStddev class to avoid stack overflow when replacing StddevSamp
Added tests in window_function_test.py.

depends on rapidsai/cudf#9527

closes #3814

Signed-off-by: Raza Jafri [email protected]

abellina · 2021-10-20T21:29:21Z

There are known issues in this implementation.

Should we have it disabled by default for now? We should link the cuDF issue if there is one.

The NaNs and Nulls are not handled properly. The correct way to do this is to run two aggregations and then filter out the result.

Is this something you are planning to handle in the plugin? Or is that going to be handled under the hood by cuDF

Signed-off-by: Raza Jafri <[email protected]>

razajafri · 2021-10-26T21:18:23Z

Should we have it disabled by default for now? We should link the cuDF issue if there is one.

I kept it as a draft PR for this specific reason.

Is this something you are planning to handle in the plugin? Or is that going to be handled under the hood by cuDF

Yes, I have updated the PR to handle the Spark specific output in the plugin

razajafri · 2021-10-26T22:12:42Z

@abellina PTAL, I have cleaned up the PR and updated it against @revans2 Average PR

integration_tests/src/main/python/window_function_test.py

Signed-off-by: Raza Jafri <[email protected]>

integration_tests/src/main/python/window_function_test.py

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

integration_tests/src/main/python/window_function_test.py

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/AggregateFunctions.scala

Signed-off-by: Raza Jafri <[email protected]>

razajafri · 2021-10-29T00:48:32Z

build

Signed-off-by: Raza Jafri <[email protected]>

These changes were causing test_lead_lag_for_structs_with_arrays test to fail. I am not sure why but seems to be happening only for the case where int_gen is nested in the array_gen. Signed-off-by: Raza Jafri <[email protected]>

razajafri · 2021-10-29T20:21:19Z

build

razajafri · 2021-10-29T20:21:42Z

@revans2 can take another look?

revans2

Just one minor nit

revans2 · 2021-10-29T20:31:35Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/AggregateFunctions.scala


-  override lazy val evaluateExpression: Expression = {
+  override val evaluateExpression: Expression = {


nit: Can we add the lazy back in?

Signed-off-by: Raza Jafri <[email protected]>

razajafri · 2021-10-29T20:39:05Z

build

ttnghia · 2021-10-29T20:45:06Z

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuOverrides.scala

+        ExprChecks.groupByOnly(
+          TypeSig.DOUBLE, TypeSig.DOUBLE,
+          Seq(ParamCheck("input", TypeSig.DOUBLE,
+            TypeSig.DOUBLE))).asInstanceOf[ExprChecksImpl].contexts
+            ++
+            ExprChecks.windowOnly(


Should this be just aggNotReduction?

I didn't know about that ExprCheck. I think that's what we need, @revans2 do you see anything wrong with using aggNotReduction here?

aggNotReduction should do the same thing, just be cleaner.

Signed-off-by: Raza Jafri <[email protected]>

razajafri · 2021-11-02T18:16:13Z

build

razajafri · 2021-11-02T21:47:15Z

@ttnghia @revans2 can you please take another look

ttnghia · 2021-11-02T21:53:17Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/AggregateFunctions.scala

@@ -1204,8 +1204,27 @@ case class GpuStddevPop(child: Expression, nullOnDivideByZero: Boolean)
  override def prettyName: String = "stddev_pop"
 }

+case class WindowStddevSamp(


Are we going to support WindowStddevPop too?

Not in this PR

Why not? The std family can be supported all at once with very little code added. Otherwise, we may have new requests from customers to go back, read the code, add more code + tests etc... in a significantly much more time.

IMO adding all stddev_pop/samp and var_pop/samp at once is the most optimal way.

It might be very easy to do but that doesn't mean that we have to bundle them together. This PR is specific to a client's request and is scheduled for this release.

razajafri self-assigned this Oct 20, 2021

sameerz added the feature request New feature or request label Oct 20, 2021

razajafri force-pushed the SP-3814-stddev-windowing branch from f9ca4fb to 005ba4e Compare October 26, 2021 19:32

Stddev Windowing

1d94635

Signed-off-by: Raza Jafri <[email protected]>

razajafri force-pushed the SP-3814-stddev-windowing branch from 005ba4e to 1d94635 Compare October 26, 2021 21:10

razajafri changed the title ~~Add Std dev for windowing~~ Add Std dev for windowing [databricks] Oct 26, 2021

razajafri marked this pull request as ready for review October 26, 2021 21:16

razajafri marked this pull request as draft October 26, 2021 21:19

razajafri marked this pull request as ready for review October 27, 2021 15:08

revans2 reviewed Oct 27, 2021

View reviewed changes

razajafri added 2 commits October 27, 2021 21:08

addressed review comments

9dbe2a2

Signed-off-by: Raza Jafri <[email protected]>

removed row limit for tests

a94dfd4

Signed-off-by: Raza Jafri <[email protected]>

razajafri requested a review from revans2 October 28, 2021 04:11

razajafri added 2 commits October 27, 2021 21:16

added unbounded window

9efd0ba

Signed-off-by: Raza Jafri <[email protected]>

removed row limit from the range window test

a076d34

Signed-off-by: Raza Jafri <[email protected]>

revans2 reviewed Oct 28, 2021

View reviewed changes

addressed review comments

4aa270f

Signed-off-by: Raza Jafri <[email protected]>

razajafri requested a review from revans2 October 28, 2021 18:25

abellina self-requested a review October 29, 2021 16:13

razajafri added 2 commits October 29, 2021 11:02

Merge remote-tracking branch 'origin/branch-21.12' into HEAD

c512f44

Signed-off-by: Raza Jafri <[email protected]>

Reverting test changes

cc4ace4

These changes were causing test_lead_lag_for_structs_with_arrays test to fail. I am not sure why but seems to be happening only for the case where int_gen is nested in the array_gen. Signed-off-by: Raza Jafri <[email protected]>

revans2 previously approved these changes Oct 29, 2021

View reviewed changes

put lazy val back in for evaluation

d6d42cf

Signed-off-by: Raza Jafri <[email protected]>

razajafri dismissed revans2’s stale review via d6d42cf October 29, 2021 20:38

revans2 previously approved these changes Oct 29, 2021

View reviewed changes

ttnghia reviewed Oct 29, 2021

View reviewed changes

replace Expr Check with aggNotReduction

46b6013

Signed-off-by: Raza Jafri <[email protected]>

razajafri dismissed revans2’s stale review via 46b6013 November 2, 2021 18:16

ttnghia reviewed Nov 2, 2021

View reviewed changes

razajafri changed the title ~~Add Std dev for windowing [databricks]~~ Add Std dev samp for windowing [databricks] Nov 2, 2021

revans2 approved these changes Nov 3, 2021

View reviewed changes

abellina approved these changes Nov 3, 2021

View reviewed changes

razajafri merged commit cebcc1f into NVIDIA:branch-21.12 Nov 3, 2021

ttnghia mentioned this pull request Mar 28, 2023

[FEA] stddevsamp function #62

Closed

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Std dev samp for windowing [databricks] #3869

Add Std dev samp for windowing [databricks] #3869

razajafri commented Oct 20, 2021 •

edited

Loading

abellina commented Oct 20, 2021

razajafri commented Oct 26, 2021 •

edited

Loading

razajafri commented Oct 26, 2021

razajafri commented Oct 29, 2021

razajafri commented Oct 29, 2021

razajafri commented Oct 29, 2021

revans2 left a comment

revans2 Oct 29, 2021

razajafri commented Oct 29, 2021

ttnghia Oct 29, 2021 •

edited

Loading

razajafri Nov 2, 2021

revans2 Nov 2, 2021

razajafri commented Nov 2, 2021

razajafri commented Nov 2, 2021

ttnghia Nov 2, 2021

razajafri Nov 2, 2021

ttnghia Nov 2, 2021

razajafri Nov 2, 2021


		override lazy val evaluateExpression: Expression = {
		override val evaluateExpression: Expression = {

Add Std dev samp for windowing [databricks] #3869

Add Std dev samp for windowing [databricks] #3869

Conversation

razajafri commented Oct 20, 2021 • edited Loading

abellina commented Oct 20, 2021

razajafri commented Oct 26, 2021 • edited Loading

razajafri commented Oct 26, 2021

razajafri commented Oct 29, 2021

razajafri commented Oct 29, 2021

razajafri commented Oct 29, 2021

revans2 left a comment

Choose a reason for hiding this comment

revans2 Oct 29, 2021

Choose a reason for hiding this comment

razajafri commented Oct 29, 2021

ttnghia Oct 29, 2021 • edited Loading

Choose a reason for hiding this comment

razajafri Nov 2, 2021

Choose a reason for hiding this comment

revans2 Nov 2, 2021

Choose a reason for hiding this comment

razajafri commented Nov 2, 2021

razajafri commented Nov 2, 2021

ttnghia Nov 2, 2021

Choose a reason for hiding this comment

razajafri Nov 2, 2021

Choose a reason for hiding this comment

ttnghia Nov 2, 2021

Choose a reason for hiding this comment

razajafri Nov 2, 2021

Choose a reason for hiding this comment

razajafri commented Oct 20, 2021 •

edited

Loading

razajafri commented Oct 26, 2021 •

edited

Loading

ttnghia Oct 29, 2021 •

edited

Loading