Hi all, Thanks for all discussions!
Anyone else have questions or suggestions? if not, I will start a vote thread later. Best Kui Yuan kui yuan <catye...@gmail.com> 于2023年2月27日周一 20:21写道： > Hi Timo, > > Thanks for your advice. I totally agree with your suggestion of naming > convention, I will rename these options and update the flip later, thanks > very much. > > In our internal implementation we had put these options inside the > `FactoryUtil`, just as you expect. We have also taken into account the > changes to the CompiledPlan and we have packaged these options > appropriately to minimize intrusiveness and ensure the compatibility to the > `WatermarkPushDownSpec`. > > > A hint to the implementation: I would suggest that we add those options > > to `FactoryUtil`. All cross-connector options should end up there. > > > > Please also consider the changes to the CompiledPlan in your FLIP. This > > change has implications on the JSON format as watermark strategy of > > ExecNode becomes more complex, see WatermarkPushDownSpec > > Best > Kui Yuan > > Timo Walther <twal...@apache.org> 于2023年2月27日周一 18:05写道： > >> Hi Kui Yuan, >> >> thanks for working on this FLIP. Let me also give some comments about >> the proposed changes. >> >> I support the direction of this FLIP about handling these >> watermark-specific properties through options and /*+OPTIONS(...) */ >> hints. >> >> Regarding naming, I would like to keep the options in sync with existing >> options: >> >> > 'watermark.emit.strategy'='ON_EVENT' >> >> Let's use lower case (e.g. `on-event`) that matches with properties like >> sink.partitioner [1] or sink.delivery-guarantee [2]. >> >> > 'source.idle-timeout'='1min' >> >> According to FLIP-122 [3], we want to prefix all scan-source related >> properties with `scan.*`. This clearly includes idle-timeout and >> actually also watermark strategies which don't apply for lookup sources. >> >> Summarizing the comments above, we should use the following options: >> >> 'scan.watermark.emit.strategy'='on-event', >> 'scan.watermark.emit.on-event.gap'='10000', >> 'scan.idle-timeout'='1min', >> 'scan.watermark.alignment.group'='alignment-group-1', >> 'scan.watermark.alignment.max-drift'='1min', >> 'scan.watermark.alignment.update-interval'='1s' >> >> I know that this makes the keys even longer, but given that those >> options are for power users this should be acceptable. It also clearly >> indicates which options are for sinks, scans, and lookups. This >> potentially also helps in allow lists. >> >> A hint to the implementation: I would suggest that we add those options >> to `FactoryUtil`. All cross-connector options should end up there. >> >> Please also consider the changes to the CompiledPlan in your FLIP. This >> change has implications on the JSON format as watermark strategy of >> ExecNode becomes more complex, see WatermarkPushDownSpec [4]. >> >> Regards, >> Timo >> >> >> [1] >> >> https://nightlies.apache.org/flink/flink-docs-release-1.11/dev/table/connectors/kafka.html#sink-partitioner >> [2] >> >> https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/connectors/table/kafka/#sink-delivery-guarantee >> [3] >> >> https://cwiki.apache.org/confluence/display/FLINK/FLIP-122%3A+New+Connector+Property+Keys+for+New+Factory >> [4] >> >> https://github.com/apache/flink/blob/master/flink-table/flink-table-planner/src/main/java/org/apache/flink/table/planner/plan/abilities/source/WatermarkPushDownSpec.java >> >> >> On 24.02.23 04:55, kui yuan wrote: >> > Hi all, >> > >> > I have updated the flip according to the discussion, and we will extend >> the >> > watermark-related features with both table options and 'OPTIONS' hint, >> like >> > this: >> > >> > ``` >> > -- configure in table options >> > CREATE TABLE user_actions ( >> > ... >> > user_action_time TIMESTAMP(3), >> > WATERMARK FOR user_action_time AS user_action_time - INTERVAL '5' >> SECOND >> > ) WITH ( >> > 'watermark.emit.strategy'='ON_PERIODIC', >> > ... >> > ); >> > >> > -- use 'OPTIONS' hint >> > select ... from source_table /*+ OPTIONS('watermark.emit.strategy'= >> > 'ON_PERIODIC') */ >> > ``` >> > >> > Does everybody have any other questions? >> > >> > Best >> > Kui Yuan >> > >> > kui yuan <catye...@gmail.com> 于2023年2月23日周四 20:05写道： >> > >> >> Hi all, >> >> >> >> Thanks for all suggestions. >> >> >> >> We will extend the watermark-related features in SQL layer with dynamic >> >> table options and 'OPTIONS' hint, just as everyone expects. I will >> modify >> >> Flip-296 as discussed. >> >> >> >> @Martijn As far as I know, there is no hint interface in the table API, >> >> so we can't use hint in table API directly. if we need to extend the >> hint >> >> interface in the table API, maybe we need another flip. However, if we >> >> extend the watermark-related features in the dynamic table options, >> maybe >> >> we are able to use them indirectly in the table API like this[1]: >> >> >> >> ``` >> >> // register a table named "Orders" >> >> tableEnv.executeSql("CREATE TABLE Orders (`user` BIGINT, product >> STRING, >> >> amount INT) WITH ('watermark.emit.strategy'='ON_EVENT'...)"); >> >> ``` >> >> >> >> [1] >> >> >> https://nightlies.apache.org/flink/flink-docs-master/docs/dev/table/sql/create/ >> >> >> >> Best >> >> Kui Yuan >> >> >> >> Yun Tang <myas...@live.com> 于2023年2月23日周四 17:46写道： >> >> >> >>> Thanks for the warm discussions! >> >>> >> >>> I had an offline discussion with Kui about the replies. I think I >> could >> >>> give some explanations on the original intention to introduce another >> >>> WATERMARK_PARAMS. If we take a look at the current datastream API, the >> >>> watermark strategy does not belong to any specific connector. And we >> >>> thought the dynamic table options were more like the configurations >> within >> >>> some specific connector. >> >>> >> >>> From the review comments, I think most people feel good to make it >> part >> >>> of the dynamic table options. I think this is fine if we give more >> clear >> >>> scope definition of the dynamic table options here. And I also agree >> with >> >>> Jingsong's concern about adding SQL syntax which is the most >> concerning >> >>> part before launching this discussion. >> >>> >> >>> For Martijn's concern, if we accept to make the watermark-related >> options >> >>> part of dynamic table options, the problem becomes another topic: how >> to >> >>> support the dynamic table options in table API, which is deserved to >> create >> >>> another FLIP. >> >>> >> >>> Best >> >>> Yun Tang >> >>> ________________________________ >> >>> From: Martijn Visser <martijnvis...@apache.org> >> >>> Sent: Thursday, February 23, 2023 17:14 >> >>> To: dev@flink.apache.org <dev@flink.apache.org> >> >>> Subject: Re: [DISCUSS] FLIP-296: Watermark options for table API & SQL >> >>> >> >>> Hi, >> >>> >> >>> While I can understand that there's a desire to first focus on solving >> >>> this >> >>> problem for SQL, I do wonder if we should ignore the Table API at this >> >>> point. If we could include the syntax for the Table API, it >> potentially >> >>> could also be implemented by another contributor without needing to >> create >> >>> another FLIP. If we don't design it right now, my concern is that this >> >>> will >> >>> increase sparsity for the Table API which ultimately hurts adoption. >> >>> >> >>> With regards to the syntax, I have a preference to solve this via the >> >>> connector options (e.g. like you can currently specify things as >> >>> scan.startup.specific-offsets or scan.bounded.mode for the Kafka >> >>> connector). You could still use the dynamic table options to >> override/add >> >>> them. >> >>> >> >>> Best regards, >> >>> >> >>> Martijn >> >>> >> >>> On Thu, Feb 23, 2023 at 7:21 AM Shammon FY <zjur...@gmail.com> wrote: >> >>> >> >>>> Hi kui >> >>>> >> >>>> Thanks for your answer and +1 to yuxia too >> >>>> >> >>>>> we should not bind the watermark-related options to a connector to >> >>> ensure >> >>>> semantic clarity. >> >>>> >> >>>> In my opinion, adding watermark-related options to a connector is >> much >> >>> more >> >>>> clear. Currently users can define simple watermark strategy in DDL, >> >>> adding >> >>>> more configuration items in connector options is easy to understand >> >>>> >> >>>> Best, >> >>>> Shammon >> >>>> >> >>>> >> >>>> On Thu, Feb 23, 2023 at 10:52 AM Jingsong Li <jingsongl...@gmail.com >> > >> >>>> wrote: >> >>>> >> >>>>> Thanks for your proposal. >> >>>>> >> >>>>> +1 to yuxia, consider watermark-related hints as option hints. >> >>>>> >> >>>>> Personally, I am cautious about adding SQL syntax, WATERMARK_PARAMS >> is >> >>>>> also SQL syntax to some extent. >> >>>>> >> >>>>> We can use OPTIONS to meet this requirement if possible. >> >>>>> >> >>>>> Best, >> >>>>> Jingsong >> >>>>> >> >>>>> On Thu, Feb 23, 2023 at 10:41 AM yuxia <luoyu...@alumni.sjtu.edu.cn >> > >> >>>>> wrote: >> >>>>>> >> >>>>>> Hi, Yuan Kui. >> >>>>>> Thanks for driving it. >> >>>>>> IMO, the 'OPTIONS' hint may be not only specific to the connector >> >>>>> options. Just as a reference, we also have `sink.parallelism`[1] as >> a >> >>>>> connector options. It enables >> >>>>>> user to specific the writer's parallelism dynamically per-query. >> >>>>>> >> >>>>>> Personally, I perfer to consider watermark-related hints as option >> >>>>> hints. So, user can define a default watermark strategy for the >> table, >> >>>> and >> >>>>> if user dosen't needed to changes it, they need to do nothing in >> their >> >>>>> query instead of specific it ervery time. >> >>>>>> >> >>>>>> [1] >> >>>>> >> >>>> >> >>> >> https://nightlies.apache.org/flink/flink-docs-master/zh/docs/connectors/table/filesystem/#sink-parallelism >> >>>>>> >> >>>>>> Best regards, >> >>>>>> Yuxia >> >>>>>> >> >>>>>> Best regards, >> >>>>>> Yuxia >> >>>>>> >> >>>>>> ----- 原始邮件 ----- >> >>>>>> 发件人: "kui yuan" <catye...@gmail.com> >> >>>>>> 收件人: "dev" <dev@flink.apache.org> >> >>>>>> 抄送: "Jark Wu" <imj...@gmail.com> >> >>>>>> 发送时间: 星期三, 2023年 2 月 22日 下午 10:08:11 >> >>>>>> 主题: Re: [DISCUSS] FLIP-296: Watermark options for table API & SQL >> >>>>>> >> >>>>>> Hi all, >> >>>>>> >> >>>>>> Thanks for the lively discussion and I will respond to these >> >>> questions >> >>>>> one >> >>>>>> by one. However, there are also some common questions and I will >> >>> answer >> >>>>>> together. >> >>>>>> >> >>>>>> @郑 Thanks for your reply. The features mentioned in this flip are >> >>> only >> >>>>> for >> >>>>>> those source connectors that implement the >> SupportsWatermarkPushDown >> >>>>>> interface, generating watermarks in other graph locations is not in >> >>> the >> >>>>>> scope of this discussion. Perhaps another flip can be proposed >> >>> later to >> >>>>>> implement this feature. >> >>>>>> >> >>>>>> @Shammon Thanks for your reply. In Flip-296, a rejected alternative >> >>> is >> >>>>>> adding watermark related options in the connector options，we >> believe >> >>>> that >> >>>>>> we should not bind the watermark-related options to a connector to >> >>>> ensure >> >>>>>> semantic clarity. >> >>>>>> >> >>>>>>> What will happen if we add watermark related options in `the >> >>>> connector >> >>>>>>> options`? Will the connector ignore these options or throw an >> >>>>> exception? >> >>>>>>> How can we support this? >> >>>>>> >> >>>>>> If user defines different watermark configurations for one table in >> >>> two >> >>>>>> places, I tend to prefer the first place would prevail, but we can >> >>>> also >> >>>>>> throw exception or just print logs to prompt the user, which are >> >>>>>> implementation details. >> >>>>>> >> >>>>>>> If one table is used by two operators with different watermark >> >>>> params, >> >>>>>>> what will happen? >> >>>>>> >> >>>>>> @Martijn Thanks for your reply. I'm sorry that we are not >> >>> particularly >> >>>>>> accurate, this hint is mainly for SQL, not table API. >> >>>>>> >> >>>>>>> While the FLIP talks about watermark options for Table API & SQL, >> >>> I >> >>>>> only >> >>>>>>> see proposed syntax for SQL, not for the Table API. What is your >> >>>>> proposal >> >>>>>>> for the Table API >> >>>>>> >> >>>>>> @Jane Thanks for your reply. For the first question, If the user >> >>> uses >> >>>>> this >> >>>>>> hint on those sourse that does not implement the >> >>>>> SupportsWatermarkPushDown >> >>>>>> interface, it will be completely invalid. The task will run as >> >>> normal >> >>>> as >> >>>>> if >> >>>>>> the hint had not been used. >> >>>>>> >> >>>>>>> What's the behavior if there are multiple table sources, among >> >>> which >> >>>>>>> some do not support `SupportsWatermarkPushDown`? >> >>>>>> >> >>>>>> @Jane feedback that 'WATERMARK_PARAMS' is difficult to remember, >> >>>> perhaps >> >>>>>> the naming issue can be put to the end of the discussion, because >> >>> more >> >>>>>> people like @Martijn @Shuo are considering whether these >> >>> configurations >> >>>>>> should be put into the DDL or the 'OPTIONS' hint. Here's what I >> >>>>>> think, Putting these configs into DDL or putting them into >> 'OPTIONS' >> >>>> hint >> >>>>>> is actually the same thing, because the 'OPTIONS' hint is mainly >> >>> used >> >>>> to >> >>>>>> configure the properties of conenctor. The reason why I want to use >> >>> a >> >>>> new >> >>>>>> hint is to make sure the semantics clear, in my opinion the >> >>>> configuration >> >>>>>> of watermark should not be mixed up with connector. However, a new >> >>> hint >> >>>>>> does make it more difficult to use to some extent, for example, >> >>> when a >> >>>>> user >> >>>>>> uses both 'OPTIONS' hint and 'WATERMARK_PARAMS' hint. For this >> >>> point, >> >>>>> maby >> >>>>>> it is more appropriate to use uniform 'OPTIONS' hint. >> >>>>>> On the other hand, if we enrich more watermark option keys in >> >>> 'OPTIONS' >> >>>>>> hints, The question will be what we treat the definatrions of >> >>>> 'OPTIONS' >> >>>>>> hint, is this only specific to the connector options or could be >> >>> more? >> >>>>>> Maybe @Jark could share more insights here. In my opion, 'OPTIONS' >> >>> is >> >>>>> only >> >>>>>> related to the connector options, which is not like the gernal >> >>>> watermark >> >>>>>> options. >> >>>>>> >> >>>>>> >> >>>>>> >> >>>>>> Shuo Cheng <njucs...@gmail.com> 于2023年2月22日周三 19:17写道： >> >>>>>> >> >>>>>>> Hi Kui, >> >>>>>>> >> >>>>>>> Thanks for driving the discussion. It's quite useful to introduce >> >>>>> Watermark >> >>>>>>> options. I have some questions: >> >>>>>>> >> >>>>>>> What kind of hints is "WATERMARK_PARAMS"? >> >>>>>>> Currently, we have two kinds of hints in Flink: Dynamic Table >> >>>> Options & >> >>>>>>> Query Hints. As described in the Flip, "WATERMARK_PARAMS" is more >> >>>> like >> >>>>>>> Dynamic Table Options. So two questions arise here: >> >>>>>>> >> >>>>>>> 1) Are these watermark options to be exposed as connector WITH >> >>>>> options? Aa >> >>>>>>> described in SQL Hints doc[1], "Dynamic Table Options allow to >> >>>>> specify or >> >>>>>>> override table options dynamically", which implies that these >> >>> options >> >>>>> can >> >>>>>>> also be configured in WITH options. >> >>>>>>> >> >>>>>>> 2) Do we really need a new hint name like 'WATERMARK_PARAMS', >> >>> table >> >>>>>>> options use "OPTIONS" as hint name, like '/*+ >> >>>>>>> OPTIONS('csv.ignore-parse-errors'='true') */', maybe we can enrich >> >>>> more >> >>>>>>> table option keys for watermark, e.g., /*+ >> >>>>>>> OPTIONS('watermark.emit-strategy'='ON_PERIODIC') */. >> >>>>>>> >> >>>>>>> >> >>>>>>> [1] >> >>>>>>> >> >>>>>>> >> >>>>> >> >>>> >> >>> >> https://nightlies.apache.org/flink/flink-docs-release-1.16/docs/dev/table/sql/queries/hints/ >> >>>>>>> >> >>>>>>> On Wed, Feb 22, 2023 at 10:22 AM kui yuan <catye...@gmail.com> >> >>>> wrote: >> >>>>>>> >> >>>>>>>> Hi devs, >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> I'd like to start a discussion thread for FLIP-296[1]. This >> >>> comes >> >>>>> from an >> >>>>>>>> offline discussion with @Yun Tang, and we hope to enrich table >> >>> API >> >>>> & >> >>>>> SQL >> >>>>>>> to >> >>>>>>>> support many watermark-related features which were only >> >>> implemented >> >>>>> at >> >>>>>>> the >> >>>>>>>> datastream API level. >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> Basically, we want to introduce watermark options in table API & >> >>>> SQL >> >>>>> via >> >>>>>>>> SQL hint named 'WATERMARK_PARAMS' to support features: >> >>>>>>>> >> >>>>>>>> 1、Configurable watermark emit strategy >> >>>>>>>> >> >>>>>>>> 2、Dealing with idle sources >> >>>>>>>> >> >>>>>>>> 3、Watermark alignment >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> Last but not least, thanks to Qingsheng and Jing Zhang for the >> >>>>> initial >> >>>>>>>> reviews. >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> Looking forward to your thoughts and any feedback is >> >>> appreciated! >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> [1] >> >>>>>>>> >> >>>>>>> >> >>>>> >> >>>> >> >>> >> https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=240884405 >> >>>>>>>> >> >>>>>>>> >> >>>>>>>> Best >> >>>>>>>> >> >>>>>>>> Yuan Kui >> >>>>>>>> >> >>>>>>> >> >>>>> >> >>>> >> >>> >> >> >> > >> >>
