lincoln-lil commented on code in PR #22197:
URL: https://github.com/apache/flink/pull/22197#discussion_r1141026773
##########
flink-table/flink-table-planner/src/test/scala/org/apache/flink/table/planner/plan/common/PartialInsertTest.scala:
##########
@@ -163,6 +192,21 @@ class PartialInsertTest(isBatch: Boolean) extends
TableTestBase {
"SELECT a,b,c,d,e,123 FROM MyTable"
)
}
+
+ @Test
+ def testPartialInsert(): Unit = {
Review Comment:
Ok
##########
flink-table/flink-table-api-java/src/main/java/org/apache/flink/table/operations/SinkModifyOperation.java:
##########
@@ -121,6 +135,11 @@ public QueryOperation getChild() {
return child;
}
+ /** return an empty array when no column list specified. */
Review Comment:
Good catch! comments should be updated here since the unnecessarily
'int[0][]' was replaced by null.
##########
flink-table/flink-table-planner/src/main/scala/org/apache/flink/table/planner/calcite/PreValidateReWriter.scala:
##########
@@ -401,6 +404,15 @@ object PreValidateReWriter {
SqlUtil.newContextException(pos, e)
}
+ private def validateUnsupportedCompositeColumn(id: SqlIdentifier): Unit = {
+ assert(id != null)
+ if (!id.isSimple) {
+ val pos = id.getParserPosition
+ // TODO add accurate msg s"column name must be a simple identifier,
composite column name '${id.toString}' is not supported yet"
Review Comment:
The length check seems ok when running style check, I didn't find a suitable
resource for the error message when do poc, and will see if we can get rid of
the TODO.
##########
flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/operations/SqlToOperationConverter.java:
##########
@@ -1558,8 +1572,16 @@ private Operation convertUpdate(SqlUpdate sqlUpdate) {
catalogManager.qualifyIdentifier(unresolvedTableIdentifier));
// get query
PlannerQueryOperation queryOperation = new
PlannerQueryOperation(tableModify);
+ // TODO
+ List<String> updateColumnList =
+ sqlUpdate.getTargetColumnList().stream()
+ .map(c -> ((SqlIdentifier) c).getSimple())
+ .collect(Collectors.toList());
return new SinkModifyOperation(
- contextResolvedTable, queryOperation,
SinkModifyOperation.ModifyType.UPDATE);
+ contextResolvedTable,
+ queryOperation,
+ null, // targetColumns
Review Comment:
yes, here should be updateColumnList, seems some local changes get lost..
I'll update it.
##########
flink-table/flink-table-planner/src/test/resources/org/apache/flink/table/planner/plan/batch/sql/RowLevelUpdateTest.xml:
##########
Review Comment:
I think manually reorder the test xml file will add more cost for
maintaining since the auto generated mechanism doesn't care about the method
order, and once we add some new tests in the future then just re-generate it is
ok, WDYT?
--
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
