feat(cli): add the feature to turn of WRAP for CLI output #3341

agavra · 2019-09-12T19:55:58Z

Description

custom CLI settings are set using Oracle-like SET CLI PROPERTY VALUE syntax (which is different from SET property=value KSQL syntax)... perhaps this is confusing so I'm open to suggestions
SET CLI WRAP ON/OFF toggles line wrapping (see example below)

ksql> SELECT * FROM FOOZ EMIT CHANGES;
+------------------------------------+------------------------------------+------------------------------------+------------------------------------+------------------------------------+
|ROWTIME                             |ROWKEY                              |FOO                                 |BAR                                 |BAZ                                 |
+------------------------------------+------------------------------------+------------------------------------+------------------------------------+------------------------------------+
|1568310477336                       |null                                |hello                               |world                               |no clip                             |
|1568311062781                       |null                                |hello                               |world                               |clip all the things because this is |
|                                    |                                    |                                    |                                    |such a long string!                 |
|1568311085195                       |null                                |what about also clipping something t|world                               |clip all the things because this is |
|                                    |                                    |hat is in the beginning?            |                                    |such a long string!                 |
ksql> SET CLI WRAP OFF;
ksql> SELECT * FROM FOOZ EMIT CHANGES;
+------------------------------------+------------------------------------+------------------------------------+------------------------------------+------------------------------------+
|ROWTIME                             |ROWKEY                              |FOO                                 |BAR                                 |BAZ                                 |
+------------------------------------+------------------------------------+------------------------------------+------------------------------------+------------------------------------+
|1568310477336                       |null                                |hello                               |world                               |no clip                             |
|1568311062781                       |null                                |hello                               |world                               |clip all the things because this ...|
|1568311085195                       |null                                |what about also clipping somethin...|world                               |clip all the things because this ...|
^CQuery terminated

The help message:

set cli <property> <value>:
	Sets a CLI local property. NOTE that this differs from setting a KSQL property with 'SET property=value' in that it does not affect the server.

TODO

@JimGalasyn @rmoff - how do you suggest we show all available CLI settings? I think we should be able to link to some documentation that is auto-generated by CliConfig, but I'm not sure how to get that working

Testing done

Some unit tests
E2E CLI testing (see output above)

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 · 2019-09-16T21:59:10Z

custom CLI settings are set using Oracle-like SET PROPERTY VALUE syntax (which is different from SET property=value KSQL syntax)... perhaps this is confusing so I'm open to suggestions

My gut says this is confusing. Maybe we can have another command such as cli-config to display available CLI properties and their current values, and then use cli-config set wrap <on/off> in order to set configs, to make explicit that we're setting a CLI property rather than a KSQL property? I don't love this suggestion either, but I hope we can agree on something since we do have quite a few CLI properties (output, request-pipelining, and maybe even server) so I like the suggestion of centralizing these properties and normalizing how they're set.

rmoff · 2019-09-17T11:21:19Z

Yeah, I think this is going to be confusing:

SET 'auto.offset.reset' = 'earliest';
SET WRAP OFF;

Do we need to introduce a SERVER keyword?

SET SERVER 'auto.offset.reset' = 'earliest';
SET WRAP OFF;

Or inverted and probably easier to add on top of what we've got:

SET 'auto.offset.reset' = 'earliest';
SET CLI WRAP OFF;

agavra · 2019-09-17T15:57:18Z

thanks for the suggestions @rmoff @vcrfxia! I'm happy with SET CLI WRAP OFF, I think it's intuitive and backwards compatible.

big-andy-coates · 2019-09-18T14:54:43Z

Thinking into the future... we are likely at some point to allow people to set their own date and time formats. This is pretty common functionality.

SET DATEFORMAT yyyy-MM-dd

What's interesting here is that this would likely be both a client setting, as we'd use it in the CLI to format dates, and also potentially a server setting, i.e. it's passed to the server to allow it to parse supplied date.

So... do we really want to differentiate?

Also, why should the user care if the property is affecting the client or the server? Take WRAP - should the user care if its the server doing the wrapping before sending the output to the client, of the client doing it locally?

Just throwing that out there for thought! ;)

big-andy-coates

Thanks @agavra

