Airblader commented on a change in pull request #17204:
URL: https://github.com/apache/flink/pull/17204#discussion_r705016537
##########
File path:
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java
##########
@@ -125,4 +126,14 @@ public void validateColumnListParams(
// this makes it possible to ignore them in the validator and fall
back to regular row types
// see also SqlFunction#deriveType
}
+
+ @Override
+ public void validateInsert(SqlInsert insert) {
+ super.validateInsert(insert);
+
+ // We don't support UPSERT INTO semantics (see FLINK-22942).
Review comment:
Maybe it would be better to open a JIRA issue for supporting UPSERT (as
a placeholder, simply stating that the semantics need to be defined first) and
then link to that ticket from here rather than the one that causes us to
disallow it? What do you think?
##########
File path:
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/api/stream/sql/validation/UpsertIntoValidationTest.scala
##########
@@ -0,0 +1,47 @@
+/*
+ * 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.table.api.stream.sql.validation
+
+import org.apache.flink.table.api._
+import org.apache.flink.table.planner.utils.TableTestBase
+import org.junit.rules.ExpectedException
+import org.junit.{Rule, Test}
+
+class UpsertIntoValidationTest extends TableTestBase {
Review comment:
We generally try to not write _new_ Scala code unless necessary. If we
keep this test, we should write it in Java instead (it'll look almost exactly
the same).
Instead of the `values` test connector, we could use a new harness
infrastructure we recently introduced, see `TableFactoryHarness`. We also have
new APIs to create tables programmatically rather than through SQL strings
(which are quite nice to read in Scala, but unfortunately not in Java). We can
also chat about this more directly if you want!
However, integration tests like these are quite expensive (in terms of
execution time), so we should use them sparingly. Testing that something is
_not_ supported probably doesn't quite warrant such an expensive test since we
don't really need to test any runtime behavior here, but just the validation
path. I think we might be able to create a test for the validator here by using
something like `PlannerMocks#createDefaultPlanner`, either directly (with some
refactoring), or as inspiration. `SqlToOperationConverterTest` also does that.
--
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-unsubscr...@flink.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
