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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
