More improvements to the way CLI handles comments in multi-line statements

[big-andy-coates]

Description

Following on from @vcrfxia insights in the last pr: #2214 (review), this PR adds more improvements to the way the CLI handles inline comments.

It can now handle something like this example being cut and paste into the CLI:

SELECT -- a comment
   -- another comment
   foo, -- cos we really like comments
   -- can't get enough of 'em
   bar -- love'em
   -- best thing since sliced bread
FROM -- better than fake news
x;

To support I've enhanced the CommentStripper to handle multi-line statements with embedded comments.

I've also had to switch the JLineReader to not replace new lines with spaces. So if the input contains multiple lines, so does the output. But the parser is fine with this. History also still works, though a single history item can now span multiple lines, e.g. running:

ksql>show
> -- awesome, another comment.
> streams;

Works, and then you can list history, which shows the command over multiple lines:

ksql> history
1: show
-- awesome, another comment.
streams;

Testing done

Added appropriate unit test and manual sanity testing.

Reviewer checklist

• Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
• Ensure relevant issues are linked (description should include text like "Fixes #")

[vcrfxia]

Thanks @big-andy-coates . Super comprehensive comment support! LGTM.

[vcrfxia]

The reason this isn't wrapped into the while-loop below is so we avoid creating a StringBuilder instance when the line contains no comments, yes? How expensive is it to create a StringBuilder instance? (Trying to improve my understanding of when such optimizations make sense.)

[big-andy-coates]

Most lines won't have comments, hence I added this early out as its easy to do.

One could argue this is 'premature optimisation' - especially considering this is, in general, reacting to someone typing at a keyboard and hence performance isn't really an issue. So asking me to remove this would be a valid point.

[blueedgenick]

Pretty slick-looking Andy! But i'm not sure i like the trade-off here of giving up good history presentation (i.e. the way it works today, which i think is likely the most 'expected' way for such a thing to work) to get even better inline commenting. Is there a way to resolve that tension by tweaking the impl here ? If not, i'd personally vote against making this particular change.

[big-andy-coates]

Hey @blueedgenick,

You're feedback welcome as always!

I had a quick look at the current CLI and it too reports multiple line history over multiple lines, (by bad, I'd not checked this wasn't already the case):

version 5.0.x gives:

ksql>show \
streams;
...

ksql>history
1080: ...
1081: show \
streams;
1082: history

With the change to drop line continuation char this now becomes:

ksql>show 
streams;
...

ksql>history
1080: ...
1081: show 
streams;
1082: history

So, there isn't really any change to history output here!

[blueedgenick]

@big-andy-coates fair point! ideally the history lines would get condensed into a single-line presentation (which for some reason i thought they already were, but that could just be a sign of the state of my memory). And of course once condensed into a single line it doesn't make sense to have the embedded comments any more... not saying we shouldn't preserve the comments, as entered, in the command topic - i'm only addressing the presentation of the local history file entries here. WDYT ?

[miguno]

ideally the history lines would get condensed into a single-line presentation (which for some reason i thought they already were, but that could just be a sign of the state of my memory)

Agreed, this is a cosmetic change. I wouldn't block this PR on it. Rather, we should file a separate GH ticket to consider improving the presentation of the (local) history of commands.

[rmoff]

LGTM!

[big-andy-coates]

Hey @blueedgenick,

Personally, I think a multi-line command should be displayed in the history output over multiple lines. However, I don't feel strongly about it. As @miguno suggests, how about raising an issue with details of how you think it should work?

You could mark the issue as good first issue / help wanted as it would be a nice contained issue for the community to work on.

[dguy]

Is it worth adding a test to CliTest ?
LGTM

[big-andy-coates]

@dguy

Is it worth adding a test to CliTest ?

I'm happy with the test coverage. Combination of JLineReaderTest and KsqlParserTest should be sufficient.