Re: [DISCUSS] FLIP-189: SQL Client Usability Improvements

Timo Walther Wed, 03 Nov 2021 05:37:52 -0700

Hi Sergey,

thanks for your explanation.

Regarding keywords and other info: We should receive the informationfrom the Flink SQL parser directly. We have added a couple of newkeywords such as WATERMARK or MATCH_RECOGNIZE clauses. SQL92 would nothelp a user understand why a column name needs to be escaped. And ingeneral, we should not have duplicate code. Let's discuss this when itcomes to the implementation. I'm sure we can propagate the Calciteparser config into a nice POJO that the CLI can receive from the Executor.


Regards,
Timo


On 03.11.21 11:12, Sergey Nuyanzin wrote:

Hi 李宇彬,

I think you are right. Thank you very much for the idea.
I came across MySQL[1] and PostgreSQL[2] prompts and also
found several interesting features like control symbols to change style,
showing current property value and different datetime formats.

I have added your proposals and my findings to FLIP's page, please have a
look.

[1] https://dev.mysql.com/doc/refman/8.0/en/mysql-commands.html
[2] https://www.postgresql.org/docs/14/app-psql.html#APP-PSQL-PROMPTING

On Wed, Nov 3, 2021 at 2:31 AM 李宇彬 <lixin58...@163.com> wrote:

Hi Sergey

It is a very useful improvement I'm looking forward to. in addition, I
think prompt
can play a greater role.

To help users call commands in expected context, we can get session
context
(current catalog/db/time) from cli prompt like MySQL,
please see details as below:

https://issues.apache.org/jira/browse/FLINK-24730

On 11/2/2021 21:09，Sergey Nuyanzin<snuyan...@gmail.com> wrote：
Hi Timo

Thank you for your questions.

I will answer your questions here and update FLIP's page as well

For example, who is responsible for parsing comments? I guess the SQL
Client and not the Flink SQL parser will take care of this?
Yes, you are right. SQL Client is responsible for parsing here.
However it does not validate sql, it only validates that comments, brackets
and quotes are closed and the statement ends with a semicolon.
Also under the hood jline's splits input into words and works with them.
Within a custom parser it is possible to specify what should be considered
as a word or not considered at all (e.g. it is possible to remove
all line and block comments before submitting a query,
probably as another non default option)... During parsing it marks
what is a comment, a keyword, a quoted string etc. based on rules
defined in SQL Client parser.
SQL Client highlighter could use the result of this marking to highlight.
Completer could use it to do completion, e.g. if based on parser's marks
completer knows that a cursor is inside a comment or a string
then no need to complete anything.

Also, will the prompt hints for `'>` and ``>` support escaping? This can
be a tricky topic sometimes.
Ideally yes, I played with lots of tricky cases and it behaves ok.
At least I do not see limitations here.
In case you do please share...

In general, how do we deal with different SQL dialects in the SQL
Client. For example, it possible to `table.sql-dialect` to `HIVE`. Will
all highlighting, auto-complete and prompt hints be disabled in this case?
It could be turned off for the beginning.
To make it supported across different dialects it is required to have such
info:
1) Set of keywords
2) Quote sign
3) SQL identifier quote
4) Start of a line comment
5) Start and end of a block comment
6) Start and end of hints
I see at least 2 ways:
1. provide such api
2. create this mapping in SQL Client and use it based on current dialect
Then it will be easy to support it for a new dialect
Here the only questionable thing is keywords.
Currently I made it pretty straightforward:
if a word not inside quoted string, not inside a comment or a hint
and matches anything from
SQL92 (

*org.apache.calcite.sql.parser.SqlAbstractParserImpl#getSql92ReservedWords*)),

then it will be highlighted as a keyword.

On Tue, Nov 2, 2021 at 12:09 PM Timo Walther <twal...@apache.org> wrote:

Hi Sergey,

thanks for this nice demo video. It looks very nice and makes the SQL
Client an even more useful tool.

What I miss a bit in the FLIP is the implementation details.

For example, who is responsible for parsing comments? I guess the SQL
Client and not the Flink SQL parser will take care of this?

Also, will the prompt hints for `'>` and ``>` support escaping? This can
be a tricky topic sometimes.

