Copilot commented on code in PR #17380: URL: https://github.com/apache/iotdb/pull/17380#discussion_r3004164077
########## iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/GroupByAllTest.java: ########## @@ -0,0 +1,255 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iotdb.db.queryengine.plan.relational.analyzer; + +import org.apache.iotdb.db.queryengine.plan.planner.plan.LogicalQueryPlan; +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanNode; +import org.apache.iotdb.db.queryengine.plan.relational.planner.PlanTester; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.AggregationNode; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.AggregationTableScanNode; +import org.apache.iotdb.db.queryengine.plan.relational.planner.node.GapFillNode; + +import com.google.common.collect.ImmutableList; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.ImmutableSet; +import org.junit.Test; + +import java.util.Optional; + +import static org.apache.iotdb.db.queryengine.plan.relational.analyzer.TestUtils.assertAnalyzeSemanticException; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.assertions.PlanAssert.assertPlan; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.assertions.PlanMatchPattern.aggregation; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.assertions.PlanMatchPattern.aggregationFunction; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.assertions.PlanMatchPattern.output; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.assertions.PlanMatchPattern.singleGroupingSet; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.assertions.PlanMatchPattern.tableScan; +import static org.apache.iotdb.db.queryengine.plan.relational.planner.node.AggregationNode.Step.SINGLE; +import static org.junit.Assert.assertTrue; +import static org.junit.Assert.fail; + +/** + * Tests for the GROUP BY ALL syntax. Verifies that the analyzer correctly infers grouping columns + * from the SELECT clause and that integration with date_bin_gapfill is preserved. + */ +public class GroupByAllTest { + + // ---- basic column inference ---- + + @Test + public void groupByAllSingleColumnTest() { + // GROUP BY ALL should infer s1 as the grouping key (the only non-aggregate expression) + PlanTester planTester = new PlanTester(); + LogicalQueryPlan plan = + planTester.createPlan("SELECT s1, count(s2) FROM table1 GROUP BY ALL"); + assertPlan( + plan, + output( + aggregation( + singleGroupingSet("s1"), + ImmutableMap.of( + Optional.empty(), + aggregationFunction("count", ImmutableList.of("s2"))), + ImmutableList.of(), + Optional.empty(), + SINGLE, + tableScan( + "testdb.table1", + ImmutableList.of("s1", "s2"), + ImmutableSet.of("s1", "s2"))))); + } + + @Test + public void groupByAllMultipleColumnsTest() { + // GROUP BY ALL should infer tag1, tag2, tag3 as grouping keys. + // The optimizer pushes aggregation into the table scan, producing AggregationTableScanNode. + // Verify by walking the plan tree. + PlanTester planTester = new PlanTester(); + LogicalQueryPlan plan = + planTester.createPlan( + "SELECT tag1, tag2, tag3, count(s2) FROM table1 GROUP BY ALL"); + + PlanNode root = plan.getRootNode(); + AggregationTableScanNode aggScan = findNode(root, AggregationTableScanNode.class); + assertTrue( + "Expected AggregationTableScanNode with grouping keys [tag1, tag2, tag3]", + aggScan != null + && aggScan.getGroupingKeys().stream() + .map(s -> s.getName()) + .collect(java.util.stream.Collectors.toSet()) + .containsAll(ImmutableSet.of("tag1", "tag2", "tag3"))); Review Comment: This test hard-requires the optimizer to produce an AggregationTableScanNode. That’s an implementation detail and can change with planner/optimizer rules, making the test unnecessarily brittle. Consider asserting the inferred grouping keys via PlanMatchPattern (e.g., match either AggregationTableScanNode or an AggregationNode over a TableScan), similar to other analyzer tests in this module. ```suggestion // Assert the inferred grouping keys and aggregation via PlanMatchPattern, // without depending on a specific physical node implementation. PlanTester planTester = new PlanTester(); LogicalQueryPlan plan = planTester.createPlan( "SELECT tag1, tag2, tag3, count(s2) FROM table1 GROUP BY ALL"); assertPlan( plan, output( aggregation( singleGroupingSet("tag1", "tag2", "tag3"), ImmutableMap.of( Optional.empty(), aggregationFunction("count", ImmutableList.of("s2"))), ImmutableList.of(), Optional.empty(), SINGLE, tableScan( "testdb.table1", ImmutableList.of("tag1", "tag2", "tag3", "s2"), ImmutableSet.of("tag1", "tag2", "tag3", "s2"))))); ``` ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/StatementAnalyzer.java: ########## @@ -2718,6 +2724,69 @@ private Analysis.GroupingSetAnalysis analyzeGroupBy( return result; } + private Analysis.GroupingSetAnalysis analyzeGroupByAll( + QuerySpecification node, Scope scope, List<Expression> outputExpressions) { + ImmutableList.Builder<List<Set<FieldId>>> sets = ImmutableList.builder(); + ImmutableList.Builder<Expression> complexExpressions = ImmutableList.builder(); + ImmutableList.Builder<Expression> groupingExpressions = ImmutableList.builder(); + FunctionCall gapFillColumn = null; + ImmutableList.Builder<Expression> gapFillGroupingExpressions = ImmutableList.builder(); + + for (Expression outputExpression : outputExpressions) { + List<FunctionCall> aggregates = + extractAggregateFunctions(ImmutableList.of(outputExpression)); + List<FunctionCall> windowFunctions = + extractWindowFunctions(ImmutableList.of(outputExpression)); + if (!aggregates.isEmpty() || !windowFunctions.isEmpty()) { + continue; + } + + analyzeExpression(outputExpression, scope); Review Comment: In analyzeGroupByAll(), each inferred grouping expression comes from outputExpressions, which have already been analyzed in analyzeSelectSingleColumn/analyzeSelectAllColumns earlier in visitQuerySpecification. Calling analyzeExpression(outputExpression, scope) again here is redundant and can add avoidable analyzer work (and risks duplicating any side-effects if analyzeExpression isn’t strictly idempotent). Consider relying on the existing analysis state (e.g., use analysis.getColumnReferenceFields()/analysis.getType directly) and only analyze when the expression hasn’t been analyzed yet. ```suggestion ``` -- 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]
