lirui-apache commented on a change in pull request #11791:
URL: https://github.com/apache/flink/pull/11791#discussion_r414333859
##########
File path: flink-table/flink-sql-parser/pom.xml
##########
@@ -298,6 +348,23 @@ under the License.
<outputDirectory>${project.build.directory}/generated-sources/</outputDirectory>
</configuration>
</execution>
+ <execution>
+ <phase>generate-sources</phase>
+ <id>javacc-hive</id>
+ <goals>
+ <goal>javacc</goal>
+ </goals>
+ <configuration>
+
<sourceDirectory>${project.build.directory}/generated-sources-hive/</sourceDirectory>
Review comment:
We're not copying the parser file of Flink. The process of generating
the parser for Hive is:
1. Make a copy of the parser template in `calcite-core.jar`
2. Generate the fmpp sources from `codegen-hive` and output to
`generated-sources-hive`.
3. Call javacc to build the parser from `generated-sources-hive` and output
to `generated-sources`.
Finally, we'll have both Flink and Hive parser in `generated-sources`.
I agree moving the Hive parser to a separate module may make things a little
clearer. But it also brings redundancies. E.g. need extra pom file and
duplicated classes like `ParseException`. What do you think?
##########
File path:
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java
##########
@@ -247,7 +253,10 @@ public CatalogDatabase getDatabase(String databaseName)
throws DatabaseNotExistE
Map<String, String> properties = hiveDatabase.getParameters();
- properties.put(HiveCatalogConfig.DATABASE_LOCATION_URI,
hiveDatabase.getLocationUri());
+ boolean isGeneric = getObjectIsGeneric(properties);
+ if (!isGeneric) {
+
properties.put(SqlCreateHiveDatabase.DATABASE_LOCATION_URI,
hiveDatabase.getLocationUri());
+ }
Review comment:
I put the property keys in SqlNode because the SqlNode has to add these
extra properties for extended Hive semantics.
##########
File path:
flink-connectors/flink-connector-hive/src/main/java/org/apache/flink/table/catalog/hive/HiveCatalog.java
##########
@@ -1403,4 +1446,29 @@ public CatalogColumnStatistics
getPartitionColumnStatistics(ObjectPath tablePath
}
}
+ private static boolean createObjectIsGeneric(Map<String, String>
properties) {
+ // When creating an object, a hive object needs explicitly have
a key is_generic = false
Review comment:
It's not just about the default value. For example, when creating an
object and the `is_generic` key is missing, we'll add `is_generic=false` to the
properties.
Since the logic is different based on whether we're creating or retrieving
an object, I think it's better to reflect that in the method names.
##########
File path:
flink-table/flink-sql-parser/src/main/java/org/apache/flink/sql/parser/ddl/hive/HiveDDLUtils.java
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.flink.sql.parser.ddl.hive;
+
+import org.apache.flink.sql.parser.ddl.SqlTableOption;
+import org.apache.flink.sql.parser.impl.ParseException;
+import org.apache.flink.table.catalog.config.CatalogConfig;
+
+import org.apache.calcite.sql.SqlLiteral;
+import org.apache.calcite.sql.SqlNode;
+import org.apache.calcite.sql.SqlNodeList;
+import org.apache.calcite.sql.parser.SqlParserPos;
+
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.Set;
+
+import static
org.apache.flink.sql.parser.ddl.hive.SqlAlterHiveDatabase.ALTER_DATABASE_OP;
+import static
org.apache.flink.sql.parser.ddl.hive.SqlCreateHiveDatabase.DATABASE_LOCATION_URI;
+
+/**
+ * Util methods for Hive DDL Sql nodes.
+ */
+public class HiveDDLUtils {
+
+ private static final Set<String> RESERVED_DB_PROPERTIES = new
HashSet<>();
+
Review comment:
I agree it seems hacky. But the problem is some semantics are specific
to Hive and may never be adopted in Flink, e.g. the locations of table and
database. If we keep them in the SqlNode, the planner needs to be able handle
them, and the Catalog object (e.g. CatalogDatabase) needs extra fields for the
semantics. To avoid this, we have to encode the Hive-specific semantics as
properties, which has been mentioned and discussed in FLIP-123.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