Looking forward to have this in Flink.

Thanks,
Timo

On 02.11.21 08:26, Till Rohrmann wrote:
Hi Sergey,

I think that after answering/resolving Jark's comments, you can start a
[VOTE] thread for this FLIP. The process is described here [1]. Once the
FLIP has been accepted, which it should, given the positive feedback, you
can start working on it by creating the corresponding JIRA tickets and
then
start coding. I hope that there will be a committer familiar with the SQL
client that can help you with the code review and merging of the code.
But
again, given the positive feedback, I think there will be a volunteer.

[1]

https://cwiki.apache.org/confluence/display/FLINK/Flink+Improvement+Proposals

Cheers,
Till

On Tue, Nov 2, 2021 at 4:25 AM Jark Wu <imj...@gmail.com> wrote:

Awesome demo, looking forward to these features!

I only have a minor comment: could we provide a config to enable/disable
the prompt values?
We can also discuss whether we can enable all the new features by
default
to give them more exposure.

Best,
Jark

On Tue, 2 Nov 2021 at 10:48, JING ZHANG <beyond1...@gmail.com> wrote:

Amazing improvements and impressive video.
Big +1.

Best,
JING ZHANG

Kurt Young <ykt...@gmail.com> 于2021年11月2日周二 上午9:37写道：

Really cool improvements @Sergey. Can't wait to see it happen.

Best,
Kurt

On Tue, Nov 2, 2021 at 1:56 AM Martijn Visser <mart...@ververica.com>
wrote:

Hi Sergey,

I guess you've just set a new standard ;-) I agree with Ingo, these
improvements look really good!

Best regards,

Martijn

On Mon, 1 Nov 2021 at 18:23, Ingo Bürk <i...@ververica.com> wrote:

Hi Sergey,

I think those improvements look absolutely amazing. Thanks for the
little
video!

Best
Ingo

On Mon, Nov 1, 2021, 17:15 Sergey Nuyanzin <snuyan...@gmail.com>
wrote:

Thanks for the feedback Till.

Martijn, I have created a short demo showing some of the features
mentioned
in FLIP.
It is available at https://asciinema.org/a/446247?speed=3.0
Could you please tell if it is what you are expecting or not?

On Fri, Oct 29, 2021 at 4:59 PM Till Rohrmann <
trohrm...@apache.org>
wrote:

Thanks for creating this FLIP Sergey. I think what you propose
sounds
like
very good improvements for the SQL client. This should make the
client
a
lot more ergonomic :-)

Cheers,
Till

On Fri, Oct 29, 2021 at 11:26 AM Sergey Nuyanzin <
snuyan...@gmail.com>
wrote:

Hi Martijn,

Thank you for your suggestion with POC.
Yes I will do that and come back to this thread probably
after
the
weekend

On Thu, Oct 28, 2021 at 4:38 PM Martijn Visser <
mart...@ververica.com>
wrote:

Hi Sergey,

Thanks for taking the initiative to create a FLIP and
propose
improvements
on the SQL client. All usability improvements on the SQL
client
are
highly
appreciated, especially for new users of Flink. Multi-line
support
is
definitely one of those things I've run into myself.

I do think it would be quite nice if there would be some
kind
of
POC
which
could show (some of) the proposed improvements. Is that
something
that
might be easily feasible?

Best regards,

Martijn

On Thu, 28 Oct 2021 at 11:02, Sergey Nuyanzin <
snuyan...@gmail.com

wrote:

Hi all,

I want to start a discussion about FLIP-189: SQL Client
Usability
Improvements.

The main changes in this FLIP:

- Flink sql client parser improvements so
that sql client does not ask for ; inside a quoted
string
or a
comment
- use prompt to show what sql client is waiting for
- introduce syntax highlighting
- improve completion

For more detailed changes, please refer to FLIP-189[1].

[1]

https://cwiki.apache.org/confluence/display/FLINK/FLIP-189%3A+SQL+Client+Usability+Improvements

Look forward to your feedback.

--
Best regards,
Sergey

Re: [DISCUSS] FLIP-189: SQL Client Usability Improvements

Reply via email to