LGTM, 'cept I'm left wondering what was gained by moving the maybeHandleCliSpecificCommands call into the CLI aside from more code and having to take the line the console returned and immediately ask that same console if this is actually an internal command it should handle... I don't get it.

Plus see comment above about syntax.

ksql-cli/src/main/java/io/confluent/ksql/cli/Cli.java

ksql-cli/src/main/java/io/confluent/ksql/cli/console/CliConfig.java

ksql-cli/src/main/java/io/confluent/ksql/cli/console/cmd/SetCliProperty.java

agavra · 2019-09-18T17:26:44Z

Thanks for the review @big-andy-coates!

LGTM, 'cept I'm left wondering what was gained by moving the maybeHandleCliSpecificCommands call into the CLI aside from more code and having to take the line the console returned and immediately ask that same console if this is actually an internal command it should handle... I don't get it.

Every single time I've looked at this code, I have to dig through and find where we handle the CLI specific commands. I think it makes a lot of sense to have the code be:

read line
handle line

instead of

read and maybe handle some lines
handle the other lines

IMO, whether a command is CLI-specific or not should be part of "handling" not "reading".

agavra · 2019-09-18T17:29:55Z

@big-andy-coates - with regards to syntax, I suppose it doesn't matter except that otherwise I need to dig into the contents of the command to figure out whether it's a CLI property or not. That seems... undesirable.

big-andy-coates · 2019-09-19T16:29:10Z

Thanks for the review @big-andy-coates!

LGTM, 'cept I'm left wondering what was gained by moving the maybeHandleCliSpecificCommands call into the CLI aside from more code and having to take the line the console returned and immediately ask that same console if this is actually an internal command it should handle... I don't get it.

Every single time I've looked at this code, I have to dig through and find where we handle the CLI specific commands. I think it makes a lot of sense to have the code be:

read line

handle line

instead of

read and maybe handle some lines

handle the other lines

IMO, whether a command is CLI-specific or not should be part of "handling" not "reading".

IMHO the change makes the code worse, not better. As I see it you've changed:

Cli -> get next line to execute from Console
Console -> read input, handle any internal commands, return any non-internal
Cli -> handle not internal line

To

Cli -> get next line to execute from Console
Console -> read line and return it
Cli -> call into Console to see it its an internal command and if it is have the console handle it.
-> otherwise execute it

IMHO this is breaking encapsulation. Previously only the Console knew about the internal commands. It dealt with them. Now the Cli needs to know about them too, but only in so much as to ask the console to handle any of the lines the console just returned.

To put it another way - previously the Console returned any lines it couldn't handle to the Cli. Now the Console returns all lines, only for the Cli to have to immediately pass them back to the Console to see if it can handle them. What's the point?

Maybe if you were to revert this change and then rename Console.readLine to getNextStatement or something it would make more sense to you?

I totally agree the architecture of the CLI is questionable. But I don't think this change helps. It actually couples code more, rather than less.

big-andy-coates

Thanks @agavra

I'm still not liking the move of the Cli specific command handling as I noted above.

Also, just a thought... but in terms of the functionality this PR provides I'm not sure WRAP is the right name. Maybe this is a standard thing, but for me I would not expect any clipping of output to be controlled by WRAP. I would of expected WRAP ON to cause long lines to be wrapped over multiple lines and WRAP OFF to mean long lines mean wide columns.

The functionalty feels to me like it would be better called CLIPPING. OFF means long output and ON means truncated/clipped.

Don't know if the WRAP was inspired by other similar features on other DBs, but I think its a poor name.

ksql-cli/src/main/java/io/confluent/ksql/cli/console/CliConfig.java

agavra · 2019-09-19T16:44:18Z

@big-andy-coates - okay, I will change the readline method back. This syntax was inspired by Oracles SqlPlus: https://docs.oracle.com/cd/B19306_01/server.102/b14357/ch12040.htm where they use WRAP OFF:

Controls whether to truncate the display of a selected row if it is too long for the current line width. OFF truncates the selected row; ON enables the selected row to wrap to the next line.

big-andy-coates · 2019-09-20T09:19:16Z

@big-andy-coates - okay, I will change the readline method back. This syntax was inspired by Oracles SqlPlus: https://docs.oracle.com/cd/B19306_01/server.102/b14357/ch12040.htm where they use WRAP OFF:

