[SPARK-44871][SQL] Fix percentile_disc behaviour #42559

peter-toth · 2023-08-18T13:12:20Z

What changes were proposed in this pull request?

This PR fixes percentile_disc() function as currently it returns inforrect results in some cases. E.g.:

SELECT
  percentile_disc(0.0) WITHIN GROUP (ORDER BY a) as p0,
  percentile_disc(0.1) WITHIN GROUP (ORDER BY a) as p1,
  percentile_disc(0.2) WITHIN GROUP (ORDER BY a) as p2,
  percentile_disc(0.3) WITHIN GROUP (ORDER BY a) as p3,
  percentile_disc(0.4) WITHIN GROUP (ORDER BY a) as p4,
  percentile_disc(0.5) WITHIN GROUP (ORDER BY a) as p5,
  percentile_disc(0.6) WITHIN GROUP (ORDER BY a) as p6,
  percentile_disc(0.7) WITHIN GROUP (ORDER BY a) as p7,
  percentile_disc(0.8) WITHIN GROUP (ORDER BY a) as p8,
  percentile_disc(0.9) WITHIN GROUP (ORDER BY a) as p9,
  percentile_disc(1.0) WITHIN GROUP (ORDER BY a) as p10
FROM VALUES (0), (1), (2), (3), (4) AS v(a)

currently returns:

+---+---+---+---+---+---+---+---+---+---+---+
| p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10|
+---+---+---+---+---+---+---+---+---+---+---+
|0.0|0.0|0.0|1.0|1.0|2.0|2.0|2.0|3.0|3.0|4.0|
+---+---+---+---+---+---+---+---+---+---+---+

but after this PR it returns the correct:

+---+---+---+---+---+---+---+---+---+---+---+
| p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10|
+---+---+---+---+---+---+---+---+---+---+---+
|0.0|0.0|0.0|1.0|1.0|2.0|2.0|3.0|3.0|4.0|4.0|
+---+---+---+---+---+---+---+---+---+---+---+

Why are the changes needed?

Bugfix.

Does this PR introduce any user-facing change?

Yes, fixes a correctness bug, but the old behaviour can be restored with spark.sql.legacy.percentileDiscCalculation=true.

How was this patch tested?

Added new UTs.

peter-toth · 2023-08-18T13:17:44Z

sql/core/src/test/resources/sql-tests/results/percentiles.sql.out

+-- !query schema
+struct<p0:double,p1:double,p2:double,p3:double,p4:double,p5:double,p6:double,p7:double,p8:double,p9:double,p10:double>
+-- !query output
+0.0	0.0	0.0	1.0	1.0	2.0	2.0	3.0	3.0	4.0	4.0


These results are verified with MSSQL and PostgreSQL.

jchen5

Can we also add a comment in the code explaining the semantics:

For percentile_disc, the percentile of a value is defined as the fraction of rows with values less than or equal to that value - so the lowest value has position 1/N if it's unique, and the last value has position 1. This is the percentile returned by cume_dist().

percentile_disc(x) is defined as the minimum value with cume_dist >= x.

jchen5 · 2023-08-18T13:25:30Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala

      // Linear interpolation to get the exact percentile
      (higher - position) * toDoubleValue(lowerKey) + (position - lower) * toDoubleValue(higherKey)
    }
  }


Can we add a comment in the old getPercentile discrete codepath that it's only used when the legacy flag is set

peter-toth · 2023-08-18T14:10:05Z

Thanks @jchen5. I've added some comments in 5ab5084, please let me know if we should add more details.

jchen5

Looks good to me, thanks!

amaliujia · 2023-08-18T17:33:57Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala


+  val LEGACY_PERCENTILE_DISC_CALCULATION = buildConf("spark.sql.legacy.percentileDiscCalculation")
+    .internal()
+    .doc("If true the old bogus percentile_disc calculation is used.")


Maybe adding a bit more explanation on what is the legacy behavior so in the future people do not need to go back to this PR to understand the context?

Added in 93a018c.

MaxGekk · 2023-08-21T06:55:02Z

...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/aggregate/percentiles.scala

+  private val legacyDiscCalculation: Boolean =
+    SQLConf.get.getConf(SQLConf.LEGACY_PERCENTILE_DISC_CALCULATION)


This might not work well, see:

the reasons for moving val to constructor params in [SPARK-30894][SQL] Make Size's nullable independent from SQL config changes #27658

[SPARK-30858][SQL] Make IntegralDivide's dataType independent from SQL config changes #27628 (comment)

Ah ok, thanks. I will move this to class param then.

I've refactored the PR a bit so that only PercentileDisc requires the new legacy flag.

peter-toth · 2023-08-21T12:27:31Z

sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala

