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
> >
>
>
