Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add describe option to list streams/tables #6827

Merged
merged 10 commits into from
Feb 17, 2021
Merged

feat: add describe option to list streams/tables #6827

merged 10 commits into from
Feb 17, 2021

Conversation

lct45
Copy link
Contributor

@lct45 lct45 commented Jan 4, 2021 •
edited
Loading

Description

Add a DESCRIBE option for LIST STREAMS/TABLES to give a subset of LIST STREAMS/TABLES EXTENDED in order to cut down on the cost of returning pertinent info to the UI, per KSQL-5749. This PR also changes the DESCRIBE SOURCE syntax to be DESCRIBE SOURCE EXTENDED instead of DESCRIBE EXTENDED SOURCE to match the wording of DESCRIBE TABLES/STREAMS EXTENDED

Testing done

Added unit tests in accordance with testing done for EXTENDED

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 #")

@lct45 lct45 requested review from rodesai and a team January 4, 2021 22:27
@ghost
Copy link

ghost commented Jan 4, 2021

It looks like @lct45 hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement here.

Once you've signed reply with [clabot:check] to prove it.

Appreciation of efforts,

clabot

@lct45
Copy link
Contributor Author

lct45 commented Jan 4, 2021

[clabot:check]

@ghost
Copy link

ghost commented Jan 4, 2021

@confluentinc It looks like @lct45 just signed our Contributor License Agreement. 👍

Always at your service,

clabot

rodesai
rodesai reviewed Jan 6, 2021
View reviewed changes
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lct45! This is looking good. I've left some feedback inline. We probably want another keyword than DESCRIPTION.

ksqldb-engine/src/test/java/io/confluent/ksql/integration/JoinIntTest.java Outdated Show resolved Hide resolved
ksqldb-parser/src/test/java/io/confluent/ksql/parser/KsqlParserTest.java Show resolved Hide resolved
ksqldb-parser/src/test/java/io/confluent/ksql/parser/KsqlParserTest.java Show resolved Hide resolved
ksqldb-parser/src/main/java/io/confluent/ksql/parser/tree/ListStreams.java Outdated Show resolved Hide resolved
@lct45 lct45 force-pushed the uiListing branch 2 times, most recently from 23fc4f0 to 10e1d32 Compare January 6, 2021 16:45
spena
spena reviewed Jan 27, 2021
View reviewed changes
ksqldb-parser/src/main/antlr4/io/confluent/ksql/parser/SqlBase.g4 Outdated
@@ -55,6 +55,7 @@ statement
| (LIST | SHOW) TYPES #listTypes
| (LIST | SHOW) VARIABLES #listVariables
| DESCRIBE EXTENDED? sourceName #showColumns
| DESCRIBE EXTENDED? STREAMS #describeStreams
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this syntax going to confuse users between the use of show streams extended and describe extended streams? fyi @rodesai

I was getting confused too, so I wonder how users would react to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I do think it's somewhat confusing. It's sticking with the current semantic set up though since we have describe extended sourcename verses list streams extended so I'm not sure it would make sense to switch it to describe streams extended if we don't also switch it to describe sourceName extended. We briefly mentioned this when talking to @derekjn and @colinhicks but there wasn't a full discussion

@lct45 lct45 requested a review from JimGalasyn as a code owner February 1, 2021 19:20
@lct45
Copy link
Contributor Author

lct45 commented Feb 1, 2021

per discussion with @derekjn, transitioning describe to always have extended after the source name or streams/tables. ex DESCRIBE EXTENDED stream1 -> DESCRIBE stream1 EXTENDED. Due to the issues with DESCRIBE TABLES, this ended up being one PR instead of breaking it out into a follow-up

JimGalasyn
JimGalasyn approved these changes Feb 1, 2021
View reviewed changes
Copy link
Member

@JimGalasyn JimGalasyn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

lct45
lct45 commented Feb 1, 2021
View reviewed changes
docs/developer-guide/ksqldb-reference/describe.md
KBYTES | BIGINT
-----------------------------------------

Queries that write into this TABLE
Copy link
Contributor Author

@lct45 lct45 Feb 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not 100% sure if this is the same, it looks like this statement was moved to be under statement above, looks like the description might've been updated and the docs weren't does anyone know?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know -- please feel free to fix it as you see fit!

@MichaelDrogalis
Copy link
Contributor

Docs updates LGTM.

rodesai
rodesai approved these changes Feb 2, 2021
View reviewed changes
Copy link
Contributor

@rodesai rodesai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, other than a couple small comments inline. Feel free to merge once those are addressed.

ksqldb-parser/src/main/java/io/confluent/ksql/parser/AstBuilder.java Show resolved Hide resolved
ksqldb-rest-app/src/main/java/io/confluent/ksql/rest/server/execution/ListSourceExecutor.java Show resolved Hide resolved
@lct45 lct45 force-pushed the uiListing branch 2 times, most recently from c195891 to 820ff5f Compare February 11, 2021 16:18
@lct45 lct45 merged commit 26e3dea into confluentinc:master Feb 17, 2021
@colinhicks
Copy link
Contributor

@lct45 it would be good to tweak the PR description to match the syntax that we landed upon. e.g. describe tables; rather
than list tables description;.

@lct45 lct45 mentioned this pull request Mar 1, 2021
Closed
@lct45 lct45 deleted the uiListing branch July 8, 2021 15:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JimGalasyn JimGalasyn JimGalasyn approved these changes

@spena spena spena left review comments

@rodesai rodesai rodesai approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lct45 @MichaelDrogalis @colinhicks @rodesai @spena @JimGalasyn