Fair enough, I wondered if that was the case.

Though... I thought you always looked to Postgres for inspiration - what's with following SqlPlus?!?!? :p

big-andy-coates

Thanks @agavra LGTM 'cept for a possible put in the clipping

big-andy-coates · 2019-09-20T09:27:18Z

ksql-cli/src/main/java/io/confluent/ksql/util/TabularRow.java

+        && parts.subList(rowsToPrint, parts.size())
+        .stream()
+        .map(String::trim)
+        .noneMatch(String::isEmpty);


// clip if there are more than one line and any of the remaining lines are non-empty

Isn't this check "if there are more than one line and NO LINES ARE EMPTY". So it would return false if there are multiple lines and one is empty.

I think maybe you want '! parts....allMatch(String::isEmpty)'

good catch! added a test for it

ksql-cli/src/test/java/io/confluent/ksql/cli/CliTest.java

agavra requested a review from rmoff September 12, 2019 19:55

agavra requested a review from a team as a code owner September 12, 2019 19:55

agavra force-pushed the clip_cli branch from 32e971f to 907b91b Compare September 17, 2019 16:30

big-andy-coates reviewed Sep 18, 2019

View reviewed changes

big-andy-coates requested a review from a team September 18, 2019 16:14

agavra added 3 commits September 18, 2019 10:33

feat(cli): add the feature to turn of WRAP for CLI output

c07f3c3

feat: change SET to SET CLI to distinguish

db38e56

fix: address andy's comments

8005e8b

agavra force-pushed the clip_cli branch from 907b91b to 8005e8b Compare September 18, 2019 18:05

agavra mentioned this pull request Sep 19, 2019

fix:Output to console the key and value that was inserted by a I… #3377

Closed

2 tasks

big-andy-coates reviewed Sep 19, 2019

View reviewed changes

ksql-cli/src/main/java/io/confluent/ksql/cli/console/CliConfig.java Show resolved Hide resolved

agavra requested a review from a team September 19, 2019 16:52

refactor: remove readline change

5b17f78

big-andy-coates approved these changes Sep 20, 2019

View reviewed changes

fix: andy's comments

24babf9

agavra merged commit 3814c71 into confluentinc:master Sep 24, 2019

agavra deleted the clip_cli branch September 24, 2019 15:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): add the feature to turn of WRAP for CLI output #3341

feat(cli): add the feature to turn of WRAP for CLI output #3341

agavra commented Sep 12, 2019 •

edited

Loading

vcrfxia commented Sep 16, 2019

rmoff commented Sep 17, 2019

agavra commented Sep 17, 2019

big-andy-coates commented Sep 18, 2019 •

edited

Loading

big-andy-coates left a comment

agavra commented Sep 18, 2019

agavra commented Sep 18, 2019

big-andy-coates commented Sep 19, 2019

big-andy-coates left a comment

agavra commented Sep 19, 2019 •

edited

Loading

big-andy-coates commented Sep 20, 2019

big-andy-coates left a comment

big-andy-coates Sep 20, 2019

agavra Sep 23, 2019

feat(cli): add the feature to turn of WRAP for CLI output #3341

feat(cli): add the feature to turn of WRAP for CLI output #3341

Conversation

agavra commented Sep 12, 2019 • edited Loading

Description

TODO

Testing done

Reviewer checklist

vcrfxia commented Sep 16, 2019

rmoff commented Sep 17, 2019

agavra commented Sep 17, 2019

big-andy-coates commented Sep 18, 2019 • edited Loading

big-andy-coates left a comment

Choose a reason for hiding this comment

agavra commented Sep 18, 2019

agavra commented Sep 18, 2019

big-andy-coates commented Sep 19, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

agavra commented Sep 19, 2019 • edited Loading

big-andy-coates commented Sep 20, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

big-andy-coates Sep 20, 2019

Choose a reason for hiding this comment

agavra Sep 23, 2019

Choose a reason for hiding this comment

agavra commented Sep 12, 2019 •

edited

Loading

big-andy-coates commented Sep 18, 2019 •

edited

Loading

agavra commented Sep 19, 2019 •

edited

Loading