cryptoe commented on a change in pull request #12163:
URL: https://github.com/apache/druid/pull/12163#discussion_r791563399
##########
File path: sql/pom.xml
##########
@@ -180,6 +180,11 @@
<artifactId>validation-api</artifactId>
<scope>provided</scope>
</dependency>
+ <dependency>
+ <groupId>org.slf4j</groupId>
+ <artifactId>slf4j-api</artifactId>
+ <version>1.7.25</version>
Review comment:
Can this come from the parent pom ?
##########
File path: sql/pom.xml
##########
@@ -255,6 +260,140 @@
</execution>
</executions>
</plugin>
+
+ <plugin>
+ <groupId>org.apache.maven.plugins</groupId>
+ <artifactId>maven-dependency-plugin</artifactId>
+ <executions>
+ <execution>
+ <!-- Extract parser grammar template from Apache Calcite and put
+ it under ${project.build.directory} where all freemarker
templates are. -->
+ <id>unpack-parser-template</id>
+ <phase>initialize</phase>
+ <goals>
+ <goal>unpack</goal>
+ </goals>
+ <configuration>
+ <artifactItems>
+ <artifactItem>
+ <groupId>org.apache.calcite</groupId>
+ <artifactId>calcite-core</artifactId>
+ <version>${calcite.version}</version>
+ <type>jar</type>
+ <overWrite>true</overWrite>
+
<outputDirectory>${project.build.directory}/</outputDirectory>
+ <includes>**/Parser.jj</includes>
+ </artifactItem>
+ <artifactItem>
+ <groupId>org.apache.calcite</groupId>
+ <artifactId>calcite-core</artifactId>
+ <version>${calcite.version}</version>
+ <type>jar</type>
+ <overWrite>true</overWrite>
+
<outputDirectory>${project.build.directory}/</outputDirectory>
+ <includes>**/config.fmpp</includes>
+ </artifactItem>
+ </artifactItems>
+ </configuration>
+ </execution>
+ </executions>
+ </plugin>
+
+ <plugin>
+ <groupId>com.googlecode.fmpp-maven-plugin</groupId>
+ <artifactId>fmpp-maven-plugin</artifactId>
+ <version>1.0</version>
Review comment:
Push to parent pom would be helpful
##########
File path: sql/src/main/codegen/config.fmpp
##########
@@ -0,0 +1,433 @@
+# 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.
+
+# This file is an FMPP (http://fmpp.sourceforge.net/) configuration file to
+# allow clients to extend Calcite's SQL parser to support application specific
+# SQL statements, literals or data types.
+#
+# Calcite's parser grammar file (Parser.jj) is written in javacc
+# (http://javacc.java.net/) with Freemarker (http://freemarker.org/) variables
+# to allow clients to:
+# 1. have custom parser implementation class and package name.
+# 2. insert new parser method implementations written in javacc to parse
+# custom:
+# a) SQL statements.
+# b) literals.
+# c) data types.
+# 3. add new keywords to support custom SQL constructs added as part of (2).
+# 4. add import statements needed by inserted custom parser implementations.
+#
+# Parser template file (Parser.jj) along with this file are packaged as
+# part of the calcite-core-<version>.jar under "codegen" directory.
Review comment:
As discussed please add the resource reference and a pointer in the
pom.xml where we have specified the calcite version mentioning this file is
version dependent.
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.druid.sql.calcite.parser;
+
+import org.apache.calcite.sql.SqlInsert;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlWriter;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+/**
+ * Extends the Insert call to hold custom paramaters specific to druid i.e.
PARTITION BY and CLUSTER BY
+ * This class extends the {@link SqlInsert} so that the node can be used for
further conversion
+ */
+public class DruidSqlInsert extends SqlInsert
+{
+ // Unsure if this should be kept as is, but this allows reusing super.unparse
+ public static final SqlOperator OPERATOR = SqlInsert.OPERATOR;
+
+ private final SqlNode partitionBy;
+ private final SqlNodeList clusterBy;
+
+ public DruidSqlInsert(
+ @Nonnull SqlInsert insertNode,
+ @Nullable SqlNode partitionBy,
+ @Nullable SqlNodeList clusterBy
+ )
+ {
+ super(
+ insertNode.getParserPosition(),
+ (SqlNodeList) insertNode.getOperandList().get(0), // No better getter
to extract this
+ insertNode.getTargetTable(),
+ insertNode.getSource(),
+ insertNode.getTargetColumnList()
+ );
+ this.partitionBy = partitionBy;
+ this.clusterBy = clusterBy;
+ }
+
+ @Nullable
+ public SqlNodeList getClusterBy()
+ {
+ return clusterBy;
+ }
+
+ @Nullable
+ public String getPartitionBy()
+ {
+ if (partitionBy == null) {
+ return null;
+ }
+ return SqlLiteral.unchain(partitionBy).toValue();
+ }
+
+ @Nonnull
+ @Override
+ public SqlOperator getOperator()
+ {
+ return OPERATOR;
+ }
+
+ @Override
+ public void unparse(SqlWriter writer, int leftPrec, int rightPrec)
+ {
+ super.unparse(writer, leftPrec, rightPrec);
+ if (partitionBy != null) {
+ writer.keyword("PARTITION");
+ writer.keyword("BY");
+ writer.keyword(getPartitionBy());
+ }
+ if (clusterBy != null) {
+ writer.sep("CLUSTER BY");
+ SqlWriter.Frame frame = writer.startList("", "");
+ for (SqlNode clusterByOpts : clusterBy.getList()) {
+ clusterByOpts.unparse(writer, leftPrec, rightPrec);
+ }
+ writer.endList(frame);
+ }
+ }
Review comment:
Add a toStringMethod ?
##########
File path:
sql/src/test/java/org/apache/druid/sql/calcite/CalciteInsertDmlTest.java
##########
@@ -280,6 +283,210 @@ public void testInsertFromExternal()
.verify();
}
+ @Test
Review comment:
Hey Liked the PR overall
I think we can add a few more test cases
1. ClusterBy on expressions directly
2. Partition by on "random strings"
##########
File path: sql/src/main/codegen/config.fmpp
##########
@@ -0,0 +1,433 @@
+# 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.
+
+# This file is an FMPP (http://fmpp.sourceforge.net/) configuration file to
+# allow clients to extend Calcite's SQL parser to support application specific
+# SQL statements, literals or data types.
+#
+# Calcite's parser grammar file (Parser.jj) is written in javacc
+# (http://javacc.java.net/) with Freemarker (http://freemarker.org/) variables
+# to allow clients to:
+# 1. have custom parser implementation class and package name.
+# 2. insert new parser method implementations written in javacc to parse
+# custom:
+# a) SQL statements.
+# b) literals.
+# c) data types.
+# 3. add new keywords to support custom SQL constructs added as part of (2).
+# 4. add import statements needed by inserted custom parser implementations.
+#
+# Parser template file (Parser.jj) along with this file are packaged as
+# part of the calcite-core-<version>.jar under "codegen" directory.
+
+data: {
+ parser: {
+ # Generated parser implementation package and class name.
+ package: "org.apache.druid.sql.calcite.parser",
+ class: "DruidSqlParserImpl",
+
+ # List of additional classes and packages to import.
+ # Example. "org.apache.calcite.sql.*", "java.util.List".
+ imports: [
+ "org.apache.calcite.sql.SqlNode"
+ "org.apache.calcite.sql.SqlInsert"
+ "org.apache.druid.sql.calcite.parser.DruidSqlInsert"
+ ]
+
+ # List of new keywords. Example: "DATABASES", "TABLES". If the keyword is
not a reserved
+ # keyword add it to 'nonReservedKeywords' section.
+ keywords: [
Review comment:
Make it non reserved ?
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -765,13 +785,53 @@ static ParsedNodes create(final SqlNode node) throws
ValidationException
if (query.getKind() == SqlKind.INSERT) {
insert = (SqlInsert) query;
query = insert.getSource();
+
+ // Processing to be done when the original query has either of the
PARTITION BY or CLUSTER BY clause
+ if (insert instanceof DruidSqlInsert) {
+ DruidSqlInsert druidSqlInsert = (DruidSqlInsert) insert;
+
+ ingestionGranularity = druidSqlInsert.getPartitionBy();
+
+ if (druidSqlInsert.getClusterBy() != null) {
+ // If we have a CLUSTER BY clause, extract the information in that
CLUSTER BY and create a new SqlOrderBy
+ // node
+ SqlNode offset = null;
+ SqlNode fetch = null;
+ SqlNodeList orderByList = null;
+
+ if (query instanceof SqlOrderBy) {
+ SqlOrderBy sqlOrderBy = (SqlOrderBy) query;
+ // Extract the query present inside the SqlOrderBy (which is
free of ORDER BY, OFFSET and FETCH clauses)
Review comment:
Maybe clarify the statement
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/parser/DruidSqlInsert.java
##########
@@ -0,0 +1,102 @@
+/*
+ * 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.druid.sql.calcite.parser;
+
+import org.apache.calcite.sql.SqlInsert;
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.SqlOperator;
+import org.apache.calcite.sql.SqlWriter;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+
+/**
+ * Extends the Insert call to hold custom paramaters specific to druid i.e.
PARTITION BY and CLUSTER BY
+ * This class extends the {@link SqlInsert} so that the node can be used for
further conversion
Review comment:
"further conversion" can be made more clearer
##########
File path:
sql/src/main/java/org/apache/druid/sql/calcite/planner/DruidPlanner.java
##########
@@ -199,6 +203,10 @@ public PlannerResult plan() throws SqlParseException,
ValidationException, RelCo
final ParsedNodes parsed =
ParsedNodes.create(planner.parse(plannerContext.getSql()));
+ if (parsed.getIngestionGranularity() != null) {
+
plannerContext.getQueryContext().put(QueryContexts.INGESTION_GRANULARITY,
parsed.getIngestionGranularity());
Review comment:
Maybe add a check on the parsed string.
--
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]
