[GitHub] [flink] slinkydeveloper commented on pull request #18573: [FLINK-25841][table] Expose plan via TableEnvironment.compilePlanSql/executePlan

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

slinkydeveloper commented on pull request #18573:
URL: https://github.com/apache/flink/pull/18573#issuecomment-1025884523


   Please note that the second commit of this PR _Splitted ExecNodeGraphJsonPlanGenerator[...]_ has been extracted to a separate PR: https://github.com/apache/flink/pull/18572 
   
   I have a couple of open questions before finishing to polish this PR and mark as ready to review:
   
   * Do we want to remove the old "json plan" apis in `TableEnvironmentInternal`? They are not useful anymore with this PR and they are internal and experimental. IMO we should, so I added a commit at the end to show how the end result would look like without them. 
   * Should we expose explain for `CompiledPlan`? Should we keep it internal in `TableEnvironment`? In the FLIP there is no mention about it. IMO we should expose it, and you see it's committed. I can remove it if you want.
   * Do we expect users to load plans from classpath? I personally don't see a use case where this is necessary, as i can't see any reason why you would embed the plan in your jar, because the pattern users should follow always look like "if plan not exist, then compile it". But I might be totally wrong here. For our internal testing purpose, I added to `JsonPlanTestBase` a method to create a `PlanReference` starting from the classpath file path. I can promote it to `PlanReference#fromClasspath` if you think we need it.  
   
   @twalthr @matriv any opinions?
   
   


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