Hi, all Look like there’s only one divergence about option [ table | sql-client ].dml-sync in this thread, correct me if I’m wrong.
1. Leaving the context of this thread, from a user's perspective, the table.xx configurations should take effect in Table API & SQL, the sql-client.xx configurations should only take effect in sql-client. In my(the user's) opinion, other explanations are counterintuitive. 2. It should be pointed out that both all existed table.xx configurations like table.exec.state.ttl, table.optimizer.agg-phase-strategy, table.local-time-zone,etc.. and the proposed sql-client.xx configurations like sql-client.verbose, sql-client.execution.max-table-result.rows comply with this convention. 3. Considering the portability to support different CLI tools (sql-client, sql-gateway, etc.), I prefer table.dml-sync. In addition, I think sql-client/sql-gateway/other CLI tools can be placed out of flink-table module even in an external project, this should not affect our conclusion. Hope this can help you. Best, Leonard > 在 2021年2月25日，18:51，Shengkai Fang <fskm...@gmail.com> 写道： > > Hi, everyone. > > I do some summaries about the discussion about the option. If the summary > has errors, please correct me. > > `table.dml-sync`: > - take effect for `executeMultiSql` and sql client > - benefit: SQL script portability. One script for all platforms. > - drawback: Don't work for `TableEnvironment#executeSql`. > > `table.multi-dml-sync`: > - take effect for `executeMultiSql` and sql client > - benefit: SQL script portability > - drawback: It's confused when the sql script has one dml statement but > need to set option `table.multi-dml-sync` > > `client.dml-sync`: > - take effect for sql client only > - benefit: clear definition. > - drawback: Every platform needs to define its own option. Bad SQL script > portability. > > Just as Jark said, I think the `table.dml-sync` is a good choice if we can > extend its scope and make this option works for `executeSql`. > It's straightforward and users can use this option now in table api. The > drawback is the `TableResult#await` plays the same role as the option. I > don't think the drawback is really critical because many systems have > commands play the same role with the different names. > > Best, > Shengkai > > Timo Walther <twal...@apache.org> 于2021年2月25日周四 下午4:23写道： > >> The `table.` prefix is meant to be a general option in the table >> ecosystem. Not necessarily attached to Table API or SQL Client. That's >> why SQL Client is also located in the `flink-table` module. >> >> My main concern is the SQL script portability. Declaring the sync/async >> behavior will happen in many SQL scripts. And users should be easily >> switch from SQL Client to some commercial product without the need of >> changing the script again. >> >> Sure, we can change from `sql-client.dml-sync` to `table.dml-sync` later >> but that would mean introducing future confusion. An app name (what >> `sql-client` kind of is) should not be part of a config option key if >> other apps will need the same kind of option. >> >> Regards, >> Timo >> >> >> On 24.02.21 08:59, Jark Wu wrote: >>>> From my point of view, I also prefer "sql-client.dml-sync", >>> because the behavior of this configuration is very clear. >>> Even if we introduce a new config in the future, e.g. `table.dml-sync`, >>> we can also deprecate the sql-client one. >>> >>> Introducing a "table." configuration without any implementation >>> will confuse users a lot, as they expect it should take effect on >>> the Table API. >>> >>> If we want to introduce an unified "table.dml-sync" option, I prefer >>> it should be implemented on Table API and affect all the DMLs on >>> Table API (`tEnv.executeSql`, `Table.executeInsert`, `StatementSet`), >>> as I have mentioned before [1]. >>> >>>> It would be very straightforward that it affects all the DMLs on SQL CLI >>> and >>> TableEnvironment (including `executeSql`, `StatementSet`, >>> `Table#executeInsert`, etc.). >>> This can also make SQL CLI easy to support this configuration by passing >>> through to the TableEnv. >>> >>> Best, >>> Jark >>> >>> >>> [1]: >>> >> http://apache-flink-mailing-list-archive.1008284.n3.nabble.com/DISCUSS-FLIP-163-SQL-Client-Improvements-tp48354p48665.html >>> >>> >>> On Wed, 24 Feb 2021 at 10:39, Kurt Young <ykt...@gmail.com> wrote: >>> >>>> If we all agree the option should only be handled by sql client, then >> why >>>> don't we >>>> just call it `sql-client.dml-sync`? As you said, calling it >>>> `table.dml-sync` but has no >>>> affection in `TableEnv.executeSql("INSERT INTO")` will also cause a big >>>> confusion for >>>> users. >>>> >>>> The only concern I saw is if we introduce >>>> "TableEnvironment.executeMultiSql()" in the >>>> future, how do we control the synchronization between statements? TBH I >>>> don't really >>>> see a strong requirement for such interfaces. Right now, we have a >> pretty >>>> clear semantic >>>> of `TableEnv.executeSql`, and it's very convenient for users if they >> want >>>> to execute multiple >>>> sql statements. They can simulate either synced or async execution with >>>> this building block. >>>> >>>> This will introduce slight overhead for users, but compared to the >>>> confusion we might >>>> cause if we introduce such a method of our own, I think it's better to >> wait >>>> for some more >>>> feedback. >>>> >>>> Best, >>>> Kurt >>>> >>>> >>>> On Tue, Feb 23, 2021 at 9:45 PM Timo Walther <twal...@apache.org> >> wrote: >>>> >>>>> Hi Kurt, >>>>> >>>>> we can also shorten it to `table.dml-sync` if that would help. Then it >>>>> would confuse users that do a regular `.executeSql("INSERT INTO")` in a >>>>> notebook session. >>>>> >>>>> In any case users will need to learn the semantics of this option. >>>>> `table.multi-dml-sync` should be described as "If a you are in a multi >>>>> statement environment, execute DMLs synchrounous.". I don't have a >>>>> strong opinion on shortening it to `table.dml-sync`. >>>>> >>>>> Just to clarify the implementation: The option should be handled by the >>>>> SQL Client only, but the name can be shared accross platforms. >>>>> >>>>> Regards, >>>>> Timo >>>>> >>>>> >>>>> On 23.02.21 09:54, Kurt Young wrote: >>>>>> Sorry for the late reply, but I'm confused by `table.multi-dml-sync`. >>>>>> >>>>>> IIUC this config will take effect with 2 use cases: >>>>>> 1. SQL client, either interactive mode or executing multiple >> statements >>>>> via >>>>>> -f. In most cases, >>>>>> there will be only one INSERT INTO statement but we are controlling >> the >>>>>> sync/async behavior >>>>>> with "*multi-dml*-sync". I think this will confuse a lot of users. >>>>> Besides, >>>>>> >>>>>> 2. TableEnvironment#executeMultiSql(), but this is future work, we are >>>>> also >>>>>> not sure if we will >>>>>> really introduce this in the future. >>>>>> >>>>>> I would prefer to introduce this option for only sql client. For >>>>> platforms >>>>>> Timo mentioned which >>>>>> need to control such behavior, I think it's easy and flexible to >>>>> introduce >>>>>> one on their own. >>>>>> >>>>>> Best, >>>>>> Kurt >>>>>> >>>>>> >>>>>> On Sat, Feb 20, 2021 at 10:23 AM Shengkai Fang <fskm...@gmail.com> >>>>> wrote: >>>>>> >>>>>>> Hi everyone. >>>>>>> >>>>>>> Sorry for the late response. >>>>>>> >>>>>>> For `execution.runtime-mode`, I think it's much better than >>>>>>> `table.execution.mode`. Thanks for Timo's suggestions! >>>>>>> >>>>>>> For `SHOW CREATE TABLE`, I'm +1 with Jark's comments. We should >>>> clarify >>>>> the >>>>>>> usage of the SHOW CREATE TABLE statements. It should be allowed to >>>>> specify >>>>>>> the table that is fully qualified and only works for the table that >> is >>>>>>> created by the sql statements. >>>>>>> >>>>>>> I have updated the FLIP with suggestions. It seems we have reached a >>>>>>> consensus, I'd like to start a formal vote for the FLIP. >>>>>>> >>>>>>> Please vote +1 to approve the FLIP, or -1 with a comment. >>>>>>> >>>>>>> Best, >>>>>>> Shengkai >>>>>>> >>>>>>> Jark Wu <imj...@gmail.com> 于2021年2月15日周一 下午10:50写道： >>>>>>> >>>>>>>> Hi Ingo, >>>>>>>> >>>>>>>> 1) I think you are right, the table path should be fully-qualified. >>>>>>>> >>>>>>>> 2) I think this is also a good point. The SHOW CREATE TABLE >>>>>>>> only aims to print DDL for the tables registered using SQL CREATE >>>> TABLE >>>>>>>> DDL. >>>>>>>> If a table is registered using Table API, e.g. >>>>>>>> `StreamTableEnvironment#createTemporaryView(String, DataStream)`, >>>>>>>> currently it's not possible to print DDL for such tables. >>>>>>>> I think we should point it out in the FLIP. >>>>>>>> >>>>>>>> Best, >>>>>>>> Jark >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Mon, 15 Feb 2021 at 21:33, Ingo Bürk <i...@ververica.com> wrote: >>>>>>>> >>>>>>>>> Hi all, >>>>>>>>> >>>>>>>>> I have a couple questions about the SHOW CREATE TABLE statement. >>>>>>>>> >>>>>>>>> 1) Contrary to the example in the FLIP I think the returned DDL >>>> should >>>>>>>>> always have the table identifier fully-qualified. Otherwise the DDL >>>>>>>> depends >>>>>>>>> on the current context (catalog/database), which could be >>>> surprising, >>>>>>>>> especially since "the same" table can behave differently if created >>>> in >>>>>>>>> different catalogs. >>>>>>>>> 2) How should this handle tables which cannot be fully >> characterized >>>>> by >>>>>>>>> properties only? I don't know if there's an example for this yet, >>>> but >>>>>>>>> hypothetically this is not currently a requirement, right? This >>>> isn't >>>>>>> as >>>>>>>>> much of a problem if this syntax is SQL-client-specific, but if >> it's >>>>>>>>> general Flink SQL syntax we should consider this (one way or >>>> another). >>>>>>>>> >>>>>>>>> >>>>>>>>> Regards >>>>>>>>> Ingo >>>>>>>>> >>>>>>>>> On Fri, Feb 12, 2021 at 3:53 PM Timo Walther <twal...@apache.org> >>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Hi Shengkai, >>>>>>>>>> >>>>>>>>>> thanks for updating the FLIP. >>>>>>>>>> >>>>>>>>>> I have one last comment for the option `table.execution.mode`. >>>> Should >>>>>>>> we >>>>>>>>>> already use the global Flink option `execution.runtime-mode` >>>> instead? >>>>>>>>>> >>>>>>>>>> We are using Flink's options where possible (e.g. `pipeline.name` >>>>>>> and >>>>>>>>>> `parallism.default`) why not also for batch/streaming mode? >>>>>>>>>> >>>>>>>>>> The description of the option matches to the Blink planner >>>> behavior: >>>>>>>>>> >>>>>>>>>> ``` >>>>>>>>>> Among other things, this controls task scheduling, network shuffle >>>>>>>>>> behavior, and time semantics. >>>>>>>>>> ``` >>>>>>>>>> >>>>>>>>>> Regards, >>>>>>>>>> Timo >>>>>>>>>> >>>>>>>>>> On 10.02.21 06:30, Shengkai Fang wrote: >>>>>>>>>>> Hi, guys. >>>>>>>>>>> >>>>>>>>>>> I have updated the FLIP. It seems we have reached agreement. >>>> Maybe >>>>>>>> we >>>>>>>>>> can >>>>>>>>>>> start the vote soon. If anyone has other questions, please leave >>>>>>> your >>>>>>>>>>> comments. >>>>>>>>>>> >>>>>>>>>>> Best， >>>>>>>>>>> Shengkai >>>>>>>>>>> >>>>>>>>>>> Rui Li <lirui.fu...@gmail.com>于2021年2月9日 周二下午7:52写道： >>>>>>>>>>> >>>>>>>>>>>> Hi guys, >>>>>>>>>>>> >>>>>>>>>>>> The conclusion sounds good to me. >>>>>>>>>>>> >>>>>>>>>>>> On Tue, Feb 9, 2021 at 5:39 PM Shengkai Fang <fskm...@gmail.com >>> >>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi, Timo, Jark. >>>>>>>>>>>>> >>>>>>>>>>>>> I am fine with the new option name. >>>>>>>>>>>>> >>>>>>>>>>>>> Best, >>>>>>>>>>>>> Shengkai >>>>>>>>>>>>> >>>>>>>>>>>>> Timo Walther <twal...@apache.org>于2021年2月9日 周二下午5:35写道： >>>>>>>>>>>>> >>>>>>>>>>>>>> Yes, `TableEnvironment#executeMultiSql()` can be future work. >>>>>>>>>>>>>> >>>>>>>>>>>>>> @Rui, Shengkai: Are you also fine with this conclusion? >>>>>>>>>>>>>> >>>>>>>>>>>>>> Thanks, >>>>>>>>>>>>>> Timo >>>>>>>>>>>>>> >>>>>>>>>>>>>> On 09.02.21 10:14, Jark Wu wrote: >>>>>>>>>>>>>>> I'm fine with `table.multi-dml-sync`. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> My previous concern about "multi" is that DML in CLI looks >>>> like >>>>>>>>>>>> single >>>>>>>>>>>>>>> statement. >>>>>>>>>>>>>>> But we can treat CLI as a multi-line accepting statements >> from >>>>>>>>>>>> opening >>>>>>>>>>>>> to >>>>>>>>>>>>>>> closing. >>>>>>>>>>>>>>> Thus, I'm fine with `table.multi-dml-sync`. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> So the conclusion is `table.multi-dml-sync` (false by >>>> default), >>>>>>>> and >>>>>>>>>>>> we >>>>>>>>>>>>>> will >>>>>>>>>>>>>>> support this config >>>>>>>>>>>>>>> in SQL CLI first, will support it in >>>>>>>>>>>> TableEnvironment#executeMultiSql() >>>>>>>>>>>>>> in >>>>>>>>>>>>>>> the future, right? >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> Best, >>>>>>>>>>>>>>> Jark >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> On Tue, 9 Feb 2021 at 16:37, Timo Walther < >> twal...@apache.org >>>>> >>>>>>>>>>>> wrote: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Hi everyone, >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> I understand Rui's concerns. `table.dml-sync` should not >>>> apply >>>>>>>> to >>>>>>>>>>>>>>>> regular `executeSql`. Actually, this option makes only sense >>>>>>>> when >>>>>>>>>>>>>>>> executing multi statements. Once we have a >>>>>>>>>>>>>>>> `TableEnvironment.executeMultiSql()` this config could be >>>>>>>>>>>> considered. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Maybe we can find a better generic name? Other platforms >> will >>>>>>>> also >>>>>>>>>>>>> need >>>>>>>>>>>>>>>> to have this config option, which is why I would like to >>>>>>> avoid a >>>>>>>>> SQL >>>>>>>>>>>>>>>> Client specific option. Otherwise every platform has to come >>>>>>> up >>>>>>>>> with >>>>>>>>>>>>>>>> this important config option separately. >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Maybe `table.multi-dml-sync` `table.multi-stmt-sync`? Or >>>> other >>>>>>>>>>>>> opinions? >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> Regards, >>>>>>>>>>>>>>>> Timo >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> On 09.02.21 08:50, Shengkai Fang wrote: >>>>>>>>>>>>>>>>> Hi, all. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> I think it may cause user confused. The main problem is we >>>>>>>> have >>>>>>>>> no >>>>>>>>>>>>>> means >>>>>>>>>>>>>>>>> to detect the conflict configuration, e.g. users set the >>>>>>> option >>>>>>>>>>>> true >>>>>>>>>>>>>> and >>>>>>>>>>>>>>>>> use `TableResult#await` together. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> Best, >>>>>>>>>>>>>>>>> Shengkai. >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> -- >>>>>>>>>>>> Best regards! >>>>>>>>>>>> Rui Li >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>> >>>>> >>>> >>> >> >>
