You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pinot.apache.org by "Copilot (via GitHub)" <gi...@apache.org> on 2025/12/10 02:06:28 UTC
Re: [PR] [bugfix] Fix SUM arithmetic rewrite under null handling [pinot]
Copilot commented on code in PR #17338:
URL: https://github.com/apache/pinot/pull/17338#discussion_r2604935652
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/rewriter/AggregationOptimizerTest.java:
##########
@@ -166,25 +262,21 @@ public void testMaxMultiplicationWithNegativeConstant() {
@Test
public void testMultiplicationWithTwoColumnsNotOptimized() {
- // Build sum(a * b) manually; should remain unchanged because neither side is a constant
- Expression multiplication = RequestUtils.getFunctionExpression("mult",
- RequestUtils.getIdentifierExpression("a"), RequestUtils.getIdentifierExpression("b"));
- Expression sum = RequestUtils.getFunctionExpression("sum", multiplication);
- PinotQuery pinotQuery = buildQueryWithSelect(sum);
+ // Parse sum(a * b); should remain unchanged because neither side is a constant
+ String query = "SELECT sum(a * b) FROM mytable";
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+
+ assertTopLevelOperator(pinotQuery.getSelectList().get(0), "sum");
_optimizer.rewrite(pinotQuery);
Function rewritten = pinotQuery.getSelectList().get(0).getFunctionCall();
+ assertTopLevelOperator(pinotQuery.getSelectList().get(0), "sum");
assertEquals(rewritten.getOperator(), "sum");
assertEquals(rewritten.getOperands().size(), 1);
Function multiplicationFunction = rewritten.getOperands().get(0).getFunctionCall();
- assertEquals(multiplicationFunction.getOperator(), "mult");
- }
-
- private PinotQuery buildQueryWithSelect(Expression expression) {
- PinotQuery pinotQuery = new PinotQuery();
- pinotQuery.setSelectList(new ArrayList<>(Collections.singletonList(expression)));
- return pinotQuery;
+ String operator = multiplicationFunction.getOperator();
+ assertTrue(operator.equalsIgnoreCase("mult") || operator.equalsIgnoreCase("times"));
Review Comment:
The assertion accommodates multiple multiplication operator names ('mult' and 'times'), but this pattern is inconsistent with other tests that only check for 'mult'. Consider standardizing on a single operator name throughout the codebase, or document why multiple operator names are expected here.
```suggestion
assertEquals(operator, "mult");
```
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/rewriter/AggregationOptimizerTest.java:
##########
@@ -19,104 +19,198 @@
package org.apache.pinot.sql.parsers.rewriter;
import java.util.ArrayList;
-import java.util.Collections;
+import java.util.List;
import org.apache.pinot.common.request.Expression;
import org.apache.pinot.common.request.ExpressionType;
import org.apache.pinot.common.request.Function;
import org.apache.pinot.common.request.PinotQuery;
-import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.common.utils.config.QueryOptionsUtils;
+import org.apache.pinot.spi.utils.CommonConstants.Broker.Request.QueryOptionKey;
import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
public class AggregationOptimizerTest {
private final AggregationOptimizer _optimizer = new AggregationOptimizer();
+ @BeforeClass
+ public void setUp() {
+ List<QueryRewriter> queryRewriters = new ArrayList<>();
+ for (QueryRewriter queryRewriter : QueryRewriterFactory.getQueryRewriters()) {
+ if (!(queryRewriter instanceof AggregationOptimizer)) {
+ queryRewriters.add(queryRewriter);
+ }
+ }
+ CalciteSqlParser.QUERY_REWRITERS.clear();
+ CalciteSqlParser.QUERY_REWRITERS.addAll(queryRewriters);
+ }
+
+ @AfterClass
+ public void tearDown() {
+ CalciteSqlParser.QUERY_REWRITERS.clear();
+ CalciteSqlParser.QUERY_REWRITERS.addAll(QueryRewriterFactory.getQueryRewriters());
+ }
+
@Test
public void testSumColumnPlusConstant() {
- // Test: SELECT sum(met + 2) → SELECT sum(met) + 2 * count(1)
+ // Test: SELECT sum(met + 2) → SELECT sum(met) + 2 * count(met)
String query = "SELECT sum(met + 2) FROM mytable";
PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Expression original = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(original, "sum");
+
// Apply optimizer
_optimizer.rewrite(pinotQuery);
// Verify optimization
Expression selectExpression = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(selectExpression, "add");
+ verifyOptimizedAddition(selectExpression, "met", 2);
+ }
+
+ @Test
+ public void testSumColumnPlusConstantWithNullHandlingEnabled() {
+ // Test: Optimizer rewrites using count(column) (null handling on)
+ String query = "SET enableNullHandling=true; SELECT sum(met + 2) FROM mytable";
+ SqlNodeAndOptions sqlNodeAndOptions = CalciteSqlParser.compileToSqlNodeAndOptions(query);
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(sqlNodeAndOptions);
+
+ Expression original = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(original, "sum");
+
+ // Apply optimizer
+ _optimizer.rewrite(pinotQuery);
+
+ assertTrue(QueryOptionsUtils.isNullHandlingEnabled(pinotQuery.getQueryOptions()));
+ Expression selectExpression = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(selectExpression, "add");
verifyOptimizedAddition(selectExpression, "met", 2);
}
+ @Test
+ public void testSumRewriteWithNullsKeepsSemantics() {
+ // Dataset with a null to ensure count(column) vs count(1) changes the result
+ Integer[] values = {1, null};
+ int sumNonNull = 1; // sum of non-null values
+ int countAll = values.length; // would be 2 if we incorrectly used count(1)
Review Comment:
The comment is misleading because `countAll` is not actually used in the test. The test correctly uses `countNonNull` in the calculation. Consider removing this variable and its comment, or clarify that it represents the incorrect behavior being prevented.
```suggestion
```
##########
pinot-common/src/test/java/org/apache/pinot/sql/parsers/rewriter/AggregationOptimizerTest.java:
##########
@@ -19,104 +19,198 @@
package org.apache.pinot.sql.parsers.rewriter;
import java.util.ArrayList;
-import java.util.Collections;
+import java.util.List;
import org.apache.pinot.common.request.Expression;
import org.apache.pinot.common.request.ExpressionType;
import org.apache.pinot.common.request.Function;
import org.apache.pinot.common.request.PinotQuery;
-import org.apache.pinot.common.utils.request.RequestUtils;
+import org.apache.pinot.common.utils.config.QueryOptionsUtils;
+import org.apache.pinot.spi.utils.CommonConstants.Broker.Request.QueryOptionKey;
import org.apache.pinot.sql.parsers.CalciteSqlParser;
+import org.apache.pinot.sql.parsers.SqlNodeAndOptions;
+import org.testng.annotations.AfterClass;
+import org.testng.annotations.BeforeClass;
import org.testng.annotations.Test;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
+import static org.testng.Assert.assertTrue;
public class AggregationOptimizerTest {
private final AggregationOptimizer _optimizer = new AggregationOptimizer();
+ @BeforeClass
+ public void setUp() {
+ List<QueryRewriter> queryRewriters = new ArrayList<>();
+ for (QueryRewriter queryRewriter : QueryRewriterFactory.getQueryRewriters()) {
+ if (!(queryRewriter instanceof AggregationOptimizer)) {
+ queryRewriters.add(queryRewriter);
+ }
+ }
+ CalciteSqlParser.QUERY_REWRITERS.clear();
+ CalciteSqlParser.QUERY_REWRITERS.addAll(queryRewriters);
+ }
+
+ @AfterClass
+ public void tearDown() {
+ CalciteSqlParser.QUERY_REWRITERS.clear();
+ CalciteSqlParser.QUERY_REWRITERS.addAll(QueryRewriterFactory.getQueryRewriters());
+ }
+
@Test
public void testSumColumnPlusConstant() {
- // Test: SELECT sum(met + 2) → SELECT sum(met) + 2 * count(1)
+ // Test: SELECT sum(met + 2) → SELECT sum(met) + 2 * count(met)
String query = "SELECT sum(met + 2) FROM mytable";
PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(query);
+ Expression original = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(original, "sum");
+
// Apply optimizer
_optimizer.rewrite(pinotQuery);
// Verify optimization
Expression selectExpression = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(selectExpression, "add");
+ verifyOptimizedAddition(selectExpression, "met", 2);
+ }
+
+ @Test
+ public void testSumColumnPlusConstantWithNullHandlingEnabled() {
+ // Test: Optimizer rewrites using count(column) (null handling on)
+ String query = "SET enableNullHandling=true; SELECT sum(met + 2) FROM mytable";
+ SqlNodeAndOptions sqlNodeAndOptions = CalciteSqlParser.compileToSqlNodeAndOptions(query);
+ PinotQuery pinotQuery = CalciteSqlParser.compileToPinotQuery(sqlNodeAndOptions);
+
+ Expression original = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(original, "sum");
+
+ // Apply optimizer
+ _optimizer.rewrite(pinotQuery);
+
+ assertTrue(QueryOptionsUtils.isNullHandlingEnabled(pinotQuery.getQueryOptions()));
+ Expression selectExpression = pinotQuery.getSelectList().get(0);
+ assertTopLevelOperator(selectExpression, "add");
verifyOptimizedAddition(selectExpression, "met", 2);
}
+ @Test
+ public void testSumRewriteWithNullsKeepsSemantics() {
+ // Dataset with a null to ensure count(column) vs count(1) changes the result
+ Integer[] values = {1, null};
+ int sumNonNull = 1; // sum of non-null values
+ int countAll = values.length; // would be 2 if we incorrectly used count(1)
+ int countNonNull = 1; // correct count for null handling
+ int constant = 2;
+ int expected = sumNonNull + constant * countNonNull; // expected sum(met + 2) with null handling
Review Comment:
The test defines a dataset with nulls but doesn't actually execute a query against this data or verify the computed result. The test only verifies the AST structure uses `count(column)` instead of `count(1)`. Consider either executing the query with actual data to verify correctness, or rename the test to clarify it only validates query structure.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@pinot.apache.org
For additional commands, e-mail: commits-help@pinot.apache.org