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.

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

Merged
merged 4 commits into from
Oct 7, 2023

Conversation

comphead
Copy link
Contributor

@comphead comphead commented Oct 3, 2023

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 output

+------------------------------------------------------------+---------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| name                                                       | value                     | description                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             |
+------------------------------------------------------------+---------------------------+---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------+
| datafusion.catalog.create_default_catalog_and_schema       | true                      | Whether the default catalog and schema should be created automatically.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                 |
| datafusion.catalog.default_catalog                         | datafusion                | The default catalog name - this impacts what SQL queries use if not specified                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |
| datafusion.catalog.default_schema                          | public                    | The default schema name - this impacts what SQL queries use if not specified                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            |
| datafusion.catalog.format                                  |                           | Type of `TableProvider` to use when loading `default` schema                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                            |
| datafusion.catalog.has_header                              | false                     | If the file has a header   

Are these changes tested?

Yes

Are there any user-facing changes?

Yes, the SHOW ALL output have extra description column and confusing settings column name was renamed to value

@github-actions github-actions bot added sql SQL Planner core Core DataFusion crate sqllogictest SQL Logic Tests (.slt) labels Oct 3, 2023
Copy link
Contributor

@alamb alamb left a 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?

@comphead
Copy link
Contributor Author

comphead commented Oct 4, 2023

@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.

SHOW ALL VERBOSE
SHOW value VERBOSE

@alamb
Copy link
Contributor

alamb commented Oct 4, 2023

Having another verbose pair makes much more sense for me.

I agree

# Conflicts:
#	datafusion/sqllogictest/test_files/information_schema.slt
@comphead
Copy link
Contributor Author

comphead commented Oct 5, 2023

Having another verbose pair makes much more sense for me.

I agree

Added SHOW ALL VERBOSE, SHOW param VERBOSE support.
User doc updated

@comphead comphead requested a review from alamb October 5, 2023 15:12
Copy link
Contributor

@alamb alamb left a 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
Copy link
Contributor

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 |
Copy link
Contributor

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 👍

@alamb
Copy link
Contributor

alamb commented Oct 5, 2023

cc @waitingkuo

@alamb alamb changed the title Expand SHOW ALL stmt to show settings description Support SHOW ALL VERBOSE to show settings description Oct 5, 2023
@alamb alamb merged commit e23d34b into apache:main Oct 7, 2023
24 checks passed
@alamb
Copy link
Contributor

alamb commented Oct 7, 2023

Thanks again @comphead -- this is a really nice contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core DataFusion crate sql SQL Planner sqllogictest SQL Logic Tests (.slt)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add settings description in SHOW ALL output
2 participants