Skip to content

fix: use the current_namespace with show tables statement #4134

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

Closed
wants to merge 1 commit into from

Conversation

rchowell
Copy link
Contributor

@rchowell rchowell commented Apr 1, 2025

Changes Made

This prepends the current_namespace to the list_tables call when executing the SQL SHOW TABLES statement. This fixes the current issue, but it's a bit more than that.

Ideally you can't give the namespace in the pattern, but in the FROM <location> which is not supported in sqlparser at the moment. So this PR makes it work like we want, but there's still work to do on how patterns are applied and validated.

What I really want is,

SHOW TABLES {FROM|IN} <catalog>.<namespace>

but the qualified identifier in the FROM position is not supported by sqlparser at the moment. I think long term I would prefer this, but the current workaround is to include the pattern in the namespace. I think using the current set namespace and prepending it automatically to the pattern gets close to what we actually want.

Related Issues

closes #4053

Checklist

  • Documented in API Docs (if applicable)
  • Documented in User Guide (if applicable)
  • If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
  • Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

@github-actions github-actions bot added the fix label Apr 1, 2025
Copy link

codecov bot commented Apr 2, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.43%. Comparing base (8fe5a2b) to head (36f56a8).
Report is 272 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4134   +/-   ##
=======================================
  Coverage   78.42%   78.43%           
=======================================
  Files         787      787           
  Lines      100864   100876   +12     
=======================================
+ Hits        79105    79120   +15     
+ Misses      21759    21756    -3     
Files with missing lines Coverage Δ
src/daft-sql/src/exec.rs 85.93% <100.00%> (+2.00%) ⬆️

... and 3 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@universalmind303 universalmind303 left a comment

Choose a reason for hiding this comment

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

so one thing I dont really like is that we're just adding it as a prefix, but not enforcing anywhere that it's an actual namespace.

for example.

from daft import Catalog, Session


cat = Catalog.from_pydict(
        name="cat3",
        tables={
            "aa_table": {"x": [1]},
            "aaa_table": {"x": [1]},
            "another_table": {"x": [1]},
            "a.table": {"x": [1]},
        },
    )
sess = Session()
sess.attach(cat)

sess.use("cat3.a")

sess.sql("show tables").collect()

i would expect this to only return a.table, but instead it returns all of the tables.

@rchowell
Copy link
Contributor Author

rchowell commented Apr 2, 2025

@universalmind303 true, I think this is a bug/fault of the in-memory catalog list_tables implementation.

I think what makes the most sense is to define our own pattern and allow prefixing with a namespace. The pyiceberg list_tables only takes a namespace then we can do the pattern evaluation on our end which would let us achieve #4007

@rchowell
Copy link
Contributor Author

Closing with reference to #4053. This work is not prioritized and would require deeper work that what's covered in this outdated PR.

@rchowell rchowell closed this Jun 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SHOW statement should use current namespace
2 participants