Hi, devs, I have started the vote[1] for this FLIP. If you have any questions, please don't hesitate to reply to this thread.
[1] https://lists.apache.org/thread/bsgqvvs9wx1dkv7p3m9ctockh84rl11j Best, Jane On Fri, Jan 6, 2023 at 6:48 PM Jane Chan <qingyue....@gmail.com> wrote: > Hi, devs, > > Thanks for all the feedback. > > Based on the discussion[1], we seem to have a consensus so far, so I would > like to start a vote on FLIP-280[2], which begins on the following Monday > (Jan 9th at 10:00 AM GMT). > > If you have any questions or doubts, please do not hesitate to follow up > on this discussion. > > Best, > Jane > > [1] https://lists.apache.org/thread/5xywxv7g43byoh0jbx1b6qo6gx6wjkcz > [2] > https://cwiki.apache.org/confluence/display/FLINK/FLIP-280%3A+Introduce+EXPLAIN+PLAN_ADVICE+to+provide+SQL+advice > > On Fri, Jan 6, 2023 at 4:27 PM Jingsong Li <jingsongl...@gmail.com> wrote: > >> 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 >> > > > > >> > >> > > > > >> >> > > > > > >> > > >> >
