Reference relevant issue in ReplaceSumWithStats (#74396)

Since the SQL `SUM` function behaves as expected, #45251 can be closed. As soon as #71582 is resolved, we can go back to using the `sum` aggregation instead of `stats`.
elastic · Jun 23, 2021 · 48e5a2f · 48e5a2f
1 parent 592b61a
commit 48e5a2f
Show file tree

Hide file tree

Showing 4 changed files with 8 additions and 8 deletions.
diff --git a/x-pack/plugin/sql/qa/server/src/main/resources/agg.sql-spec b/x-pack/plugin/sql/qa/server/src/main/resources/agg.sql-spec
@@ -450,7 +450,7 @@ aggSumWithMultipleHavingOnAliasAndFunction
 SELECT gender g, CAST(SUM(emp_no) AS INT) s FROM "test_emp" GROUP BY g HAVING s > 10 AND SUM(emp_no) > 10000000 ORDER BY gender;
 aggSumWithHavingOnIntegralDivision
 SELECT SUM(salary)/100 AS sum FROM test_emp HAVING sum <= 48248;
-// AwaitsFix https://github.com/elastic/elasticsearch/issues/45251
+// AwaitsFix https://github.com/elastic/elasticsearch/issues/71582
 aggSumWithHavingOnNull-Ignore
 SELECT SUM(languages) AS sum FROM test_emp GROUP BY languages HAVING sum < 20 ORDER BY sum;
 

diff --git a/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java b/x-pack/plugin/sql/src/main/java/org/elasticsearch/xpack/sql/optimizer/Optimizer.java
@@ -983,7 +983,7 @@ public LogicalPlan apply(LogicalPlan p) {
 
     // This class is a workaround for the SUM(all zeros) = NULL issue raised in https://github.com/elastic/elasticsearch/issues/45251 and
     // should be removed as soon as root cause is fixed and the sum aggregation results can differentiate between SUM(all zeroes)
-    // and SUM(all nulls)
+    // and SUM(all nulls) (https://github.com/elastic/elasticsearch/issues/71582)
     // NOTE: this rule should always be applied AFTER the ReplaceAggsWithStats rule
     static class ReplaceSumWithStats extends OptimizerBasicRule {
 

diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/optimizer/OptimizerTests.java
@@ -1119,11 +1119,11 @@ public void testReplaceCastOnLiteral() {
     }
 
     /**
-     * Once the root cause of https://github.com/elastic/elasticsearch/issues/45251 is fixed in the <code>sum</code> ES aggregation
-     * (can differentiate between <code>SUM(all zeroes)</code> and <code>SUM(all nulls)</code>),
-     * remove the {@link OptimizerTests#testSumIsReplacedWithStats()}, and re-enable the following test.
+     * Once https://github.com/elastic/elasticsearch/issues/71582 is addressed (ES `sum` aggregation can differentiate between
+     * <code>SUM(all zeroes)</code> and <code>SUM(all nulls)</code>), remove the {@link OptimizerTests#testSumIsReplacedWithStats()}, and
+     * re-enable the following test.
      */
-    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/45251")
+    @AwaitsFix(bugUrl = "https://github.com/elastic/elasticsearch/issues/71582")
     public void testSumIsNotReplacedWithStats() {
         FieldAttribute fa = getFieldAttribute();
         Sum sum = new Sum(EMPTY, fa);

diff --git a/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java b/...ck/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/planner/QueryTranslatorTests.java
@@ -1207,8 +1207,8 @@ public void testPercentileOptimization() {
     }
 
     // Tests the workaround for the SUM(all zeros) = NULL issue raised in https://github.com/elastic/elasticsearch/issues/45251 and
-    // should be removed as soon as root cause is fixed and the sum aggregation results can differentiate between SUM(all zeroes)
-    // and SUM(all nulls)
+    // should be removed as soon as root cause https://github.com/elastic/elasticsearch/issues/71582 is fixed and the sum aggregation
+    // results can differentiate between SUM(all zeroes) and SUM(all nulls)
     public void testReplaceSumWithStats() {
         List<String> testCases = asList(
             "SELECT keyword, SUM(int) FROM test GROUP BY keyword",