Hi Xuyang, Thank you for the clarification. I now have a clear understanding of the issue.
Actually the work that supports unaligned window in window agg operator is > included in this flip. We will always try to implement > features in window agg operator first and then consider falling back to > legacy group window agg if there are too many problems. > Could you provide more details on this part? My initial concerns arose from the fact that, aside from a brief mention, there seems to be a lack of comprehensive exploration on this topic, and notably, it's also absent from the PoC code. Best, Jane On Tue, Dec 12, 2023 at 2:55 PM Xuyang <xyzhong...@163.com> wrote: > Hi, Jane. > > > Thanks for your feedback. > > > >1. Let's set aside the workload for a moment and clearly conceptualize the > >ideal implementation in the end state, or at the very least, document the > > >current limitations in this FLIP to prevent them from being overlooked. > > > Actually the work that supports unaligned window in window agg operator is > included in this flip. We will always try to implement > features in window agg operator first and then consider falling back to > legacy group window agg if there are too many problems. > >2. Can we consider maintaining consistency at the ExecNode level for the > >session window TVF with other TVFs? > You're right, thanks for your reminder. I'll move the logic about fallback > to legacy legacy group window agg inside the exec node instead of physical > node. > > > > -- > > Best！ > Xuyang > > > > > > At 2023-12-11 20:38:25, "Jane Chan" <qingyue....@gmail.com> wrote: > >Hi Xuyang, > > > >Thanks for driving this discussion. > > > >I briefly reviewed the first PoC code[1] you provided for implementing the > >session window TVF, and I noticed a problem. In the `translateToExecNode` > >method of `StreamPhysicalWindowAggregate`, the session window TVF is > >translated into the legacy `StreamExecGroupWindowAggregate` instead of > >`StreamExecWindowAggregate`. I can guess that the reason for this is > >because we currently lack an operator that supports unaligned windows (and > >session windows belong to this category), but this means that we are only > >supporting session window TVF at the syntax level. My concern is that, if > >we support it in this way, it will be very difficult to get rid of the > >legacy implementation in forthcoming 2.0. > > > >Certainly, I understand that supporting it at the operator level would > >significantly increase the scope of work for this FLIP. Here my cents: > > > >1. Let's set aside the workload for a moment and clearly conceptualize the > >ideal implementation in the end state, or at the very least, document the > >current limitations in this FLIP to prevent them from being overlooked. > > > >2. Can we consider maintaining consistency at the ExecNode level for the > >session window TVF with other TVFs? By doing so, even if we introduce the > >actual unaligned window operator in a future 2.x release, we could still > >ensure that the session TVF from the version it is supported can be > >successfully deserialized through the `CompiledPlan`. > > > >WDYT? > > > >Best, > >Jane > > > >[1] https://github.com/xuyangzhong/flink/tree/FLINK-24024 > > > >On Mon, Dec 11, 2023 at 11:41 AM Xuyang <xyzhong...@163.com> wrote: > > > >> Hi, Jim. > >> >As a clarification, since FLINK-24204 is finishing up work from > >> >FLIP-145[1], do we need to discuss anything before you work out the > >> details > >> >of FLINK-24024 as a PR? > >> Which issue do you mean? It seems that FLINK-24204[1] is the issue with > >> table api&sql type system. > >> > >> > >> > I've got a PR up [3] for moving at least one of the classes you are > >> touching. > >> Nice work! Since we are not going to delete the legacy group window agg > >> operator actually, the only compatibility issue > >> may be that when using flink sql, the legacy group window agg operator > >> will be rewritten into new operators. Will these tests be affected about > >> this rewritten? > >> > >> > >> > >> > >> [1] https://issues.apache.org/jira/browse/FLINK-24204 > >> > >> > >> > >> > >> > >> > >> -- > >> > >> Best！ > >> Xuyang > >> > >> > >> > >> > >> > >> At 2023-12-09 06:25:30, "Jim Hughes" <jhug...@confluent.io.INVALID> > wrote: > >> >Hi Xuyang, > >> > > >> >As a clarification, since FLINK-24204 is finishing up work from > >> >FLIP-145[1], do we need to discuss anything before you work out the > >> details > >> >of FLINK-24024 as a PR? > >> > > >> >Relatedly, as that goes up for a PR, as part of FLINK-33421 [2], Bonnie > >> and > >> >I are working through migrating some of the JsonPlan Tests and ITCases > to > >> >RestoreTests. I've got a PR up [3] for moving at least one of the > classes > >> >you are touching. Let me know if I can share any details about that > work. > >> > > >> >Cheers, > >> > > >> >Jim > >> > > >> >1. > >> > > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-145%3A+Support+SQL+windowing+table-valued+function#FLIP145:SupportSQLwindowingtablevaluedfunction-SessionWindows > >> > > >> >2. https://issues.apache.org/jira/browse/FLINK-33421 > >> >3. https://github.com/apache/flink/pull/23886 > >> >https://issues.apache.org/jira/browse/FLINK-33676 > >> > > >> >On Tue, Nov 28, 2023 at 7:31 AM Xuyang <xyzhong...@163.com> wrote: > >> > > >> >> Hi all. > >> >> I'd like to start a discussion of FLIP-392: Deprecate the Legacy > Group > >> >> Window Aggregation. > >> >> > >> >> > >> >> Although the current Flink SQL Window Aggregation documentation[1] > >> >> indicates that the legacy Group Window Aggregation > >> >> syntax has been deprecated, the new Window TVF Aggregation syntax has > >> not > >> >> fully covered all of the features of the legacy one. > >> >> > >> >> > >> >> Compared to Group Window Aggergation, Window TVF Aggergation has > several > >> >> advantages, such as two-stage optimization, > >> >> support for standard GROUPING SET syntax, and so on. However, it > needs > >> to > >> >> supplement and enrich the following features. > >> >> > >> >> > >> >> 1. Support for SESSION Window TVF Aggregation > >> >> 2. Support for consuming CDC stream > >> >> 3. Support for HOP window size with non-integer step length > >> >> 4. Support for configurations such as early fire, late fire and allow > >> >> lateness > >> >> (which are internal experimental configurations in Group Window > >> >> Aggregation and not public to users yet.) > >> >> 5. Unification of the Window TVF Aggregation operator in runtime at > the > >> >> implementation layer > >> >> (In the long term, the cost to maintain the operators about Window > TVF > >> >> Aggregation and Group Window Aggregation is too expensive.) > >> >> > >> >> > >> >> This flip aims to continue the unfinished work in FLIP-145[2], which > is > >> to > >> >> fully enable the capabilities of Window TVF Aggregation > >> >> and officially deprecate the legacy syntax Group Window > Aggregation, to > >> >> prepare for the removal of the legacy one in Flink 2.0. > >> >> > >> >> > >> >> I have already done some preliminary POC to validate the feasibility > of > >> >> the related work in this flip as follows. > >> >> 1. POC for SESSION Window TVF Aggregation [3] > >> >> 2. POC for CUMULATE in Group Window Aggregation operator [4] > >> >> 3. POC for consuming CDC stream in Window Aggregation operator [5] > >> >> > >> >> > >> >> Looking forward to your feedback and thoughts! > >> >> > >> >> > >> >> > >> >> [1] > >> >> > >> > https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/sql/queries/window-agg/ > >> >> > >> >> [2] > >> >> > >> > https://cwiki.apache.org/confluence/display/FLINK/FLIP-145%3A+Support+SQL+windowing+table-valued+function#FLIP145:SupportSQLwindowingtablevaluedfunction-SessionWindows > >> >> [3] https://github.com/xuyangzhong/flink/tree/FLINK-24024 > >> >> [4] > >> >> > >> > https://github.com/xuyangzhong/flink/tree/poc_legacy_group_window_agg_cumulate > >> >> [5] > >> >> > >> > https://github.com/xuyangzhong/flink/tree/poc_window_agg_consumes_cdc_stream > >> >> > >> >> > >> >> > >> >> -- > >> >> > >> >> Best！ > >> >> Xuyang > >> >
