morrySnow commented on code in PR #23121:
URL: https://github.com/apache/doris/pull/23121#discussion_r1314845287
##########
fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -438,6 +445,7 @@ primaryExpression
| identifier
#columnReference
| base=primaryExpression DOT fieldName=identifier
#dereference
| LEFT_PAREN expression RIGHT_PAREN
#parenthesizedExpression
+ | LEFT_PAREN namedExpression (COMMA namedExpression)+ RIGHT_PAREN
#rowConstructor
Review Comment:
`rowConstructor` should put ahead than `subqueryExpression`
##########
fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -438,6 +445,7 @@ primaryExpression
| identifier
#columnReference
| base=primaryExpression DOT fieldName=identifier
#dereference
| LEFT_PAREN expression RIGHT_PAREN
#parenthesizedExpression
+ | LEFT_PAREN namedExpression (COMMA namedExpression)+ RIGHT_PAREN
#rowConstructor
Review Comment:
branch name aka(#xxx) should be align with other lines
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2322,4 +2356,20 @@ private String parseConstant(ConstantContext context) {
public Object visitCollate(CollateContext ctx) {
return visit(ctx.primaryExpression());
}
+
+ private static class Row extends Expression {
+ public Row(List<Expression> children) {
Review Comment:
```suggestion
public Row(List<NamedExpression> children) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -2322,4 +2356,20 @@ private String parseConstant(ConstantContext context) {
public Object visitCollate(CollateContext ctx) {
return visit(ctx.primaryExpression());
}
+
+ private static class Row extends Expression {
Review Comment:
add comment to explain what this class does
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -1634,6 +1663,11 @@ public Expression
visitParenthesizedExpression(ParenthesizedExpressionContext ct
return getExpression(ctx.expression());
}
+ @Override
+ public Expression visitRowConstructor(RowConstructorContext ctx) {
+ return new
Row(ctx.namedExpression().stream().map(this::visitNamedExpression).collect(Collectors.toList()));
Review Comment:
```suggestion
return new
Row(ctx.namedExpression().stream().map(this::visitNamedExpression).collect(ImmutableList.toImmutableList()));
```
##########
regression-test/suites/nereids_p0/insert_into_table/insert_values.groovy:
##########
@@ -0,0 +1,118 @@
+// 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.
+
+suite('nereids_insert_into_values') {
+ sql 'set enable_nereids_planner=true'
+ sql 'set enable_fallback_to_original_planner=false'
+ sql 'set enable_nereids_dml=true'
+ sql 'set enable_strict_consistency_dml=true'
+
+ sql 'use nereids_insert_into_table_test'
+
+ def t1 = 'value_t1'
+ def t2 = 'value_t2'
+ def t3 = 'value_t3'
+
+ sql "drop table if exists ${t1}"
+ sql "drop table if exists ${t2}"
+ sql "drop table if exists ${t3}"
+
+ sql """
+ create table ${t1} (
+ id int,
+ id1 int,
+ c1 bigint,
+ c2 string,
+ c3 double,
+ c4 date
+ ) unique key (id, id1)
+ distributed by hash(id, id1) buckets 13
+ properties(
+ 'replication_num'='1',
+ "function_column.sequence_col" = "c4"
Review Comment:
add mor table cases
##########
regression-test/suites/nereids_p0/insert_into_table/insert_values.groovy:
##########
@@ -0,0 +1,118 @@
+// 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.
+
+suite('nereids_insert_into_values') {
+ sql 'set enable_nereids_planner=true'
+ sql 'set enable_fallback_to_original_planner=false'
+ sql 'set enable_nereids_dml=true'
+ sql 'set enable_strict_consistency_dml=true'
+
+ sql 'use nereids_insert_into_table_test'
+
+ def t1 = 'value_t1'
+ def t2 = 'value_t2'
+ def t3 = 'value_t3'
+
+ sql "drop table if exists ${t1}"
+ sql "drop table if exists ${t2}"
+ sql "drop table if exists ${t3}"
+
+ sql """
+ create table ${t1} (
+ id int,
+ id1 int,
+ c1 bigint,
+ c2 string,
+ c3 double,
+ c4 date
+ ) unique key (id, id1)
+ distributed by hash(id, id1) buckets 13
+ properties(
+ 'replication_num'='1',
+ "function_column.sequence_col" = "c4"
+ );
+ """
+
+ sql """
+ create table ${t2} (
+ id int,
+ c1 bigint,
+ c2 string,
+ c3 double,
+ c4 date
+ ) unique key (id)
+ distributed by hash(id) buckets 13
+ properties(
+ 'replication_num'='1'
+ );
+ """
+
+ sql """
+ create table ${t3} (
+ id int
+ ) distributed by hash(id) buckets 13
+ properties(
+ 'replication_num'='1'
+ );
+ """
+
+ sql """
+ INSERT INTO ${t1} VALUES
+ (1, 10, 1, '1', 1.0, '2000-01-01'),
Review Comment:
add some line with function call, or other expression rather than just
literal
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -700,6 +705,30 @@ public LogicalPlan
visitRegularQuerySpecification(RegularQuerySpecificationConte
});
}
+ @Override
+ public LogicalPlan visitInlineTable(InlineTableContext ctx) {
+ List<List<Expression>> exprs = ctx.expression().stream()
+ .map(e -> {
+ Object o = visit(e);
+ if (o instanceof Row) {
+ return (((Row) o).children());
+ } else if (o instanceof Expression) {
+ return ImmutableList.of(((Expression) o));
+ } else {
+ throw new AnalysisException("invalid inline table");
+ }
+ }).collect(Collectors.toList());
+ LogicalPlan relation = new LogicalOneRowRelation(
+ StatementScopeIdGenerator.newRelationId(),
+ exprs.get(0).stream().map(e -> new Alias(e,
e.toSql())).collect(Collectors.toList()));
+ for (List<Expression> exprList : exprs.subList(1, exprs.size())) {
Review Comment:
```suggestion
for (List<Expression> exprs : exprs.subList(1, exprs.size())) {
```
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -700,6 +705,30 @@ public LogicalPlan
visitRegularQuerySpecification(RegularQuerySpecificationConte
});
}
+ @Override
+ public LogicalPlan visitInlineTable(InlineTableContext ctx) {
+ List<List<Expression>> exprs = ctx.expression().stream()
+ .map(e -> {
+ Object o = visit(e);
+ if (o instanceof Row) {
+ return (((Row) o).children());
+ } else if (o instanceof Expression) {
+ return ImmutableList.of(((Expression) o));
+ } else {
+ throw new AnalysisException("invalid inline table");
+ }
+ }).collect(Collectors.toList());
+ LogicalPlan relation = new LogicalOneRowRelation(
+ StatementScopeIdGenerator.newRelationId(),
+ exprs.get(0).stream().map(e -> new Alias(e,
e.toSql())).collect(Collectors.toList()));
+ for (List<Expression> exprList : exprs.subList(1, exprs.size())) {
Review Comment:
`subList` is expensive, use `for (int i = 1; i < ...)` instead
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -700,6 +705,30 @@ public LogicalPlan
visitRegularQuerySpecification(RegularQuerySpecificationConte
});
}
+ @Override
+ public LogicalPlan visitInlineTable(InlineTableContext ctx) {
+ List<List<Expression>> exprs = ctx.expression().stream()
Review Comment:
```suggestion
List<List<Expression>> exprsList = ctx.expression().stream()
```
##########
regression-test/suites/nereids_p0/insert_into_table/insert_values.groovy:
##########
@@ -0,0 +1,118 @@
+// 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.
+
+suite('nereids_insert_into_values') {
+ sql 'set enable_nereids_planner=true'
+ sql 'set enable_fallback_to_original_planner=false'
+ sql 'set enable_nereids_dml=true'
+ sql 'set enable_strict_consistency_dml=true'
+
+ sql 'use nereids_insert_into_table_test'
+
+ def t1 = 'value_t1'
+ def t2 = 'value_t2'
+ def t3 = 'value_t3'
+
+ sql "drop table if exists ${t1}"
+ sql "drop table if exists ${t2}"
+ sql "drop table if exists ${t3}"
+
+ sql """
+ create table ${t1} (
+ id int,
+ id1 int,
+ c1 bigint,
+ c2 string,
+ c3 double,
+ c4 date
+ ) unique key (id, id1)
+ distributed by hash(id, id1) buckets 13
+ properties(
+ 'replication_num'='1',
+ "function_column.sequence_col" = "c4"
+ );
+ """
+
+ sql """
+ create table ${t2} (
+ id int,
+ c1 bigint,
+ c2 string,
+ c3 double,
+ c4 date
+ ) unique key (id)
+ distributed by hash(id) buckets 13
+ properties(
+ 'replication_num'='1'
+ );
+ """
+
+ sql """
+ create table ${t3} (
+ id int
+ ) distributed by hash(id) buckets 13
+ properties(
+ 'replication_num'='1'
+ );
+ """
+
+ sql """
+ INSERT INTO ${t1} VALUES
+ (1, 10, 1, '1', 1.0, '2000-01-01'),
+ (2, 20, 2, '2', 2.0, '2000-01-02'),
+ (3, 30, 3, '3', 3.0, '2000-01-03');
+ """
+
+ sql """
+ INSERT INTO ${t2} VALUES
+ (1, 10, '10', 10.0, '2000-01-10'),
+ (2, 20, '20', 20.0, '2000-01-20'),
+ (3, 30, '30', 30.0, '2000-01-30'),
+ (4, 4, '4', 4.0, '2000-01-04'),
+ (5, 5, '5', 5.0, '2000-01-05');
+ """
+
+ sql """
+ INSERT INTO ${t3} VALUES
+ (1),
+ (4),
+ (5);
+ """
+
+ sql "sync"
+ qt_sql_1 "select * from ${t1}, ${t2}, ${t3} order by ${t1}.id, ${t1}.id1,
${t2}.id, ${t3}.id"
Review Comment:
do not use meaningless name, such as `sql_1`. u should use the case name to
explain what do u want test
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/analysis/BindSink.java:
##########
@@ -116,8 +118,13 @@ public List<Rule> buildRules() {
"mv column's ref column cannot
be null");
Expression parsedExpression =
expressionParser.parseExpression(
column.getDefineExpr().toSql());
- Expression boundExpression =
SlotReplacer.INSTANCE
+ Expression boundSlotExpression =
SlotReplacer.INSTANCE
.replace(parsedExpression,
columnToOutput);
+ // the boundSlotExpression is an
expression whose slots are bound but function
+ // may not be bound, we have to bind
it again.
+ // for example: to_bitmap.
+ Expression boundExpression =
FunctionBinder.INSTANCE.rewrite(
+ boundSlotExpression, new
ExpressionRewriteContext(ctx.cascadesContext));
Review Comment:
do u add case for this?
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -700,6 +705,30 @@ public LogicalPlan
visitRegularQuerySpecification(RegularQuerySpecificationConte
});
}
+ @Override
+ public LogicalPlan visitInlineTable(InlineTableContext ctx) {
+ List<List<Expression>> exprs = ctx.expression().stream()
+ .map(e -> {
+ Object o = visit(e);
+ if (o instanceof Row) {
+ return (((Row) o).children());
+ } else if (o instanceof Expression) {
+ return ImmutableList.of(((Expression) o));
+ } else {
+ throw new AnalysisException("invalid inline table");
+ }
+ }).collect(Collectors.toList());
+ LogicalPlan relation = new LogicalOneRowRelation(
+ StatementScopeIdGenerator.newRelationId(),
+ exprs.get(0).stream().map(e -> new Alias(e,
e.toSql())).collect(Collectors.toList()));
+ for (List<Expression> exprList : exprs.subList(1, exprs.size())) {
+ relation = new LogicalUnion(Qualifier.ALL,
ImmutableList.of(relation, new LogicalOneRowRelation(
+ StatementScopeIdGenerator.newRelationId(),
+ exprList.stream().map(e -> new Alias(e,
e.toSql())).collect(Collectors.toList()))));
Review Comment:
```suggestion
exprList.stream().map(e -> new Alias(e,
e.toSql())).collect(ImmutableList.toImmutableList()))));
```
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -646,6 +646,9 @@ public void executeByLegacy(TUniqueId queryId) throws
Exception {
profile.getSummaryProfile().setQueryBeginTime();
context.setStmtId(STMT_ID_GENERATOR.incrementAndGet());
context.setQueryId(queryId);
+ if (parsedStmt == null) {
+ parseByLegacy();
+ }
Review Comment:
why add parsedStmt == null? we already check parsedStmt == null in
parseByLegacy()
##########
regression-test/suites/nereids_p0/insert_into_table/insert_values.groovy:
##########
@@ -0,0 +1,118 @@
+// 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.
+
+suite('nereids_insert_into_values') {
Review Comment:
add agg table cases
##########
fe/fe-core/src/main/java/org/apache/doris/qe/StmtExecutor.java:
##########
@@ -862,7 +865,9 @@ public void analyze(TQueryOptions tQueryOptions) throws
UserException, Interrupt
context.getForwardedStmtId());
}
- parseByLegacy();
+ if (parsedStmt == null) {
+ parseByLegacy();
+ }
Review Comment:
why add `parsedStmt == null`? we already check `parsedStmt == null` in
`parseByLegacy()`
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/parser/LogicalPlanBuilder.java:
##########
@@ -700,6 +705,30 @@ public LogicalPlan
visitRegularQuerySpecification(RegularQuerySpecificationConte
});
}
+ @Override
+ public LogicalPlan visitInlineTable(InlineTableContext ctx) {
+ List<List<Expression>> exprs = ctx.expression().stream()
+ .map(e -> {
+ Object o = visit(e);
+ if (o instanceof Row) {
+ return (((Row) o).children());
+ } else if (o instanceof Expression) {
+ return ImmutableList.of(((Expression) o));
+ } else {
+ throw new AnalysisException("invalid inline table");
+ }
+ }).collect(Collectors.toList());
Review Comment:
```suggestion
}).collect(ImmutableList.toImmutableList());
```
##########
regression-test/suites/nereids_p0/insert_into_table/insert_values.groovy:
##########
@@ -0,0 +1,118 @@
+// 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.
+
+suite('nereids_insert_into_values') {
+ sql 'set enable_nereids_planner=true'
+ sql 'set enable_fallback_to_original_planner=false'
+ sql 'set enable_nereids_dml=true'
+ sql 'set enable_strict_consistency_dml=true'
+
+ sql 'use nereids_insert_into_table_test'
+
+ def t1 = 'value_t1'
+ def t2 = 'value_t2'
+ def t3 = 'value_t3'
+
+ sql "drop table if exists ${t1}"
+ sql "drop table if exists ${t2}"
+ sql "drop table if exists ${t3}"
+
+ sql """
+ create table ${t1} (
+ id int,
+ id1 int,
+ c1 bigint,
+ c2 string,
+ c3 double,
+ c4 date
+ ) unique key (id, id1)
+ distributed by hash(id, id1) buckets 13
+ properties(
+ 'replication_num'='1',
+ "function_column.sequence_col" = "c4"
+ );
+ """
+
+ sql """
+ create table ${t2} (
+ id int,
+ c1 bigint,
+ c2 string,
+ c3 double,
+ c4 date
+ ) unique key (id)
+ distributed by hash(id) buckets 13
+ properties(
+ 'replication_num'='1'
+ );
+ """
+
+ sql """
+ create table ${t3} (
+ id int
+ ) distributed by hash(id) buckets 13
+ properties(
+ 'replication_num'='1'
+ );
+ """
+
+ sql """
+ INSERT INTO ${t1} VALUES
+ (1, 10, 1, '1', 1.0, '2000-01-01'),
+ (2, 20, 2, '2', 2.0, '2000-01-02'),
+ (3, 30, 3, '3', 3.0, '2000-01-03');
+ """
+
+ sql """
+ INSERT INTO ${t2} VALUES
+ (1, 10, '10', 10.0, '2000-01-10'),
+ (2, 20, '20', 20.0, '2000-01-20'),
+ (3, 30, '30', 30.0, '2000-01-30'),
+ (4, 4, '4', 4.0, '2000-01-04'),
+ (5, 5, '5', 5.0, '2000-01-05');
+ """
+
+ sql """
+ INSERT INTO ${t3} VALUES
+ (1),
+ (4),
+ (5);
+ """
+
+ sql "sync"
+ qt_sql_1 "select * from ${t1}, ${t2}, ${t3} order by ${t1}.id, ${t1}.id1,
${t2}.id, ${t3}.id"
Review Comment:
why use a cross join case?
##########
fe/fe-core/src/main/antlr4/org/apache/doris/nereids/DorisParser.g4:
##########
@@ -438,6 +445,7 @@ primaryExpression
| identifier
#columnReference
| base=primaryExpression DOT fieldName=identifier
#dereference
| LEFT_PAREN expression RIGHT_PAREN
#parenthesizedExpression
+ | LEFT_PAREN namedExpression (COMMA namedExpression)+ RIGHT_PAREN
#rowConstructor
Review Comment:
if u use `rowConstructor` as expression. This mean u want to support syntax
like this `select (1, 2, 3)`. but u do not process it at all.
##########
fe/fe-core/src/test/java/org/apache/doris/nereids/trees/plans/ExplainInsertCommandTest.java:
##########
@@ -109,6 +109,16 @@ public void testInsertIntoSomeColumns() throws Exception {
Assertions.assertEquals(4,
getOutputFragment(sql).getOutputExprs().size());
}
+ @Test
+ public void testInsertIntoValues() throws Exception {
+ String sql = "explain insert into t1 values(1, 1, 1, 1), (2, 2, 2, 2),
(3, 3, 3, 3)";
Review Comment:
add some other expression rather than literal expression cases
--
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]
