feat(cli): improve CLI transient queries with headers and spacing (partial fix for #892) #3047

agavra · 2019-07-02T23:53:15Z

Description

Ladies and Gentlemen, the moment you have all been waiting for! Hot off the press 🔥 🔥 🔥 Formatted CLI output for transient queries, with a shiny new header row 🎉 🎉 🎉

ksql> SELECT * FROM ALL_PUBLICATIONS;
+-----------------------+-----------------------+-----------------------+-----------------------+
|ROWTIME                |ROWKEY                 |AUTHOR                 |TITLE                  |
+-----------------------+-----------------------+-----------------------+-----------------------+
|1562173641326          |C.S. Lewis             |C.S. Lewis             |The Silver Chair       |
|1562173641364          |George R. R. Martin    |George R. R. Martin    |A Song of Ice and Fire |
|1562173641378          |C.S. Lewis             |C.S. Lewis             |Perelandra             |
|1562173641392          |George R. R. Martin    |George R. R. Martin    |Fire & Blood           |
|1562173641406          |J. R. R. Tolkien       |J. R. R. Tolkien       |The Hobbit             |
|1562173641419          |J. R. R. Tolkien       |J. R. R. Tolkien       |The Lord of the Rings  |
|1562173641432          |George R. R. Martin    |George R. R. Martin    |A Dream of Spring      |
|1562173641446          |J. R. R. Tolkien       |J. R. R. Tolkien       |The Fellowship of the R|
|                       |                       |                       |ing                    |
|1562173641460          |George R. R. Martin    |George R. R. Martin    |The Ice Dragon         |

It even does very fancy 💃 🕺 wrapping for lines that are too long and adjusts based on your terminal size.

Notes

This has a hacked version of #3043 as part of it, we should resolve that independently

Testing done

Unit testing
CLI integration 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 #")

MichaelDrogalis · 2019-07-03T02:36:23Z

May I suggest a slight cosmetic adjustment to match MySQL?

+---------+--------+-------+
| article | dealer | price |
+---------+--------+-------+
|       1 | A      |  3.45 |
|       1 | B      |  3.99 |
|       2 | A      | 10.99 |
|       3 | B      |  1.45 |
|       3 | C      |  1.69 |
|       3 | D      |  1.25 |
|       4 | D      | 19.95 |
+---------+--------+-------+

Top row of dashes before the headers
Row of dashes under the headers
No dashes between rows

Mostly curious to see how it looks if we try to line 'em up

hjafarpour · 2019-07-03T06:57:40Z

This is great @agavra , I agree with @MichaelDrogalis 's comment on matching MySQL. Postgres also is very similar to MySQL. That way it would really look and feel like a database! ;)

big-andy-coates

Thanks @agavra

Some suggestions below. The reason I've requested changes is the attempt to get a fix in for the double meta column bug, which is fixed in my own PR.

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

big-andy-coates · 2019-07-03T15:28:17Z

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

+  private final List<String> value;
+  private final List<String> header;
+
+  public TabularRow(


header and value are used mutually exclusively. They both map to a list of strings. So this class could be simplified to just take the width and single List<String> param, pushing up the computation of what is passed as the list of strings to the caller, or maybe two factory methods.

the reason I have it this way is that in the near future I want to change the column format based on the header - so passing in both will soon be helpful (e.g. fixed width for int columns, etc...)

OK, in that case I'd still go for two factory methods to make this clear:

TabularRow.createHeader(headers) vs TabularRow.createRow(headers, row).

And you can still decouple this class from FieldInfo and GenericRow by having the calling code do the toString stuff. Less couple is good! Plus, you can process the headers once for the query, rather than per-row.

And you can still decouple this class from FieldInfo and GenericRow by having the calling code do the toString stuff.

I don't think the current calling class would be a good place to do that. I could introduce another wrapper class (e.g. Tabular) that handled all of the row widths based on the field infos and things like that, but that is overkill for this PR. As it stands, it's pretty decoupled - it's just in a constructor (moved to be static initializer).

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

big-andy-coates · 2019-07-03T15:37:28Z

ksql-engine/src/main/java/io/confluent/ksql/planner/plan/KsqlBareOutputNode.java

@@ -39,7 +39,17 @@ public KsqlBareOutputNode(
      final OptionalInt limit,
      final TimestampExtractionPolicy extractionPolicy
  ) {
-    super(id, source, schema, limit, extractionPolicy);
+    super(


Don't think adding this was intentional.... we shouldn't be doing this.

I was just hacking it to make it work - wasn't going to commit until your fix is in :)

ksql-rest-app/src/main/java/io/confluent/ksql/rest/util/EntityUtil.java

ksql-common/src/main/java/io/confluent/ksql/schema/ksql/LogicalSchema.java

big-andy-coates · 2019-07-03T15:43:49Z

ksql-cli/src/test/java/io/confluent/ksql/util/TabularRowTest.java

+  }
+
+  @Test
+  public void shouldMultilineFormatRow() {


Add test for what happens if the number of columns is higher than the width....

Interesting edge case!

😨 good catch! I think if this is the case I'll default back to the old behavior

I've decided to set a minimum cell width of 5 - it'll look all weird if the terminal is super small, but that's going to be the case anyway

spena · 2019-07-03T15:55:28Z

@agavra While testing this, I see that the output fits the full screen. Can we use a fixed column instead? Mysql and other Dbs do not fit the whole screen, although I understand that those DBs know what's the max value read to use it in the column width, and KSQL does not know as it is streaming real-time data.

Btw, if I stop the query and rerun it again, the headers are not displayed anymore.

ksql> select * from t1;
|ROWTIME                                                |ROWKEY                                                 |ID                                                     |
+═══════════════════════════════════════════════════════+═══════════════════════════════════════════════════════+═══════════════════════════════════════════════════════+
|1562104196553                                          |null                                                   |1                                                      |
+-------------------------------------------------------+-------------------------------------------------------+-------------------------------------------------------+
^CQuery terminated
      select * from t1;
|1562104196553                                                                     |null                                                                              |1                                                                                 |
+----------------------------------------------------------------------------------+----------------------------------------------------------------------------------+----------------------------------------------------------------------------------+
ksql> select * from t1;
|1562104196553                                                                     |null                                                                              |1                                                                                 |
+----------------------------------------------------------------------------------+----------------------------------------------------------------------------------+----------------------------------------------------------------------------------+
ksql> 
ksql> 
ksql> select * from t1;
|1562104196553                                                                     |null                                                                              |1                                                                                 |
+----------------------------------------------------------------------------------+----------------------------------------------------------------------------------+----------------------------------------------------------------------------------+

agavra · 2019-07-03T15:58:08Z

@spena - I'm not sure what's going on with your terminal, but my headers do show multiple times...

and yeah, my concern was that I don't know the max value length when I start it up. The other SQL engines also have more powerful formatting options for the user to choose how they want it to show up. I think until we implement those, we should have a good default option (which, to me, full width makes sense)

+--------------------------------+--------------------------------+--------------------------------+
|ROWTIME                         |ROWKEY                          |ID                              |
+--------------------------------+--------------------------------+--------------------------------+
|1562021756897                   |a                               |1                               |
^CQuery terminated
ksql> SELECT * FROM FOO;
+--------------------------------+--------------------------------+--------------------------------+
|ROWTIME                         |ROWKEY                          |ID                              |
+--------------------------------+--------------------------------+--------------------------------+
|1562021756897                   |a                               |1                               |
^CQuery terminated
ksql> SELECT * FROM FOO;
+--------------------------------+--------------------------------+--------------------------------+
|ROWTIME                         |ROWKEY                          |ID                              |
+--------------------------------+--------------------------------+--------------------------------+
|1562021756897                   |a                               |1                               |
^CQuery terminated
ksql>

Also tested with resizing terminal:

|ROWTIME                   |ROWKEY                    |ID                        |
+--------------------------+--------------------------+--------------------------+
|1562021756897             |a                         |1                         |
^CQuery terminated
ksql> SELECT * FROM FOO;
+----------------------------------------------+----------------------------------------------+----------------------------------------------+
|ROWTIME                                       |ROWKEY                                        |ID                                            |
+----------------------------------------------+----------------------------------------------+----------------------------------------------+
|1562021756897                                 |a                                             |1                                             |
^CQuery terminated
ksql> SELECT * FROM FOO;
+-----------------+-----------------+-----------------+
|ROWTIME          |ROWKEY           |ID               |
+-----------------+-----------------+-----------------+
|1562021756897    |a                |1                |
^CQuery terminated

vcrfxia

LGTM! Thanks @agavra -- very nice feature :)

big-andy-coates

Thanks for the update @agavra.

I'm with you on using the full width by default as a good starting point.

big-andy-coates · 2019-07-04T12:43:53Z

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

+  private final List<String> value;
+  private final List<String> header;
+
+  public TabularRow(


OK, in that case I'd still go for two factory methods to make this clear:

TabularRow.createHeader(headers) vs TabularRow.createRow(headers, row).

And you can still decouple this class from FieldInfo and GenericRow by having the calling code do the toString stuff. Less couple is good! Plus, you can process the headers once for the query, rather than per-row.

big-andy-coates · 2019-07-04T12:49:05Z

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

+  }
+
+  @SuppressWarnings("ForLoopReplaceableByForEach") // clearer to read this way
+  private static void toString(


nit: rename formatRow or something.

agavra requested a review from a team as a code owner July 2, 2019 23:53

big-andy-coates suggested changes Jul 3, 2019

View reviewed changes

big-andy-coates requested a review from a team July 3, 2019 15:42

big-andy-coates reviewed Jul 3, 2019

View reviewed changes

agavra force-pushed the cli_header branch from 837ff85 to 3317333 Compare July 3, 2019 17:11

vcrfxia approved these changes Jul 3, 2019

View reviewed changes

vcrfxia requested a review from a team July 3, 2019 22:04

big-andy-coates approved these changes Jul 4, 2019

View reviewed changes

agavra added 4 commits July 15, 2019 09:28

feat(cli): improve CLI transient queries with headers and spacing

b1e93df

chore: various improvements based on comments and suggestions

4560e47

style: extracted a constant

c61bba1

chore: minor changes & fix things

dd214a5

agavra force-pushed the cli_header branch from 3317333 to dd214a5 Compare July 15, 2019 19:46

agavra added 2 commits July 15, 2019 12:57

fix: some issues

93f7bf9

fix: fix dead store

24b0b9e

agavra merged commit 050b72a into confluentinc:master Jul 15, 2019

agavra deleted the cli_header branch July 15, 2019 22:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(cli): improve CLI transient queries with headers and spacing (partial fix for #892) #3047

feat(cli): improve CLI transient queries with headers and spacing (partial fix for #892) #3047

agavra commented Jul 2, 2019 •

edited

Loading

MichaelDrogalis commented Jul 3, 2019

hjafarpour commented Jul 3, 2019

big-andy-coates left a comment

big-andy-coates Jul 3, 2019

agavra Jul 3, 2019

big-andy-coates Jul 4, 2019

agavra Jul 15, 2019

big-andy-coates Jul 3, 2019

agavra Jul 3, 2019

big-andy-coates Jul 3, 2019

agavra Jul 3, 2019

agavra Jul 3, 2019

spena commented Jul 3, 2019

agavra commented Jul 3, 2019 •

edited

Loading

vcrfxia left a comment

big-andy-coates left a comment

big-andy-coates Jul 4, 2019

big-andy-coates Jul 4, 2019

feat(cli): improve CLI transient queries with headers and spacing (partial fix for #892) #3047

feat(cli): improve CLI transient queries with headers and spacing (partial fix for #892) #3047

Conversation

agavra commented Jul 2, 2019 • edited Loading

Description

Notes

Testing done

Reviewer checklist

MichaelDrogalis commented Jul 3, 2019

hjafarpour commented Jul 3, 2019

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spena commented Jul 3, 2019

agavra commented Jul 3, 2019 • edited Loading

vcrfxia left a comment

Choose a reason for hiding this comment

big-andy-coates left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agavra commented Jul 2, 2019 •

edited

Loading

agavra commented Jul 3, 2019 •

edited

Loading