+      "incorrectly mapped the requested percentile to the sorted range of values in some cases " +
+      "and so returned incorrect results. Also, the new implementation is faster as it doesn't " +
+      "contain the interpolation logic that the old percentile_cont based one did.")
+    .version("3.3.4")


This bug was introduced with the very first version of percentile_disc in 3.3.0: https://issues.apache.org/jira/browse/SPARK-37691 so 3.3.4 seems to be the earliest still active release where we should backport this fix to.

This PR fixes `percentile_disc()` function as currently it returns inforrect results in some cases. E.g.: ``` SELECT percentile_disc(0.0) WITHIN GROUP (ORDER BY a) as p0, percentile_disc(0.1) WITHIN GROUP (ORDER BY a) as p1, percentile_disc(0.2) WITHIN GROUP (ORDER BY a) as p2, percentile_disc(0.3) WITHIN GROUP (ORDER BY a) as p3, percentile_disc(0.4) WITHIN GROUP (ORDER BY a) as p4, percentile_disc(0.5) WITHIN GROUP (ORDER BY a) as p5, percentile_disc(0.6) WITHIN GROUP (ORDER BY a) as p6, percentile_disc(0.7) WITHIN GROUP (ORDER BY a) as p7, percentile_disc(0.8) WITHIN GROUP (ORDER BY a) as p8, percentile_disc(0.9) WITHIN GROUP (ORDER BY a) as p9, percentile_disc(1.0) WITHIN GROUP (ORDER BY a) as p10 FROM VALUES (0), (1), (2), (3), (4) AS v(a) ``` currently returns: ``` +---+---+---+---+---+---+---+---+---+---+---+ | p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10| +---+---+---+---+---+---+---+---+---+---+---+ |0.0|0.0|0.0|1.0|1.0|2.0|2.0|2.0|3.0|3.0|4.0| +---+---+---+---+---+---+---+---+---+---+---+ ``` but after this PR it returns the correct: ``` +---+---+---+---+---+---+---+---+---+---+---+ | p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10| +---+---+---+---+---+---+---+---+---+---+---+ |0.0|0.0|0.0|1.0|1.0|2.0|2.0|3.0|3.0|4.0|4.0| +---+---+---+---+---+---+---+---+---+---+---+ ``` Bugfix. Yes, fixes a correctness bug, but the old behaviour can be restored with `spark.sql.legacy.percentileDiscCalculation=true`. Added new UTs. Closes #42559 from peter-toth/SPARK-44871-fix-percentile-disc-behaviour. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com> (cherry picked from commit bd8fbf4) Signed-off-by: Peter Toth <peter.toth@gmail.com>

peter-toth · 2023-08-22T12:56:26Z

Thanks for the review! Merged to master/3.5. I will open a backport PR to 3.4 and 3.3 soon.

This PR fixes `percentile_disc()` function as currently it returns inforrect results in some cases. E.g.: ``` SELECT percentile_disc(0.0) WITHIN GROUP (ORDER BY a) as p0, percentile_disc(0.1) WITHIN GROUP (ORDER BY a) as p1, percentile_disc(0.2) WITHIN GROUP (ORDER BY a) as p2, percentile_disc(0.3) WITHIN GROUP (ORDER BY a) as p3, percentile_disc(0.4) WITHIN GROUP (ORDER BY a) as p4, percentile_disc(0.5) WITHIN GROUP (ORDER BY a) as p5, percentile_disc(0.6) WITHIN GROUP (ORDER BY a) as p6, percentile_disc(0.7) WITHIN GROUP (ORDER BY a) as p7, percentile_disc(0.8) WITHIN GROUP (ORDER BY a) as p8, percentile_disc(0.9) WITHIN GROUP (ORDER BY a) as p9, percentile_disc(1.0) WITHIN GROUP (ORDER BY a) as p10 FROM VALUES (0), (1), (2), (3), (4) AS v(a) ``` currently returns: ``` +---+---+---+---+---+---+---+---+---+---+---+ | p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10| +---+---+---+---+---+---+---+---+---+---+---+ |0.0|0.0|0.0|1.0|1.0|2.0|2.0|2.0|3.0|3.0|4.0| +---+---+---+---+---+---+---+---+---+---+---+ ``` but after this PR it returns the correct: ``` +---+---+---+---+---+---+---+---+---+---+---+ | p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10| +---+---+---+---+---+---+---+---+---+---+---+ |0.0|0.0|0.0|1.0|1.0|2.0|2.0|3.0|3.0|4.0|4.0| +---+---+---+---+---+---+---+---+---+---+---+ ``` Bugfix. Yes, fixes a correctness bug, but the old behaviour can be restored with `spark.sql.legacy.percentileDiscCalculation=true`. Added new UTs. Closes apache#42559 from peter-toth/SPARK-44871-fix-percentile-disc-behaviour. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com>

