Thanks Jinsong for the suggestions. +1 for #1, as users are constantly puzzled by the inconsistencies between the Table / SQL module and SQL Client.
Best, Weike On Mon, Jul 6, 2020 at 11:55 AM Jingsong Li <jingsongl...@gmail.com> wrote: > +1 for expanding the scope of #2, a SQL test framework(Should not be too > heavy but clear) and guidelines for developers. > > Best, > Jingsong > > On Thu, Jul 2, 2020 at 12:30 PM Kurt Young <ykt...@gmail.com> wrote: > > > Thanks Jingsong for bringing this up discussion and sorry for the late > > reply. > > > > I'm in general +1 for #1, and want to expand the scope of #2. > > > > First of all, I think the approach Jingsong proposed in #2 can help with > > covering more > > e2e use cases of SQL, which also draws a clean line between how to design > > IT cases for > > table API and SQL. > > > > This is a good intent to have, but what I want to point out is it's still > > not sufficient yet. I think > > the ultimate problem we are facing is *designing a testing principle for > > table & SQL*. I will > > share some of my observations and issues I've seen: > > > > 1. The lack of SQL client's functionality is definitely the first victim > of > > this issue. During our > > development, I don't know how many developers will take SQL client into > > consideration when > > they introduce new features, especially which need to be exposed through > > SQL client. > > > > 2. We have a very fuzzy boundary of what kind of tests should be written > in > > table API or in SQL. > > So we end up with two testing packages called "sql" and "table" for the > > same testing purpose, with > > *random tests created in each package*. > > > > 3. We don't have a guideline for developers, especially for new > > contributors, about how many tests > > they should write and where to put. For example, when we improve the data > > type support, say we > > start to support higher precision of Timestamp, we don't have a clue how > > many tests we should provide. > > Should we add some tests for the code generation part? should we add some > > tests for the integration test? > > Or is it already enough to test it with simple expression tests? We don't > > have a clean answer for it. > > As a result, we will be busy fixing bugs around new features we > introduced, > > even if the bug itself looks > > very simple which *will surprise our users by the bad quality of Flink > SQL > > engine*. > > > > So I think what we really need here (which is also very urgent IMO) is > > *designing > > some testing principles* > > *for table API & SQL*. Writing SQL's IT case through sql client might be > > one of them. If we can keep > > improving the testing principles we have, I believe we will have a much > > more reliable SQL engine in the > > future, which can attract more users to the Flink community. > > > > Best, > > Kurt > > > > > > On Thu, Jun 18, 2020 at 5:14 PM Jark Wu <imj...@gmail.com> wrote: > > > > > +1 for #1 > > > I think this is what we are currently doing, that forward SQL > statements > > to > > > TableEnv#executeSql, e.g. FLINK-17113, FLINK-18059. > > > But IMO the SQL CLI specific statements (EXIT, QUIT) should still stay > > only > > > in SQL CLI. > > > > > > Another idea is that, the reviewer/committer should check tests are > both > > > added for SQL CLI and TableEnv if it is a new statement. > > > > > > Best, > > > Jark > > > > > > On Thu, 18 Jun 2020 at 15:40, Rui Li <lirui.fu...@gmail.com> wrote: > > > > > > > Thanks Jingsong for bringing up the discussion. Perhaps we can do > both > > #1 > > > > and #2? I like the idea that SQL Client should just forward SQL > > > statements > > > > to TableEnvironment. IIUC, with TableEnvironment::executeSql in > place, > > > most > > > > statements can be forwarded. Only a very limited set of statements > need > > > to > > > > be handled by SQL Client, like SET and EXIT. > > > > > > > > On Thu, Jun 18, 2020 at 3:19 PM Jingsong Li <jingsongl...@gmail.com> > > > > wrote: > > > > > > > > > Hi all, > > > > > > > > > > I'd like to start a discussion for the new features lacking support > > of > > > > > Sql-client. > > > > > > > > > > I've seen the new DDL syntax that SQL client lacks support for many > > > > times. > > > > > For every new DDL syntax, we need to add support in sql-client. Add > > > > > a corresponding SqlCommand in sql-client, otherwise this DDL is > still > > > > > not working in sql-client. > > > > > > > > > > But it looks like developers always forgot to add support in > > > sql-client. > > > > > Lots of DDL features just be added in parser and planner, but lack > > > > > sql-client support, so users will wait for the next release. Just > > like: > > > > > https://issues.apache.org/jira/browse/FLINK-7151 > > > > > https://issues.apache.org/jira/browse/FLINK-17198 > > > > > https://issues.apache.org/jira/browse/FLINK-15468 > > > > > https://issues.apache.org/jira/browse/FLINK-15175 > > > > > > > > > > How to solve this? > > > > > I think we have two options: > > > > > > > > > > 1. Unify the parser in sql-client and TableEnvironment truly. > Really > > > make > > > > > sql-client have all abilities from TableEnvironment. sql-client > just > > > > > forward sql to TableEnvironment. Can it be? > > > > > 2. A new testing framework mechanism: We can make sql-related tests > > > more > > > > > "up front", we can move e2e tests (I think we are doing now) and it > > > cases > > > > > to sql-client oriented. This may require a new testing framework > > > > mechanism, > > > > > for example, we can do something like hive sql testing [1] to > > > > > sql-client oriented. In this way, the testing can cover more > > horizontal > > > > and > > > > > vertical and it is easy to migrate tests from other systems too. > And > > I > > > > > think, Flink's DDLs are enough stronger to support pure SQLs > testing. > > > > > > > > > > What do you think? > > > > > > > > > > [1]https://github.com/apache/hive/tree/master/ql/src/test/queries > > > > > > > > > > Best, > > > > > Jingsong > > > > > > > > > > > > > > > > > -- > > > > Best regards! > > > > Rui Li > > > > > > > > > > > > -- > Best, Jingsong Lee >
