-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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.
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
Support SHOW ALL VERBOSE
to show settings description
#7735
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @comphead -- I really like that the descriptions are now included.
I think the change to make it possible to select all the settings directly from df_settings
is great 💯
select * from information_schema.df_settings
As you mention, I am not so sure about including the description in SHOW <name>
and SHOW ALL
as it makes the output much more verbose.
I think it would be a nicer experience if SHOW ALL
kept the current behavior and we extended SHOW ALL VERBOSE
and SHOW <value> VERBOSE
that added the description too.
I checked and this appears to be supported by the parser so we could probably make this work without waiting on an upstream change
❯ show all verbose;
0 rows in set. Query took 0.002 seconds.
What do you think?
@alamb thanks. My concern with the first sentence as the developer who introducing the new param is not aware about concise first sentence requirements and this will still eventually lead to the wide output. Having another verbose pair makes much more sense for me.
|
I agree |
# Conflicts: # datafusion/sqllogictest/test_files/information_schema.slt
Added |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks really nice to me -- thank you @comphead
@@ -202,12 +202,85 @@ datafusion.sql_parser.dialect generic | |||
datafusion.sql_parser.enable_ident_normalization true | |||
datafusion.sql_parser.parse_float_as_decimal false | |||
|
|||
# show all variables with verbose | |||
query TTT rowsort |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🥇
```SQL | ||
> show all; | ||
|
||
+-------------------------------------------------+---------+ | ||
| name | setting | | ||
| name | value | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this change in column name 👍
cc @waitingkuo |
SHOW ALL VERBOSE
to show settings description
Thanks again @comphead -- this is a really nice contribution |
Which issue does this PR close?
Closes #7736 .
Rationale for this change
Expand SHOW ALL stmt to show settings description. Currently it requires going through the code to find the description for the specific datafusion setting, moreover code covered by macros which makes harder to understand the full setting name.
What changes are included in this PR?
The PR is to add settings description to the
SHOW ALL
outputAre these changes tested?
Yes
Are there any user-facing changes?
Yes, the
SHOW ALL
output have extradescription
column and confusingsettings
column name was renamed tovalue