peter-toth · 2023-08-22T14:03:01Z

Opened backport PRs:

### What changes were proposed in this pull request? This PR fixes `percentile_disc()` function as currently it returns inforrect results in some cases. E.g.: ``` SELECT percentile_disc(0.0) WITHIN GROUP (ORDER BY a) as p0, percentile_disc(0.1) WITHIN GROUP (ORDER BY a) as p1, percentile_disc(0.2) WITHIN GROUP (ORDER BY a) as p2, percentile_disc(0.3) WITHIN GROUP (ORDER BY a) as p3, percentile_disc(0.4) WITHIN GROUP (ORDER BY a) as p4, percentile_disc(0.5) WITHIN GROUP (ORDER BY a) as p5, percentile_disc(0.6) WITHIN GROUP (ORDER BY a) as p6, percentile_disc(0.7) WITHIN GROUP (ORDER BY a) as p7, percentile_disc(0.8) WITHIN GROUP (ORDER BY a) as p8, percentile_disc(0.9) WITHIN GROUP (ORDER BY a) as p9, percentile_disc(1.0) WITHIN GROUP (ORDER BY a) as p10 FROM VALUES (0), (1), (2), (3), (4) AS v(a) ``` currently returns: ``` +---+---+---+---+---+---+---+---+---+---+---+ | p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10| +---+---+---+---+---+---+---+---+---+---+---+ |0.0|0.0|0.0|1.0|1.0|2.0|2.0|2.0|3.0|3.0|4.0| +---+---+---+---+---+---+---+---+---+---+---+ ``` but after this PR it returns the correct: ``` +---+---+---+---+---+---+---+---+---+---+---+ | p0| p1| p2| p3| p4| p5| p6| p7| p8| p9|p10| +---+---+---+---+---+---+---+---+---+---+---+ |0.0|0.0|0.0|1.0|1.0|2.0|2.0|3.0|3.0|4.0|4.0| +---+---+---+---+---+---+---+---+---+---+---+ ``` ### Why are the changes needed? Bugfix. ### Does this PR introduce _any_ user-facing change? Yes, fixes a correctness bug, but the old behaviour can be restored with `spark.sql.legacy.percentileDiscCalculation=true`. ### How was this patch tested? Added new UTs. Closes apache#42559 from peter-toth/SPARK-44871-fix-percentile-disc-behaviour. Authored-by: Peter Toth <peter.toth@gmail.com> Signed-off-by: Peter Toth <peter.toth@gmail.com>

[SPARK-44871][SQL] Fix PERCENTILE_DISC behaviour

ac423c0

github-actions bot added the SQL label Aug 18, 2023

peter-toth commented Aug 18, 2023

View reviewed changes

jchen5 reviewed Aug 18, 2023

View reviewed changes

minor improvement

1d9f1a0

peter-toth force-pushed the SPARK-44871-fix-percentile-disc-behaviour branch from 5395044 to 1d9f1a0 Compare August 18, 2023 13:33

add comments

5ab5084

jchen5 approved these changes Aug 18, 2023

View reviewed changes

combine tests

da83c65

amaliujia reviewed Aug 18, 2023

View reviewed changes

better config description

93a018c

HyukjinKwon approved these changes Aug 21, 2023

View reviewed changes

MaxGekk requested changes Aug 21, 2023

View reviewed changes

peter-toth added 3 commits August 21, 2023 14:19

move legacy flag to class param, refactor code

18639c6

change legacy config version to earliest active release version

ca1d971

add legacy test case

82bbdc9

peter-toth force-pushed the SPARK-44871-fix-percentile-disc-behaviour branch from ccf2d8e to 82bbdc9 Compare August 21, 2023 12:21

peter-toth commented Aug 21, 2023

View reviewed changes

peter-toth added 2 commits August 21, 2023 18:40

regenerate golden files

50aead0

Merge branch 'master' into SPARK-44871-fix-percentile-disc-behaviour

6cd697c

cloud-fan approved these changes Aug 22, 2023

View reviewed changes

MaxGekk approved these changes Aug 22, 2023

View reviewed changes

peter-toth closed this in bd8fbf4 Aug 22, 2023

		private val legacyDiscCalculation: Boolean =
		SQLConf.get.getConf(SQLConf.LEGACY_PERCENTILE_DISC_CALCULATION)

[SPARK-44871][SQL] Fix percentile_disc behaviour #42559

[SPARK-44871][SQL] Fix percentile_disc behaviour #42559

Uh oh!

Conversation

peter-toth commented Aug 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jchen5 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-toth commented Aug 18, 2023

Uh oh!

jchen5 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

peter-toth commented Aug 22, 2023

Uh oh!

peter-toth commented Aug 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

peter-toth commented Aug 18, 2023 •

edited

Loading

jchen5 left a comment •

edited

Loading