Thanks Jane for your feedback. `EXPLAIN PLAN_ADVICE` looks good to me.
Best, Jingsong On Thu, Jan 5, 2023 at 5:20 PM Jane Chan <qingyue....@gmail.com> wrote: > > Hi, devs, > > After discussing with Godfrey <godfre...@gmail.com>, Lincoln > <lincoln.8...@gmail.com>, and Jark <imj...@gmail.com>, I've updated the > FLIP document[1] and look forward to your opinions and suggestions. > > The highlight difference is listed as follows. > > - *The proposed syntax changes from EXPLAIN ANALYZED_PHYSICAL_PLAN > <query> to EXPLAIN PLAN_ADVICE <query>*. > - The reason for changing the syntax is that the output format and > analyzed target are two orthogonal concepts and better be > decoupled. On the > other hand, users may care about the advice content instead of which > plan > is analyzed, and thus PHYSICAL should be kept from users. > > > - *The output format changes from JSON to current tree-style text. > Introduce ExplainFormat to classify the output format.* > - The current output format is a mixture of plain text (AST, > Optimized Physical Plan, and Optimized Execution Plan) and JSON > (Physical > Execution Plan, via EXPLAIN JSON_EXECUTION_PLAN ), which is not > structured > and categorized. By introducing ExplainFormat, we can better classify > the > output format and have more flexibility to extend more formats in the > future. > > > - *The PlanAnalyzer installation gets rid of SPI.* > - PlanAnalyzer should be an internal interface and not be exposed to > users. Therefore, the Factory mechanism is unsuitable for this. > > > To Godfrey <godfre...@gmail.com>, Jingsong <jingsongl...@gmail.com>, and > Shengkai <fskm...@gmail.com>, Thanks for your comments and questions. > > @Jingsong > > > Can you give examples of other systems for the syntax? > > In other systems, is EXPLAIN ANALYZE already PHYSICAL_PLAN? > > > > For other systems like MySQL[2], PostgreSQL[3], Presto[4], and TiDB[5] > > EXPLAIN ANALYZE <query> > is the mainstream syntax. > > However, it represents an actual measurement of the cost, i.e., the > statement will execute the statement, which is unsuitable for this > condition. > > > `EXPLAIN ANALYZED_PHYSICAL_PLAN <query>` looks a bit strange, and even > > stranger that it contains `advice`. The purpose of FLIP seems to be a bit > > more to `advice`, so can we just > > introduce a syntax for `advice`? > > > Good point. After several discussions, the syntax has been updated to > > EXPLAIN PLAN_ADVICE <query> > > @Godfrey > > Do we really need to expose `PlanAnalyzerFactory` as public interface? > > I prefer we only expose ExplainDetail#ANALYZED_PHYSICAL_PLAN and the > > analyzed result. > > Which is enough for users and consistent with the results of `explain` > > method.The classes about plan analyzer are in table planner module, which > > does not public api (public interfaces should be defined in > > flink-table-api-java module). And PlanAnalyzer is depend on RelNode, which > > is internal class of planner, and not expose to users. > > > Good point. After reconsideration, the SPI mechanism is removed from the > FLIP. PlanAnalyzer should be an internal implementation much similar to a > RelOptRule, and should not be exposed to users. > > @Shengkai > > > 1. `PlanAnalyzer#analyze` uses the FlinkRelNode as the input. Could you > > share some thoughts about the motivation? In my experience, users mainly > > care about 2 things when they develop their job: > > a. Why their SQL can not work? For example, their streaming SQL contains an > > OVER window but their ORDER key is not ROWTIME. In this case, we may don't > > have a physical node or logical node because, during the optimization, the > > planner has already thrown the exception. > > > > The prerequisite for providing advice is that the optimized physical can > be generated. The planner should throw exceptions if the query contains > syntax errors or other problems. > > > > > b. Many users care about whether their state is compatible after upgrading > > their Flink version. In this case, I think the old execplan and the SQL > > statement are the user's input. > > > Good point. State compatibility detection is beneficial, but it better be > decoupled with EXPLAIN PLAN_ADVICE. We could provide a separate mechanism > for cross-version validation. > > > 2. I am just curious how other people add the rules to the Advisor. When > > rules increases, all these rules should be added to the Flink codebase? > > > It is much similar to adding a RelOptRule to RuleSet. The number of > analyzers will not be growing too fast. So adding them to the Flink > codebase may not be a concern. > > > 3. How do users configure another advisor? > > > After reconsideration, I would like to let PlanAdvisor be an internal > interface, which is different from implementing a custom connector/format. > > [1] > https://cwiki.apache.org/confluence/display/FLINK/FLIP-280%3A+Introduce+EXPLAIN+PLAN_ADVICE+to+provide+SQL+advice > [2] https://dev.mysql.com/doc/refman/8.0/en/explain.html#explain-analyze > [3] https://www.postgresql.org/docs/current/sql-explain.html > [4] https://prestodb.io/docs/current/sql/explain-analyze.html > [5] https://docs.pingcap.com/tidb/dev/sql-statement-explain-analyze > > Best regards, > Jane > > On Tue, Jan 3, 2023 at 6:20 PM Jingsong Li <jingsongl...@gmail.com> wrote: > > > Thanks Jane for the FLIP! It looks very nice! > > > > Can you give examples of other systems for the syntax? > > In other systems, is EXPLAIN ANALYZE already PHYSICAL_PLAN? > > > > `EXPLAIN ANALYZED_PHYSICAL_PLAN <query>` looks a bit strange, and even > > stranger that it contains `advice`. > > > > The purpose of FLIP seems to be a bit more to `advice`, so can we just > > introduce a syntax for `advice`? > > > > Best, > > Jingsong > > > > On Tue, Jan 3, 2023 at 3:40 PM godfrey he <godfre...@gmail.com> wrote: > > > > > > Thanks for driving this discussion. > > > > > > Do we really need to expose `PlanAnalyzerFactory` as public interface? > > > I prefer we only expose ExplainDetail#ANALYZED_PHYSICAL_PLAN and the > > > analyzed result. > > > Which is enough for users and consistent with the results of `explain` > > method. > > > > > > The classes about plan analyzer are in table planner module, which > > > does not public api > > > (public interfaces should be defined in flink-table-api-java module). > > > And PlanAnalyzer is depend on RelNode, which is internal class of > > > planner, and not expose to users. > > > > > > Bests, > > > Godfrey > > > > > > > > > Shengkai Fang <fskm...@gmail.com> 于2023年1月3日周二 13:43写道： > > > > > > > > Sorry for the missing answer about the configuration of the Analyzer. > > Users > > > > may don't need to configure this with SQL statements. In the SQL > > Gateway, > > > > users can configure the endpoints with the option > > `sql-gateway.endpoint.type` > > > > in the flink-conf. > > > > > > > > Best, > > > > Shengkai > > > > > > > > Shengkai Fang <fskm...@gmail.com> 于2023年1月3日周二 12:26写道： > > > > > > > > > Hi, Jane. > > > > > > > > > > Thanks for bringing this to the discussion. I have some questions > > about > > > > > the FLIP: > > > > > > > > > > 1. `PlanAnalyzer#analyze` uses the FlinkRelNode as the input. Could > > you > > > > > share some thoughts about the motivation? In my experience, users > > mainly > > > > > care about 2 things when they develop their job: > > > > > > > > > > a. Why their SQL can not work? For example, their streaming SQL > > contains > > > > > an OVER window but their ORDER key is not ROWTIME. In this case, we > > may > > > > > don't have a physical node or logical node because, during the > > > > > optimization, the planner has already thrown the exception. > > > > > > > > > > b. Many users care about whether their state is compatible after > > upgrading > > > > > their Flink version. In this case, I think the old execplan and the > > SQL > > > > > statement are the user's input. > > > > > > > > > > So, I think we should introduce methods like > > `PlanAnalyzer#analyze(String > > > > > sql)` and `PlanAnalyzer#analyze(String sql, ExecnodeGraph)` here. > > > > > > > > > > 2. I am just curious how other people add the rules to the Advisor. > > When > > > > > rules increases, all these rules should be added to the Flink > > codebase? > > > > > 3. How do users configure another advisor? > > > > > > > > > > Best, > > > > > Shengkai > > > > > > > > > > > > > > > > > > > > Jane Chan <qingyue....@gmail.com> 于2022年12月28日周三 12:30写道： > > > > > > > > > >> Hi @yuxia, Thank you for reviewing the FLIP and raising questions. > > > > >> > > > > >> 1: Is the PlanAnalyzerFactory also expected to be implemented by > > users > > > > >> just > > > > >> > like DynamicTableSourceFactory or other factories? If so, I > > notice that > > > > >> in > > > > >> > the code of PlanAnalyzerManager#registerAnalyzers, the code is as > > > > >> follows: > > > > >> > FactoryUtil.discoverFactory(classLoader, > > PlanAnalyzerFactory.class, > > > > >> > StreamPlanAnalyzerFactory.STREAM_IDENTIFIER)); IIUC, it'll always > > find > > > > >> the > > > > >> > factory with the name > > StreamPlanAnalyzerFactory.STREAM_IDENTIFIER; Is > > > > >> it a > > > > >> > typo or by design ? > > > > >> > > > > >> > > > > >> This is a really good open question. For the short answer, yes, it > > is by > > > > >> design. I'll explain the consideration in more detail. > > > > >> > > > > >> The standard procedure to create a custom table source/sink is to > > > > >> implement > > > > >> the factory and the source/sink class. There is a strong 1v1 > > relationship > > > > >> between the factory and the source/sink. > > > > >> > > > > >> SQL > > > > >> > > > > >> DynamicTableSourceFactory > > > > >> > > > > >> Source > > > > >> > > > > >> create table … with (‘connector’ = ‘foo’) > > > > >> > > > > >> #factoryIdentifer.equals(“foo”) > > > > >> > > > > >> FooTableSource > > > > >> > > > > >> > > > > >> *Apart from that, the custom function module is another kind of > > > > >> implementation. The factory creates a collection of functions. This > > is a > > > > >> 1vN relationship between the factory and the functions.* > > > > >> > > > > >> SQL > > > > >> > > > > >> ModuleFactory > > > > >> > > > > >> Function > > > > >> > > > > >> load module ‘bar’ > > > > >> > > > > >> #factoryIdentifier.equals(“bar”) > > > > >> > > > > >> A collection of functions > > > > >> > > > > >> Back to the plan analyzers, if we choose the first style, we also > > need to > > > > >> expose a new SQL syntax to users, like "CREATE ANALYZER foo WITH > > ..." to > > > > >> specify the factory identifier. But I think it is too heavy because > > an > > > > >> analyzer is an auxiliary tool to help users write better queries, > > and thus > > > > >> it should be exposed at the API level other than the user syntax > > level. > > > > >> > > > > >> As a result, I propose to follow the second style. Then we don't > > need to > > > > >> introduce new syntax to create analyzers. Let > > StreamPlanAnalyzerFactory be > > > > >> the default factory to create analyzers under the streaming mode, > > and the > > > > >> custom analyzers will register themselves in > > StreamPlanAnalyzerFactory. > > > > >> > > > > >> @Override > > > > >> public List<PlanAnalyzer> createAnalyzers() { > > > > >> return Arrays.asList( > > > > >> FooAnalyzer.INSTANCE, > > > > >> BarAnalyzer.INSTANCE, > > > > >> ...); > > > > >> } > > > > >> > > > > >> > > > > >> 2: Is there any special reason make PlanAdvice be a final class? > > Would it > > > > >> > be better to make it an interface and we provide a default > > > > >> implementation? > > > > >> > My concern is some users may want have their own implementation > > for > > > > >> > PlanAdvice. But it may be overthinking. If you think it won't > > bring any > > > > >> > problem, I'm also fine with that. > > > > >> > > > > >> > > > > >> The reason why making PlanAdvice final is that I think users would > > prefer > > > > >> to implement the custom PlanAnalyzer than PlanAdvice. PlanAdvice is > > a POJO > > > > >> class to represent the analyzed result provided by PlanAnalyzer. > > > > >> > > > > >> > > > > >> 3: Is there a way only show advice? For me, it seems the advice > > will be > > > > >> > more useful and the nodes may contains to many details. > > > > >> > > > > >> > > > > >> The result contains two parts: the optimized physical plan itself + > > the > > > > >> analysis of the plan. > > > > >> > > > > >> For PlanAdvice with the scope as GLOBAL, it is possible to do so. > > While > > > > >> for > > > > >> a LOCAL scope, the advice content is specific to certain nodes > > (E.g., some > > > > >> certain rel nodes are sensitive to state TTL configuration). In this > > > > >> situation, the plan cannot be omitted. On the other hand, the plan > > is > > > > >> necessary from the visualization perspective. During the PoC phase, > > I made > > > > >> some attempts to adapt the Flink Visualizer to illustrate the > > analyzed > > > > >> plan, and it looks like the following pic. I think this is > > intuitive to > > > > >> help users understand their queries and what they can do according > > to the > > > > >> advice. > > > > >> > > > > >> > > > > >> > > > > >> 4: I'm curious about what't the global advice will look like. Is it > > > > >> > possible to provide an example? > > > > >> > > > > >> > > > > >> Here is an example to illustrate the non-deterministic update issue. > > > > >> > > > > >> create temporary table cdc_with_meta ( > > > > >> a int, > > > > >> b bigint, > > > > >> c string, > > > > >> d boolean, > > > > >> metadata_1 int metadata, > > > > >> metadata_2 string metadata, > > > > >> metadata_3 bigint metadata, > > > > >> primary key (a) not enforced > > > > >> ) with ( > > > > >> 'connector' = 'values', > > > > >> 'changelog-mode' = 'I,UA,UB,D', > > > > >> 'readable-metadata' = 'metadata_1:INT, metadata_2:STRING, > > > > >> metadata_3:BIGINT' > > > > >> ); > > > > >> > > > > >> create temporary table sink_without_pk ( > > > > >> a int, > > > > >> b bigint, > > > > >> c string > > > > >> ) with ( > > > > >> 'connector' = 'values', > > > > >> 'sink-insert-only' = 'false' > > > > >> ); > > > > >> > > > > >> insert into sink_without_pk > > > > >> select a, metadata_3, c > > > > >> from cdc_with_meta; > > > > >> > > > > >> And with compilation as SCHEMA, the result is as below. > > > > >> > > > > >> { > > > > >> "nodes" : [ { > > > > >> "id" : 1, > > > > >> "type" : "StreamPhysicalTableSourceScan", > > > > >> "digest" : "TableSourceScan(table=[[default_catalog, > > default_database, > > > > >> cdc_with_meta, project=[a, c], metadata=[metadata_3]]], fields=[a, > > c, > > > > >> metadata_3], upsertKeys=[[a]])", > > > > >> "changelog_mode" : "I,UB,UA,D" > > > > >> }, { > > > > >> "id" : 2, > > > > >> "type" : "StreamPhysicalCalc", > > > > >> "digest" : "Calc(select=[a, metadata_3, c], upsertKeys=[[a]])", > > > > >> "changelog_mode" : "I,UB,UA,D", > > > > >> "predecessors" : [ { > > > > >> "id" : 1, > > > > >> "distribution" : "ANY", > > > > >> "changelog_mode" : "I,UB,UA,D" > > > > >> } ] > > > > >> }, { > > > > >> "id" : 3, > > > > >> "type" : "StreamPhysicalSink", > > > > >> "digest" : > > > > >> "Sink(table=[default_catalog.default_database.sink_without_pk], > > > > >> fields=[a, metadata_3, c])", > > > > >> "changelog_mode" : "NONE", > > > > >> "predecessors" : [ { > > > > >> "id" : 2, > > > > >> "distribution" : "ANY", > > > > >> "changelog_mode" : "I,UB,UA,D" > > > > >> } ] > > > > >> } ], > > > > >> "advice" : [ { > > > > >> "kind" : "WARNING", > > > > >> "scope" : "GLOBAL", > > > > >> "content" : "The metadata column(s): 'metadata_3' in cdc source > > may > > > > >> cause wrong result or error on downstream operators, please consider > > > > >> removing these columns or use a non-cdc source that only has insert > > > > >> messages.\nsource node:\nTableSourceScan(table=[[default_catalog, > > > > >> default_database, cdc_with_meta, project=[a, c], > > metadata=[metadata_3]]], > > > > >> fields=[a, c, metadata_3], changelogMode=[I,UB,UA,D], > > upsertKeys=[[a]])\n" > > > > >> } ] > > > > >> } > > > > >> > > > > >> > > > > >> Best regards, > > > > >> Jane Chan > > > > >> > > > > >> On Tue, Dec 27, 2022 at 8:06 PM yuxia <luoyu...@alumni.sjtu.edu.cn> > > > > >> wrote: > > > > >> > > > > >> > Thanks for driving this FLIP. It should be a good improvement to > > users. > > > > >> > But I have few questions: > > > > >> > 1: Is the PlanAnalyzerFactory also expected to be implemented by > > users > > > > >> > just like DynamicTableSourceFactory or other factories? If so, I > > notice > > > > >> > that in the code of PlanAnalyzerManager#registerAnalyzers, the > > code is > > > > >> as > > > > >> > follows: > > > > >> > FactoryUtil.discoverFactory(classLoader, > > PlanAnalyzerFactory.class, > > > > >> > StreamPlanAnalyzerFactory.STREAM_IDENTIFIER)); > > > > >> > > > > > >> > IIUC, it'll always find the factory with the name > > > > >> > StreamPlanAnalyzerFactory.STREAM_IDENTIFIER; Is it a typo or by > > design ? > > > > >> > > > > > >> > 2: Is there any special reason make PlanAdvice be a final class? > > Would > > > > >> it > > > > >> > be better to make it an interface and we provide a default > > > > >> implementation? > > > > >> > My concern is some users may want have their own implementation > > for > > > > >> > PlanAdvice. But it may be overthinking. If you think it won't > > bring any > > > > >> > problem, I'm also fine with that. > > > > >> > > > > > >> > 3: Is there a way only show advice? For me, it seems the advice > > will be > > > > >> > more useful and the nodes may contains to many details. > > > > >> > > > > > >> > 4: I'm curious about what't the global advice will look like. Is > > it > > > > >> > possible to provide an example? > > > > >> > > > > > >> > > > > >> > > > > >> > > > > >> > > > > > >> > Best regards, > > > > >> > Yuxia > > > > >> > > > > > >> > ----- 原始邮件 ----- > > > > >> > 发件人: "Jane Chan" <qingyue....@gmail.com> > > > > >> > 收件人: "dev" <dev@flink.apache.org> > > > > >> > 发送时间: 星期一, 2022年 12 月 26日 下午 9:39:18 > > > > >> > 主题: [DISCUSS] FLIP-280: Introduce a new explain mode to provide > > SQL > > > > >> advice > > > > >> > > > > > >> > Hi, devs, > > > > >> > > > > > >> > I would like to start a discussion on FLIP-280: Introduce a new > > explain > > > > >> > mode to provide SQL advice[1]. > > > > >> > > > > > >> > Currently, Flink SQL EXPLAIN statement provides three details to > > display > > > > >> > the plan. However, there is a considerable gap between the current > > > > >> > explained result and the actual, applicable actions for users to > > improve > > > > >> > their queries. > > > > >> > > > > > >> > To provide more understandable, actionable advice closer to the > > user's > > > > >> > perspective, we propose a new explain mode that analyzes the > > physical > > > > >> plan > > > > >> > and attaches available tuning advice and data correctness > > warnings. > > > > >> > > > > > >> > EXPLAIN ANALYZED_PHYSICAL_PLAN <query> > > > > >> > > > > > >> > I've included more details in [1], and I look forward to your > > feedback. > > > > >> > > > > > >> > [1] > > > > >> > > > > > >> > > > > > >> > > https://cwiki.apache.org/confluence/display/FLINK/FLIP-280%3A+Support+EXPLAIN+SQL+statements+with+advice > > > > >> > [2] POC: https://github.com/LadyForest/flink/tree/FLIP-280 > > > > >> > > > > > >> > Best regards, > > > > >> > Jane Chan > > > > >> > > > > > >> > > > > > > >
