Hi Lincoln, Thanks for your suggestion. I've reviewed the comments from the previous PR review[1], and the agreement at the time was that any configuration options not included in ExecutionConfigOptions and OptimizerConfigOptions should have the Experimental annotation explicitly added. Since this annotation has been relatively stable from 1.9.0 until now, you make a valid point, and we can elevate it to the PublicEvolving level.
Please let me know if you have any questions. [1] https://github.com/apache/flink/pull/8980 Best, Jane On Tue, May 21, 2024 at 10:25 PM Lincoln Lee <lincoln.8...@gmail.com> wrote: > Hi Jane, > > Thanks for the updates! > > Just one small comment on the options in IncrementalAggregateRule > & RelNodeBlock, should we also change the API level from Experimental > to PublicEvolving? > > > Best, > Lincoln Lee > > > Jane Chan <qingyue....@gmail.com> 于2024年5月21日周二 16:41写道： > > > Hi all, > > > > Thanks for your valuable feedback! > > > > To @Xuannan > > > > For options to be moved to another module/package, I think we have to > > > mark the old option deprecated in 1.20 for it to be removed in 2.0, > > > according to the API compatibility guarantees[1]. We can introduce the > > > new option in 1.20 with the same option key in the intended class. > > > > > > Good point, fixed. > > > > To @Lincoln and @Benchao > > > > Thanks for sharing the insights into the historical context of which I > was > > unaware. I've reorganized the sheet. > > > > 3. Regarding WindowEmitStrategy, IIUC it is currently unsupported on TVF > > > window, so it's recommended to keep it untouched for now and follow up > in > > > FLINK-29692 > > > > > > How to tackle the configuration is up to whether to remove the legacy > > window aggregate in 2.0, and I've updated the FLIP to leverage this part > to > > FLINK-29692. > > > > Please let me know if that answers your questions or if you have other > > comments. > > > > Best, > > Jane > > > > > > On Mon, May 20, 2024 at 1:52 PM Ron Liu <ron9....@gmail.com> wrote: > > > > > Hi, Lincoln > > > > > > > 2. Regarding the options in HashAggCodeGenerator, since this new > > feature > > > has gone > > > through a couple of release cycles and could be considered for > > > PublicEvolving now, > > > cc @Ron Liu <ron9....@gmail.com> WDYT? > > > > > > Thanks for cc'ing me, +1 for public these options now. > > > > > > Best, > > > Ron > > > > > > Benchao Li <libenc...@apache.org> 于2024年5月20日周一 13:08写道： > > > > > > > I agree with Lincoln about the experimental features. > > > > > > > > Some of these configurations do not even have proper implementation, > > > > take 'table.exec.range-sort.enabled' as an example, there was a > > > > discussion[1] about it before. > > > > > > > > [1] https://lists.apache.org/thread/q5h3obx36pf9po28r0jzmwnmvtyjmwdr > > > > > > > > Lincoln Lee <lincoln.8...@gmail.com> 于2024年5月20日周一 12:01写道： > > > > > > > > > > Hi Jane, > > > > > > > > > > Thanks for the proposal! > > > > > > > > > > +1 for the changes except for these annotated as experimental ones. > > > > > > > > > > For the options annotated as experimental, > > > > > > > > > > +1 for the moving of IncrementalAggregateRule & RelNodeBlock. > > > > > > > > > > For the rest of the options, there are some suggestions: > > > > > > > > > > 1. for the batch related parameters, it's recommended to either > > delete > > > > > them (leaving the necessary defaults value in place) or leave them > as > > > > they > > > > > are. Including: > > > > > FlinkRelMdRowCount > > > > > FlinkRexUtil > > > > > BatchPhysicalSortRule > > > > > JoinDeriveNullFilterRule > > > > > BatchPhysicalJoinRuleBase > > > > > BatchPhysicalSortMergeJoinRule > > > > > > > > > > What I understand about the history of these options is that they > > were > > > > once > > > > > used for fine > > > > > tuning for tpc testing, and the current flink planner no longer > > relies > > > on > > > > > these internal > > > > > options when testing tpc[1]. In addition, these options are too > > obscure > > > > for > > > > > SQL users, > > > > > and some of them are actually magic numbers. > > > > > > > > > > 2. Regarding the options in HashAggCodeGenerator, since this new > > > feature > > > > > has gone > > > > > through a couple of release cycles and could be considered for > > > > > PublicEvolving now, > > > > > cc @Ron Liu <ron9....@gmail.com> WDYT? > > > > > > > > > > 3. Regarding WindowEmitStrategy, IIUC it is currently unsupported > on > > > TVF > > > > > window, so > > > > > it's recommended to keep it untouched for now and follow up in > > > > > FLINK-29692[2]. cc @Xuyang <xyzhong...@163.com> > > > > > > > > > > [1] > > > > > > > > > > > > > > > https://github.com/ververica/flink-sql-benchmark/blob/master/tools/common/flink-conf.yaml > > > > > [2] https://issues.apache.org/jira/browse/FLINK-29692 > > > > > > > > > > > > > > > Best, > > > > > Lincoln Lee > > > > > > > > > > > > > > > Yubin Li <lyb5...@gmail.com> 于2024年5月17日周五 10:49写道： > > > > > > > > > > > Hi Jane, > > > > > > > > > > > > Thank Jane for driving this proposal ! > > > > > > > > > > > > This makes sense for users, +1 for that. > > > > > > > > > > > > Best, > > > > > > Yubin > > > > > > > > > > > > On Thu, May 16, 2024 at 3:17 PM Jark Wu <imj...@gmail.com> > wrote: > > > > > > > > > > > > > > Hi Jane, > > > > > > > > > > > > > > Thanks for the proposal. +1 from my side. > > > > > > > > > > > > > > > > > > > > > Best, > > > > > > > Jark > > > > > > > > > > > > > > On Thu, 16 May 2024 at 10:28, Xuannan Su < > suxuanna...@gmail.com> > > > > wrote: > > > > > > > > > > > > > > > Hi Jane, > > > > > > > > > > > > > > > > Thanks for driving this effort! And +1 for the proposed > > changes. > > > > > > > > > > > > > > > > I have one comment on the migration plan. > > > > > > > > > > > > > > > > For options to be moved to another module/package, I think we > > > have > > > > to > > > > > > > > mark the old option deprecated in 1.20 for it to be removed > in > > > 2.0, > > > > > > > > according to the API compatibility guarantees[1]. We can > > > introduce > > > > the > > > > > > > > new option in 1.20 with the same option key in the intended > > > class. > > > > > > > > WDYT? > > > > > > > > > > > > > > > > Best, > > > > > > > > Xuannan > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > https://nightlies.apache.org/flink/flink-docs-master/docs/ops/upgrading/#api-compatibility-guarantees > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Wed, May 15, 2024 at 6:20 PM Jane Chan < > > qingyue....@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > Hi all, > > > > > > > > > > > > > > > > > > I'd like to start a discussion on FLIP-457: Improve > Table/SQL > > > > > > > > Configuration > > > > > > > > > for Flink 2.0 [1]. This FLIP revisited all Table/SQL > > > > configurations > > > > > > to > > > > > > > > > improve user-friendliness and maintainability as Flink > moves > > > > toward > > > > > > 2.0. > > > > > > > > > > > > > > > > > > I am looking forward to your feedback. > > > > > > > > > > > > > > > > > > Best regards, > > > > > > > > > Jane > > > > > > > > > > > > > > > > > > [1] > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=307136992 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > Best, > > > > Benchao Li > > > > > > > > > >
