[GitHub] [flink] wuchong commented on a change in pull request #13577: [FLINK-16579][table] Upgrade Calcite version to 1.26 for Flink SQL

[GitBox <gi...@apache.org>]

wuchong commented on a change in pull request #13577:
URL: https://github.com/apache/flink/pull/13577#discussion_r502761824



##########
File path: flink-table/flink-table-planner-blink/pom.xml
##########
@@ -189,10 +194,6 @@ under the License.
 					<groupId>org.apache.commons</groupId>
 					<artifactId>commons-dbcp2</artifactId>
 				</exclusion>
-				<exclusion>
-					<groupId>com.esri.geometry</groupId>
-					<artifactId>esri-geometry-api</artifactId>

Review comment:
       Why we have to depend on this dependency now?

##########
File path: flink-table/flink-sql-parser-hive/src/test/java/org/apache/flink/sql/parser/hive/FlinkHiveSqlParserImplTest.java
##########
@@ -35,15 +35,12 @@ protected SqlParserImplFactory parserImplFactory() {
 	}
 
 	// overrides test methods that we don't support
-	@Override

Review comment:
       What about changing to use `@Ignore` +	`@Test` annotations and also update the comment above?
   This can avoid the IDEA warns this method is never used. 

##########
File path: flink-table/flink-table-planner-blink/src/main/scala/org/apache/flink/table/planner/plan/schema/TableSourceTable.scala
##########
@@ -65,15 +65,8 @@ class TableSourceTable(
     statistic) {
 
   override def getQualifiedName: util.List[String] = {
-    val names = super.getQualifiedName
     val builder = ImmutableList.builder[String]()
-    builder.addAll(names)
-    if (dynamicOptions.size() != 0) {

Review comment:
       It seems that `dynamicOptions` is never used in this class, should we remove it?

##########
File path: flink-table/flink-sql-parser/src/test/java/org/apache/flink/sql/parser/FlinkSqlParserImplTest.java
##########
@@ -994,7 +992,6 @@ public void testShowViews() {
 
 	// Override the test because our ROW field type default is nullable,
 	// which is different with Calcite.
-	@Override

Review comment:
       Use `@Test` instead?

##########
File path: flink-table/flink-table-planner/src/main/java/org/apache/calcite/schema/Statistic.java
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.calcite.schema;
+
+import org.apache.calcite.rel.RelCollation;
+import org.apache.calcite.rel.RelDistribution;
+import org.apache.calcite.rel.RelReferentialConstraint;
+import org.apache.calcite.util.ImmutableBitSet;
+
+import java.util.Collections;
+import java.util.List;
+
+/**
+ * Statistics about a {@link Table}.
+ *
+ * <p>Each of the methods may return {@code null} meaning "not known".</p>
+ *
+ * <p>Changes:
+ *
+ * <ul>
+ *     <li>Line 61: default collations change from null to empty list.</li>

Review comment:
       Is it a temporary solution? Why we have to override the default value in interface?
   But this change is only needed in legacy planner?




----------------------------------------------------------------
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:
users@infra.apache.org