[GitHub] [flink] wenlong88 commented on a change in pull request #18215: [FLINK-25392][table-planner]Support new StatementSet syntax in planner and parser

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

wenlong88 commented on a change in pull request #18215:
URL: https://github.com/apache/flink/pull/18215#discussion_r777785120



##########
File path: flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/calcite/FlinkCalciteSqlValidator.java
##########
@@ -126,17 +125,4 @@ public void validateColumnListParams(
         // this makes it possible to ignore them in the validator and fall back to regular row types
         // see also SqlFunction#deriveType
     }
-

Review comment:
       I didn't get the confusion " validation code scattered through 2/3 different components of the codebase", I just added the validation in the PlannerImpl. I add cases in some part of  planner tests, because we need to make sure that we really support the new syntax e2e, just like other kind of sql statements. 
   
   I reverted the change you made in FLINK-22942, because I think the change was made in the wrong way, and it is necessary in the validation in StatementSet and Execute, that's why I made the change in this pr:
   1.  we should not call validateInsert directly because some metadata used in validation would missed.
   2. the former change didn't cover explain
   3. in my understanding, we put the validation of flink custom node in the PlannerImpl, such as SqlRichExplain/RichSqlInsert, because calcite validator cannot cover them, and we wan to put all of validation in one place. Another way is put the validation in the SqlRichExplain/RichSqlInsert by rewriting the SqlNode#validate, but it would make the validation scattering in every node.




-- 
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