Restore usage of filtered SUM #17378

kgyrtkirk · 2024-10-18T12:01:04Z

Recent upgrade of calcite have brought a change which have fixed a minor correctness issue by not allowing the rewrite of SUM(CASE WHEN COND THEN C ELSE 0 END) to SUM(C) FILTER(COND)

This patch restores the old behaviour as the nested case_searched the above may retain in the plans could have serious performance impacts.

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+{
+  public DruidAggregateCaseToFilterRule()
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+{
+  public DruidAggregateCaseToFilterRule()
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+{
+  public DruidAggregateCaseToFilterRule()
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+        && value.equals(((RexLiteral) rexNode).getValueAs(BigDecimal.class));
+  }
+
+  private static class RewriteShuttle extends RexShuttle


gianm · 2024-11-07T16:51:34Z

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+ *    SUM(CASE WHEN COND THEN COL1 ELSE 0 END)
+ * without introducing an inner case this should be removed.
+ */
+public class DruidAggregateCaseToFilterRule extends RelOptRule implements SubstitutionRule


Does this rule create a null-handling bug? I thought that was why we stopped doing the transformation. If so I wonder if there's some other way we can fix the performance regression.

yes - it kinda does that

I was exploring to do it differently - one way is to do a more complicated rewrite:
did made it to be able to rewrite it back to a CASE COUNT() = 0 THEN NULL ELSE COALESCE(SUM() FILTER (F), 0) END ; not sure about that path - it really makes it more complicated...
I'm a bit skeptical about its long term pros/cons; we can have it - I have a version about it here:
https://github.com/kgyrtkirk/calcite/pull/7/files
If there would be more such rewrites it might possibly worth it - but right now I don't thin there will be more

the interesting cases for a SUM(CASE WHEN filter THEN valueCol ELSE 0 END) are:

original expression filtered-SUM filtered-SUM0

empty table null null 0

filtered row 0 null 0

selected; valueCol is null null null 0

selected; valueCol is 1 1 1 1

I wonder if we could possibly tweak the filtering to return zero in case there are filtered rows - or restore the null-handling bug

I was exploring the how to handle this with a rewrite that utilizes a customized filteredaggregator which could fix the all rows filtered case during the calcite/druid translation
but that would need things to be more bent around the filteredaggregator things...

I've stepped back and added a context flag which could restore the compliant behaviour and made the more performant the default - if someone wants to have that 0 then this could be turned off.

gianm · 2024-11-07T16:52:41Z

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+import java.util.List;
+
+/**
+ * Druid version of {@link AggregateCaseToFilterRule}


Does this rule completely cover the functionality of AggregateCaseToFilterRule? If so, can we stop running AggregateCaseToFilterRule? i.e., by removing CoreRules.AGGREGATE_CASE_TO_FILTER from CalciteRulesManager.

no - it just provides some extra logic
I think this should be removed when the upstream one could cover this case as well

…ase-rewrite-0

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+{
+  public DruidAggregateCaseToFilterRule()
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+{
+  public DruidAggregateCaseToFilterRule()
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+{
+  public DruidAggregateCaseToFilterRule()
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


gianm · 2024-12-04T22:41:50Z

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+ * | >0              | N>0          | 1        | N    | N            |
+ * +-----------------+--------------+----------+------+--------------+
+ *
+ * The only inconsistency this causes is when then condition matches all rows; and for all o


Incomplete javadoc?

gianm · 2024-12-04T22:44:14Z

processing/src/main/java/org/apache/druid/query/QueryContexts.java

@@ -88,6 +88,8 @@ public class QueryContexts
  public static final String UNCOVERED_INTERVALS_LIMIT_KEY = "uncoveredIntervalsLimit";
  public static final String MIN_TOP_N_THRESHOLD = "minTopNThreshold";
  public static final String CATALOG_VALIDATION_ENABLED = "catalogValidationEnabled";
+  public static final String EXTENDED_FILTERED_SUM_REWRITE_ENABLED = "extendedFilteredSumRewrite";


I see the new context parameter in this PR is undocumented and defaults to true (the old behavior, more performant, yet incorrect in the edge cases). IMO it's OK to have it be undocumented, but we should have a javadoc here explaining what is going on. Can you please add one and link it both to the rule (using @link) and also says something like: this is here for testing purposes, it is a known issue that the production configuration is incorrect in certain edge cases, and this context parameter can be removed once we have a way to do a rewrite of these case statements that is both high-performance and correct.

Added apidoc-s for this field to explain its purpose - I've also updated the docs for the class doing the rewrite.

sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+
+  public DruidAggregateCaseToFilterRule(boolean extendedFilteredSumRewrite)
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+
+  public DruidAggregateCaseToFilterRule(boolean extendedFilteredSumRewrite)
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+
+  public DruidAggregateCaseToFilterRule(boolean extendedFilteredSumRewrite)
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+
+  public DruidAggregateCaseToFilterRule(boolean extendedFilteredSumRewrite)
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+
+  public DruidAggregateCaseToFilterRule(boolean extendedFilteredSumRewrite)
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


sql/src/main/java/org/apache/druid/sql/calcite/rule/DruidAggregateCaseToFilterRule.java

+
+  public DruidAggregateCaseToFilterRule(boolean extendedFilteredSumRewrite)
+  {
+    super(operand(Aggregate.class, operand(Project.class, any())));


kgyrtkirk added 10 commits October 18, 2024 06:57

create now passing test

0fb319c

hacky fix

6815a53

stuff

e7b913f

stuff

c60488a

fixone

f4a5ef6

fixone

3ee98c6

cleanup

a9dc538

cleanup/update/etc

d680fef

rename

c1a806b

updates

fed8e70

github-actions bot added the Area - Querying label Oct 18, 2024

github-advanced-security bot found potential problems Oct 18, 2024

View reviewed changes

fix CalciteQueryTest defaults mode

bd9c154

github-advanced-security bot found potential problems Oct 18, 2024

View reviewed changes

gianm reviewed Nov 7, 2024

View reviewed changes

kgyrtkirk added 3 commits November 8, 2024 09:38

aa

fe0a494

update doc

c7462d8

Merge commit 'bd9c154fb71adeacbf538dc91595c5c53974b3f9' into sum-of-c…

117e543

…ase-rewrite-0

github-advanced-security bot found potential problems Nov 8, 2024

View reviewed changes

kgyrtkirk mentioned this pull request Nov 11, 2024

Regression in filtered aggregate for double columns #17455

Open

kgyrtkirk added 7 commits December 3, 2024 16:51

add other opts

b280a29

add toggle

272716f

cleanup/remove usage of other rule

5b726c3

some updates

26b2b24

up

fd56eac

undo unintended change

3465fdc

apply rule later; retain benefits of AggregateRemove

8b90f20

gianm reviewed Dec 4, 2024

View reviewed changes

update apidocs

053f635

kgyrtkirk added 2 commits December 5, 2024 08:57

more updates

f97ff69

Merge remote-tracking branch 'apache/master' into sum-of-case-rewrite-0

dc6a56d

github-advanced-security bot found potential problems Dec 5, 2024

View reviewed changes

gianm approved these changes Dec 9, 2024

View reviewed changes

kgyrtkirk added 3 commits December 11, 2024 12:54

fix coverage

333465b

Merge remote-tracking branch 'apache/master' into sum-of-case-rewrite-0

7e1fa86

update

8dee5d4

kgyrtkirk merged commit 1a38434 into apache:master Dec 12, 2024
82 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restore usage of filtered SUM #17378

Restore usage of filtered SUM #17378

kgyrtkirk commented Oct 18, 2024

gianm Nov 7, 2024

kgyrtkirk Dec 3, 2024

kgyrtkirk Dec 4, 2024

gianm Nov 7, 2024

kgyrtkirk Nov 8, 2024

gianm Dec 4, 2024

gianm Dec 4, 2024

kgyrtkirk Dec 5, 2024

	original expression	filtered-SUM	filtered-SUM0
empty table	null	null	0
filtered row	0	null	0
selected; valueCol is null	null	null	0
selected; valueCol is 1	1	1	1

Restore usage of filtered SUM #17378

Restore usage of filtered SUM #17378

Conversation

kgyrtkirk commented Oct 18, 2024

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