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

Add support for optional catalog name to SHOW SCHEMAS | TABLES #1063

Merged
merged 10 commits into from
Apr 5, 2023

Conversation

charlesbluca
Copy link
Collaborator

@charlesbluca charlesbluca commented Feb 24, 2023

WIP to unblock some of the functionality of Metabase using its deprecated Presto driver with our server implementation.

TODO:

  • Add support for SHOW TABLES FROM <catalog-name>.<schema-name>

Comment on lines 35 to 39
catalog_name = show_schemas.getFrom() or context.catalog_name
if catalog_name != context.catalog_name:
raise RuntimeError(
f"A catalog with the name {catalog_name} is not present."
)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For now, we are able to parse statements like SHOW SCHEMAS FROM "dask_sql", but fail in any case where the catalog name isn't dask_sql; I figured it would make more sense to actually bother refactoring the Python handling of catalogs once we add functionality to add/remove/switch catalogs through queries.

dask_sql/server/app.py Outdated Show resolved Hide resolved
@codecov-commenter
Copy link

codecov-commenter commented Feb 24, 2023

Codecov Report

Merging #1063 (2576877) into main (ce55082) will decrease coverage by 2.21%.
The diff coverage is 100.00%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main    #1063      +/-   ##
==========================================
- Coverage   81.75%   79.55%   -2.21%     
==========================================
  Files          78       78              
  Lines        4380     4387       +7     
  Branches      788      790       +2     
==========================================
- Hits         3581     3490      -91     
- Misses        626      720      +94     
- Partials      173      177       +4     
Impacted Files Coverage Δ
dask_sql/physical/rel/custom/__init__.py 100.00% <100.00%> (ø)
dask_sql/physical/rel/custom/show_schemas.py 87.50% <100.00%> (ø)
dask_sql/physical/rel/custom/show_tables.py 86.36% <100.00%> (+3.03%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@charlesbluca charlesbluca changed the title Add support for SHOW SCHEMAS FROM <catalog-name> Add support for optional catalog name to SHOW SCHEMAS | TABLES Mar 13, 2023
@jdye64
Copy link
Collaborator

jdye64 commented Mar 13, 2023

@charlesbluca just wanted to share this here since we talked about this today. Seems like after this merges things should be much more straightforward here.

apache/datafusion#5343 (comment)

@charlesbluca charlesbluca marked this pull request as ready for review March 16, 2023 15:12
Copy link
Collaborator

@jdye64 jdye64 left a comment

Choose a reason for hiding this comment

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

@charlesbluca looks good. Had one small request

dask_planner/src/parser.rs Outdated Show resolved Hide resolved
dask_planner/src/sql/logical/show_schemas.rs Show resolved Hide resolved
Copy link
Collaborator

@ayushdg ayushdg left a comment

Choose a reason for hiding this comment

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

Minor suggestion but not a blocker

tests/integration/test_show.py Show resolved Hide resolved
Comment on lines -55 to -57
gpuci_logger "Install latest dask-cuda"
gpuci_mamba_retry update -y -c rapidsai-nightly dask-cuda

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't think this should be necessary anymore? Removing because the recent pinning of dask/distributed in dask-cuda nightlies is causing GPU failures due to the order of install here

@charlesbluca charlesbluca merged commit 0f6cab8 into dask-contrib:main Apr 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants