[GitHub] [flink] twalthr commented on a diff in pull request #21203: [FLINK-21239][Table SQL/API] Upgrade Calcite to 1.28.0

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

twalthr commented on code in PR #21203:
URL: https://github.com/apache/flink/pull/21203#discussion_r1015472139


##########
flink-core/src/main/java/org/apache/flink/core/classloading/ComponentClassLoader.java:
##########
@@ -189,6 +190,14 @@ public URL getResource(final String name) {
         return null;
     }
 
+    @Override
+    public InputStream getResourceAsStream(String name) {
+        if (isOwnerFirstClass(name)) {

Review Comment:
   Shouldn't this be the same logic as below in `Enumeration<URL> getResources(final String name)`?
   ```
   if (isComponentFirstResource(name)) {
               ...
           }
           if (isOwnerFirstResource(name)) {
               ...
           }
           return loadResourceFromComponentOnly(name);
   ```



##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/functions/casting/AbstractCodeGeneratorCastRule.java:
##########
@@ -204,9 +204,10 @@ public String getSessionTimeZoneTerm() {
         }
 
         @Override
-        public String declareVariable(String type, String variablePrefix) {
+        public String declareVariable(String type, String variablePrefix, String defaultValue) {

Review Comment:
   Could we simply not update Janino in this PR to get rid of the changes in d690aa914b4a1a56036499f0e27b708f87281d4b? Once Janino 3.1.9 is released we can perform the update. 



##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -49,6 +49,18 @@ under the License.
 				</exclusion>
 			</exclusions>
 		</dependency>
+		<dependency>

Review Comment:
   Can you update `mvn dependency:tree` comment below? Who needs those two dependencies?



##########
flink-table/flink-table-planner/src/main/java/org/apache/calcite/tools/RelBuilder.java:
##########
@@ -0,0 +1,5161 @@
+/*
+ * 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.tools;

Review Comment:
   We offer a `FlinkRelBuilder` already. Can we override the affected methods instead of copying the entire `RelBuilder` class? It seems only the
   
   ```
   public RelBuilder join(JoinRelType joinType, RexNode condition,
         Set<CorrelationId> variablesSet) {
   ```
   
   Is affected?



##########
flink-table/flink-table-planner/pom.xml:
##########
@@ -49,6 +49,18 @@ under the License.
 				</exclusion>
 			</exclusions>
 		</dependency>
+		<dependency>

Review Comment:
   We would also need to add them to the NOTICE file and relocate if we keep them.



##########
flink-table/flink-sql-parser-hive/pom.xml:
##########
@@ -63,11 +63,51 @@ under the License.
 				</exclusion>
 			</exclusions>
 		</dependency>
+		<dependency>
+			<groupId>org.apache.commons</groupId>
+			<artifactId>commons-lang3</artifactId>
+		</dependency>
 		<dependency>
 			<groupId>org.apache.calcite</groupId>
 			<artifactId>calcite-core</artifactId>
 			<version>${calcite.version}</version>
 			<exclusions>

Review Comment:
   Can we add a `"mvn dependency:tree" as of Calcite 1.28.0:` comment here. It is quite difficult to understand what is left after so many exclusions.



-- 
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: issues-unsubscribe@flink.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org