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-unsubscr...@flink.apache.org

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


Reply via email to