Hi Timo, Thanks for the updating.
Regarding to "multiline statement support", I'm also fine that `TableEnvironment.executeSql()` only supports single line statement, and we can support multiline statement later (needs more discussion about this). Regarding to "StatementSet.explian()", I don't have strong opinions about that. Regarding to "TableResult.getJobClient()", I think it's unnecessary. The reason is: first, many statements (e.g. DDL, show xx, use xx) will not submit a Flink job. second, `TableEnvironment.executeSql()` and `StatementSet.execute()` are synchronous method, `TableResult` will be returned only after the job is finished or failed. Regarding to "whether StatementSet.execute() needs to throw exception", I think we should choose a unified way to tell whether the execution is successful. If `TableResult` contains ERROR kind (non-runtime exception), users need to not only check the result but also catch the runtime exception in their code. or `StatementSet.execute()` does not throw any exception (including runtime exception), all exception messages are in the result. I prefer "StatementSet.execute() needs to throw exception". cc @Jark Wu <imj...@gmail.com> I will update the agreed parts to the document first. Best, Godfrey Timo Walther <twal...@apache.org> 于2020年3月25日周三 下午6:51写道: > Hi Godfrey, > > thanks for starting the discussion on the mailing list. And sorry again > for the late reply to FLIP-84. I have updated the Google doc one more > time to incorporate the offline discussions. > > From Dawid's and my view, it is fine to postpone the multiline support > to a separate method. This can be future work even though we will need > it rather soon. > > If there are no objections, I suggest to update the FLIP-84 again and > have another voting process. > > Thanks, > Timo > > > On 25.03.20 11:17, godfrey he wrote: > > Hi community, > > Timo, Fabian and Dawid have some feedbacks about FLIP-84[1]. The > feedbacks > > are all about new introduced methods. We had a discussion yesterday, and > > most of feedbacks have been agreed upon. Here is the conclusions: > > > > *1. about proposed methods in `TableEnvironment`:* > > > > the original proposed methods: > > > > TableEnvironment.createDmlBatch(): DmlBatch > > TableEnvironment.executeStatement(String statement): ResultTable > > > > the new proposed methods: > > > > // we should not use abbreviations in the API, and the term "Batch" is > > easily confused with batch/streaming processing > > TableEnvironment.createStatementSet(): StatementSet > > > > // every method that takes SQL should have `Sql` in its name > > // supports multiline statement ??? > > TableEnvironment.executeSql(String statement): TableResult > > > > // new methods. supports explaining DQL and DML > > TableEnvironment.explainSql(String statement, ExplainDetail... details): > > String > > > > > > *2. about proposed related classes:* > > > > the original proposed classes: > > > > interface DmlBatch { > > void addInsert(String insert); > > void addInsert(String targetPath, Table table); > > ResultTable execute() throws Exception ; > > String explain(boolean extended); > > } > > > > public interface ResultTable { > > TableSchema getResultSchema(); > > Iterable<Row> getResultRows(); > > } > > > > the new proposed classes: > > > > interface StatementSet { > > // every method that takes SQL should have `Sql` in its name > > // return StatementSet instance for fluent programming > > addInsertSql(String statement): StatementSet > > > > // return StatementSet instance for fluent programming > > addInsert(String tablePath, Table table): StatementSet > > > > // new method. support overwrite mode > > addInsert(String tablePath, Table table, boolean overwrite): > > StatementSet > > > > explain(): String > > > > // new method. supports adding more details for the result > > explain(ExplainDetail... extraDetails): String > > > > // throw exception ??? > > execute(): TableResult > > } > > > > interface TableResult { > > getTableSchema(): TableSchema > > > > // avoid custom parsing of an "OK" row in programming > > getResultKind(): ResultKind > > > > // instead of `get` make it explicit that this is might be > triggering > > an expensive operation > > collect(): Iterable<Row> > > > > // for fluent programming > > print(): Unit > > } > > > > enum ResultKind { > > SUCCESS, // for DDL, DCL and statements with a simple "OK" > > SUCCESS_WITH_CONTENT, // rows with important content are available > > (DML, DQL) > > } > > > > > > *3. new proposed methods in `Table`* > > > > `Table.insertInto()` will be deprecated, and the following methods are > > introduced: > > > > Table.executeInsert(String tablePath): TableResult > > Table.executeInsert(String tablePath, boolean overwrite): TableResult > > Table.explain(ExplainDetail... details): String > > Table.execute(): TableResult > > > > There are two issues need further discussion, one is whether > > `TableEnvironment.executeSql(String statement): TableResult` needs to > > support multiline statement (or whether `TableEnvironment` needs to > support > > multiline statement), and another one is whether `StatementSet.execute()` > > needs to throw exception. > > > > please refer to the feedback document [2] for the details. > > > > Any suggestions are warmly welcomed! > > > > [1] > > > https://wiki.apache.org/confluence/pages/viewpage.action?pageId=134745878 > > [2] > > > https://docs.google.com/document/d/1ueLjQWRPdLTFB_TReAyhseAX-1N3j4WYWD0F02Uau0E/edit > > > > Best, > > Godfrey > > > >