Copilot commented on code in PR #18681:
URL: https://github.com/apache/pinot/pull/18681#discussion_r3715385535
##########
pinot-materialized-view/src/test/java/org/apache/pinot/materializedview/rewrite/AggregationSubsumptionStrategyTest.java:
##########
@@ -894,4 +894,150 @@ public void
testFinerMaterializedViewSketchWithOrderByAlias() {
.getFunctionCall().getOperands().get(0).getIdentifier().getName(),
"raw_hll_FlightNum");
assertTrue(rewritten.getOrderByList().get(0).toString().contains("raw_hll_FlightNum"));
}
+
+ /// =======================================================================
+ /// Scalar grouping function support (e.g. DATETRUNC)
+ /// =======================================================================
+
+ /// A scalar grouping function (DATETRUNC) in the SELECT list must be
treated as a plain
+ /// projection — a direct MV column hit is sufficient. It must NOT be routed
through the
+ /// aggregation-equivalence path (which would reject it for lack of a
re-aggregation rule).
+ @Test
+ public void testScalarGroupingFunctionExactMatch() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders GROUP BY
DATETRUNC('DAY', ts)");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result, "Scalar grouping function should match via direct MV
projection");
+ assertEquals(result.getCost(), 6.0);
Review Comment:
These tests hard-code an exact plan cost (6.0), which is likely to be
brittle if the costing model changes (even when rewrite correctness remains
intact). Prefer asserting on stable behavioral outcomes (e.g., rewrite
occurred, expected MV columns appear, group-by/select shapes), or use a looser
assertion (e.g., cost is positive / within an expected range) if cost
validation is required.
##########
pinot-materialized-view/src/test/java/org/apache/pinot/materializedview/rewrite/AggregationSubsumptionStrategyTest.java:
##########
@@ -894,4 +894,150 @@ public void
testFinerMaterializedViewSketchWithOrderByAlias() {
.getFunctionCall().getOperands().get(0).getIdentifier().getName(),
"raw_hll_FlightNum");
assertTrue(rewritten.getOrderByList().get(0).toString().contains("raw_hll_FlightNum"));
}
+
+ /// =======================================================================
+ /// Scalar grouping function support (e.g. DATETRUNC)
+ /// =======================================================================
+
+ /// A scalar grouping function (DATETRUNC) in the SELECT list must be
treated as a plain
+ /// projection — a direct MV column hit is sufficient. It must NOT be routed
through the
+ /// aggregation-equivalence path (which would reject it for lack of a
re-aggregation rule).
+ @Test
+ public void testScalarGroupingFunctionExactMatch() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders GROUP BY
DATETRUNC('DAY', ts)");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result, "Scalar grouping function should match via direct MV
projection");
+ assertEquals(result.getCost(), 6.0);
+
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+
+ /// GROUP BY remapped to the MV column.
+ assertNotNull(rewritten.getGroupByList());
+ assertEquals(rewritten.getGroupByList().size(), 1);
+ assertEquals(rewritten.getGroupByList().get(0).getIdentifier().getName(),
"day");
+
+ List<Expression> selectList = rewritten.getSelectList();
+ assertEquals(selectList.size(), 2);
+
+ /// DATETRUNC('DAY', ts) → day AS datetrunc('DAY', ts) (direct projection,
alias preserves name).
+ Function dayAlias = selectList.get(0).getFunctionCall();
+ assertNotNull(dayAlias);
+ assertEquals(dayAlias.getOperator(), "as");
+ assertEquals(dayAlias.getOperands().get(0).getIdentifier().getName(),
"day");
+
+ /// SUM(revenue) → SUM(sum_rev) AS sum(revenue).
+ Function sumAlias = selectList.get(1).getFunctionCall();
+ assertNotNull(sumAlias);
+ assertEquals(sumAlias.getOperator(), "as");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperator(),
"sum");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(),
+ "sum_rev");
+ }
+
+ /// A scalar grouping function used in ORDER BY must remap to the MV column.
+ @Test
+ public void testScalarGroupingFunctionInOrderBy() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders "
+ + "GROUP BY DATETRUNC('DAY', ts) ORDER BY DATETRUNC('DAY', ts)
DESC");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result);
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+ assertNotNull(rewritten.getOrderByList());
+ assertEquals(rewritten.getOrderByList().size(), 1);
+ assertTrue(rewritten.getOrderByList().get(0).toString().contains("day"),
+ "ORDER BY scalar grouping function should remap to the MV column");
Review Comment:
Asserting via `toString().contains(\"day\")` is fragile because
formatting/normalization changes can break the test without changing semantics.
Where possible, assert on the structured AST (e.g., the identifier name in the
ORDER BY expression, and the presence of an identifier `day` at the expected
position inside the HAVING predicate).
##########
pinot-materialized-view/src/test/java/org/apache/pinot/materializedview/rewrite/AggregationSubsumptionStrategyTest.java:
##########
@@ -894,4 +894,150 @@ public void
testFinerMaterializedViewSketchWithOrderByAlias() {
.getFunctionCall().getOperands().get(0).getIdentifier().getName(),
"raw_hll_FlightNum");
assertTrue(rewritten.getOrderByList().get(0).toString().contains("raw_hll_FlightNum"));
}
+
+ /// =======================================================================
+ /// Scalar grouping function support (e.g. DATETRUNC)
+ /// =======================================================================
+
+ /// A scalar grouping function (DATETRUNC) in the SELECT list must be
treated as a plain
+ /// projection — a direct MV column hit is sufficient. It must NOT be routed
through the
+ /// aggregation-equivalence path (which would reject it for lack of a
re-aggregation rule).
+ @Test
+ public void testScalarGroupingFunctionExactMatch() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders GROUP BY
DATETRUNC('DAY', ts)");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result, "Scalar grouping function should match via direct MV
projection");
+ assertEquals(result.getCost(), 6.0);
+
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+
+ /// GROUP BY remapped to the MV column.
+ assertNotNull(rewritten.getGroupByList());
+ assertEquals(rewritten.getGroupByList().size(), 1);
+ assertEquals(rewritten.getGroupByList().get(0).getIdentifier().getName(),
"day");
+
+ List<Expression> selectList = rewritten.getSelectList();
+ assertEquals(selectList.size(), 2);
+
+ /// DATETRUNC('DAY', ts) → day AS datetrunc('DAY', ts) (direct projection,
alias preserves name).
+ Function dayAlias = selectList.get(0).getFunctionCall();
+ assertNotNull(dayAlias);
+ assertEquals(dayAlias.getOperator(), "as");
+ assertEquals(dayAlias.getOperands().get(0).getIdentifier().getName(),
"day");
+
+ /// SUM(revenue) → SUM(sum_rev) AS sum(revenue).
+ Function sumAlias = selectList.get(1).getFunctionCall();
+ assertNotNull(sumAlias);
+ assertEquals(sumAlias.getOperator(), "as");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperator(),
"sum");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(),
+ "sum_rev");
Review Comment:
This long chained assertion is hard to read/debug and may violate common
line-length/checkstyle rules. Consider extracting intermediate variables (e.g.,
the inner function call / its operand) before asserting on the final identifier
name.
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/strategy/AggregationSubsumptionStrategy.java:
##########
@@ -347,8 +347,8 @@ private List<Expression>
buildReAggSelectList(List<Expression> userSelectList,
String userAlias = MaterializedViewMatchUtils.extractUserAlias(expr);
Expression rewritten;
- if (viewProjectionMap.containsKey(stripped)
- && stripped.getFunctionCall() == null) {
+ if (!CalciteSqlParser.isAggregateExpression(stripped) &&
viewProjectionMap.containsKey(stripped)) {
+ /// Plain column OR scalar grouping function: project the MV column
directly.
rewritten =
RequestUtils.getIdentifierExpression(viewProjectionMap.get(stripped));
} else {
rewritten = rewriteAggregationExpression(stripped, viewProjectionMap);
Review Comment:
`buildReAggSelectList` falls back to `rewriteAggregationExpression(...)` for
any expression that isn't a direct MV hit, even if it is *not* an aggregate
expression (e.g., a scalar function that wasn't materialized). If this method
can be reached without `projectionSubsumes` having already rejected the plan,
`rewriteAggregationExpression` may behave incorrectly or fail with a confusing
error. Safer structure is to branch explicitly: (1) non-aggregate + MV hit =>
direct projection; (2) aggregate => `rewriteAggregationExpression`; (3)
non-aggregate without MV hit => reject/return null (or a clear failure) so
behavior is correct and diagnostics are clearer.
##########
pinot-materialized-view/src/test/java/org/apache/pinot/materializedview/rewrite/AggregationSubsumptionStrategyTest.java:
##########
@@ -894,4 +894,150 @@ public void
testFinerMaterializedViewSketchWithOrderByAlias() {
.getFunctionCall().getOperands().get(0).getIdentifier().getName(),
"raw_hll_FlightNum");
assertTrue(rewritten.getOrderByList().get(0).toString().contains("raw_hll_FlightNum"));
}
+
+ /// =======================================================================
+ /// Scalar grouping function support (e.g. DATETRUNC)
+ /// =======================================================================
+
+ /// A scalar grouping function (DATETRUNC) in the SELECT list must be
treated as a plain
+ /// projection — a direct MV column hit is sufficient. It must NOT be routed
through the
+ /// aggregation-equivalence path (which would reject it for lack of a
re-aggregation rule).
+ @Test
+ public void testScalarGroupingFunctionExactMatch() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders GROUP BY
DATETRUNC('DAY', ts)");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result, "Scalar grouping function should match via direct MV
projection");
+ assertEquals(result.getCost(), 6.0);
+
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+
+ /// GROUP BY remapped to the MV column.
+ assertNotNull(rewritten.getGroupByList());
+ assertEquals(rewritten.getGroupByList().size(), 1);
+ assertEquals(rewritten.getGroupByList().get(0).getIdentifier().getName(),
"day");
+
+ List<Expression> selectList = rewritten.getSelectList();
+ assertEquals(selectList.size(), 2);
+
+ /// DATETRUNC('DAY', ts) → day AS datetrunc('DAY', ts) (direct projection,
alias preserves name).
+ Function dayAlias = selectList.get(0).getFunctionCall();
+ assertNotNull(dayAlias);
+ assertEquals(dayAlias.getOperator(), "as");
+ assertEquals(dayAlias.getOperands().get(0).getIdentifier().getName(),
"day");
+
+ /// SUM(revenue) → SUM(sum_rev) AS sum(revenue).
+ Function sumAlias = selectList.get(1).getFunctionCall();
+ assertNotNull(sumAlias);
+ assertEquals(sumAlias.getOperator(), "as");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperator(),
"sum");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(),
+ "sum_rev");
+ }
+
+ /// A scalar grouping function used in ORDER BY must remap to the MV column.
+ @Test
+ public void testScalarGroupingFunctionInOrderBy() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders "
+ + "GROUP BY DATETRUNC('DAY', ts) ORDER BY DATETRUNC('DAY', ts)
DESC");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result);
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+ assertNotNull(rewritten.getOrderByList());
+ assertEquals(rewritten.getOrderByList().size(), 1);
+ assertTrue(rewritten.getOrderByList().get(0).toString().contains("day"),
+ "ORDER BY scalar grouping function should remap to the MV column");
+ }
+
+ /// A scalar grouping function used in HAVING must remap to the MV column.
+ @Test
+ public void testScalarGroupingFunctionInHaving() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders "
+ + "GROUP BY DATETRUNC('DAY', ts) HAVING DATETRUNC('DAY', ts) > 0");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result);
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+ assertNotNull(rewritten.getHavingExpression());
+ assertTrue(rewritten.getHavingExpression().toString().contains("day"),
+ "HAVING scalar grouping function should remap to the MV column");
+ }
+
+ /// A scalar function not present in the MV projection (different truncation
unit) must reject.
+ @Test
+ public void testNoMatchScalarFunctionNotMaterialized() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('MONTH', ts), SUM(revenue) FROM orders GROUP BY
DATETRUNC('MONTH', ts)");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNull(result, "DATETRUNC('MONTH', ts) is not a materialized
projection and must be rejected");
+ }
+
+ /// MV is grouped by a superset of the user keys (DATETRUNC('DAY', ts),
city) while the user
+ /// groups only by DATETRUNC('DAY', ts). Re-aggregation is non-trivial here
(multiple MV rows
+ /// collapse per day), and the scalar grouping function must still resolve
as a direct MV hit.
+ @Test
+ public void testScalarGroupingFunctionFinerMaterializedViewGranularity() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, city, SUM(revenue) AS sum_rev
FROM orders "
+ + "GROUP BY DATETRUNC('DAY', ts), city";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders GROUP BY
DATETRUNC('DAY', ts)");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result, "Finer MV granularity with a scalar grouping key
should re-aggregate");
+ assertEquals(result.getCost(), 6.0);
Review Comment:
These tests hard-code an exact plan cost (6.0), which is likely to be
brittle if the costing model changes (even when rewrite correctness remains
intact). Prefer asserting on stable behavioral outcomes (e.g., rewrite
occurred, expected MV columns appear, group-by/select shapes), or use a looser
assertion (e.g., cost is positive / within an expected range) if cost
validation is required.
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/strategy/AggregationSubsumptionStrategy.java:
##########
@@ -102,19 +102,19 @@ protected boolean projectionSubsumes(List<Expression>
userSelectList,
}
for (Expression expr : userSelectList) {
Expression stripped = MaterializedViewMatchUtils.stripAlias(expr);
- /// Plain column reference: direct MV projection hit is sufficient.
- if (stripped.getFunctionCall() == null) {
+ if (CalciteSqlParser.isAggregateExpression(stripped)) {
Review Comment:
The aggregate-vs-non-aggregate decision is repeated in multiple places.
Consider computing `boolean isAggregate =
CalciteSqlParser.isAggregateExpression(...)` once per expression in each method
(and/or centralizing the decision) to reduce repeated work and make the
branching logic easier to read and keep consistent.
##########
pinot-materialized-view/src/test/java/org/apache/pinot/materializedview/rewrite/AggregationSubsumptionStrategyTest.java:
##########
@@ -894,4 +894,150 @@ public void
testFinerMaterializedViewSketchWithOrderByAlias() {
.getFunctionCall().getOperands().get(0).getIdentifier().getName(),
"raw_hll_FlightNum");
assertTrue(rewritten.getOrderByList().get(0).toString().contains("raw_hll_FlightNum"));
}
+
+ /// =======================================================================
+ /// Scalar grouping function support (e.g. DATETRUNC)
+ /// =======================================================================
+
+ /// A scalar grouping function (DATETRUNC) in the SELECT list must be
treated as a plain
+ /// projection — a direct MV column hit is sufficient. It must NOT be routed
through the
+ /// aggregation-equivalence path (which would reject it for lack of a
re-aggregation rule).
+ @Test
+ public void testScalarGroupingFunctionExactMatch() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders GROUP BY
DATETRUNC('DAY', ts)");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result, "Scalar grouping function should match via direct MV
projection");
+ assertEquals(result.getCost(), 6.0);
+
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+
+ /// GROUP BY remapped to the MV column.
+ assertNotNull(rewritten.getGroupByList());
+ assertEquals(rewritten.getGroupByList().size(), 1);
+ assertEquals(rewritten.getGroupByList().get(0).getIdentifier().getName(),
"day");
+
+ List<Expression> selectList = rewritten.getSelectList();
+ assertEquals(selectList.size(), 2);
+
+ /// DATETRUNC('DAY', ts) → day AS datetrunc('DAY', ts) (direct projection,
alias preserves name).
+ Function dayAlias = selectList.get(0).getFunctionCall();
+ assertNotNull(dayAlias);
+ assertEquals(dayAlias.getOperator(), "as");
+ assertEquals(dayAlias.getOperands().get(0).getIdentifier().getName(),
"day");
+
+ /// SUM(revenue) → SUM(sum_rev) AS sum(revenue).
+ Function sumAlias = selectList.get(1).getFunctionCall();
+ assertNotNull(sumAlias);
+ assertEquals(sumAlias.getOperator(), "as");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperator(),
"sum");
+
assertEquals(sumAlias.getOperands().get(0).getFunctionCall().getOperands().get(0).getIdentifier().getName(),
+ "sum_rev");
+ }
+
+ /// A scalar grouping function used in ORDER BY must remap to the MV column.
+ @Test
+ public void testScalarGroupingFunctionInOrderBy() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders "
+ + "GROUP BY DATETRUNC('DAY', ts) ORDER BY DATETRUNC('DAY', ts)
DESC");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result);
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+ assertNotNull(rewritten.getOrderByList());
+ assertEquals(rewritten.getOrderByList().size(), 1);
+ assertTrue(rewritten.getOrderByList().get(0).toString().contains("day"),
+ "ORDER BY scalar grouping function should remap to the MV column");
+ }
+
+ /// A scalar grouping function used in HAVING must remap to the MV column.
+ @Test
+ public void testScalarGroupingFunctionInHaving() {
+ String definedSql =
+ "SELECT DATETRUNC('DAY', ts) AS day, SUM(revenue) AS sum_rev FROM
orders "
+ + "GROUP BY DATETRUNC('DAY', ts)";
+ MaterializedViewCacheEntry entry = createEntry("mv_orders_OFFLINE",
"orders", definedSql);
+
+ PinotQuery userQuery = CalciteSqlParser.compileToPinotQuery(
+ "SELECT DATETRUNC('DAY', ts), SUM(revenue) FROM orders "
+ + "GROUP BY DATETRUNC('DAY', ts) HAVING DATETRUNC('DAY', ts) > 0");
+ MaterializedViewRewritePlan result = _strategy.match(userQuery, entry);
+
+ assertNotNull(result);
+ PinotQuery rewritten = result.getMaterializedViewQuery();
+ assertNotNull(rewritten.getHavingExpression());
+ assertTrue(rewritten.getHavingExpression().toString().contains("day"),
+ "HAVING scalar grouping function should remap to the MV column");
Review Comment:
Asserting via `toString().contains(\"day\")` is fragile because
formatting/normalization changes can break the test without changing semantics.
Where possible, assert on the structured AST (e.g., the identifier name in the
ORDER BY expression, and the presence of an identifier `day` at the expected
position inside the HAVING predicate).
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/strategy/AggregationSubsumptionStrategy.java:
##########
@@ -392,7 +392,8 @@ private Expression rewriteAggregationExpression(Expression
stripped,
private Expression remapExpressionWithEquivalence(Expression expr,
Map<Expression, String> viewProjectionMap) {
- if (viewProjectionMap.containsKey(expr) && expr.getFunctionCall() == null)
{
+ /// Plain column OR scalar grouping function with a direct MV hit: map to
the MV column.
+ if (!CalciteSqlParser.isAggregateExpression(expr) &&
viewProjectionMap.containsKey(expr)) {
Review Comment:
The aggregate-vs-non-aggregate decision is repeated in multiple places.
Consider computing `boolean isAggregate =
CalciteSqlParser.isAggregateExpression(...)` once per expression in each method
(and/or centralizing the decision) to reduce repeated work and make the
branching logic easier to read and keep consistent.
##########
pinot-materialized-view/src/main/java/org/apache/pinot/materializedview/rewrite/strategy/AggregationSubsumptionStrategy.java:
##########
@@ -347,8 +347,8 @@ private List<Expression>
buildReAggSelectList(List<Expression> userSelectList,
String userAlias = MaterializedViewMatchUtils.extractUserAlias(expr);
Expression rewritten;
- if (viewProjectionMap.containsKey(stripped)
- && stripped.getFunctionCall() == null) {
+ if (!CalciteSqlParser.isAggregateExpression(stripped) &&
viewProjectionMap.containsKey(stripped)) {
Review Comment:
The aggregate-vs-non-aggregate decision is repeated in multiple places.
Consider computing `boolean isAggregate =
CalciteSqlParser.isAggregateExpression(...)` once per expression in each method
(and/or centralizing the decision) to reduce repeated work and make the
branching logic easier to read and keep consistent.
--
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]
