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

migrate dolt ls to use sql queries #6834

Merged
merged 9 commits into from
Oct 19, 2023
Merged

migrate dolt ls to use sql queries #6834

merged 9 commits into from
Oct 19, 2023

Conversation

stephkyou
Copy link
Contributor

@stephkyou stephkyou commented Oct 18, 2023

This change updates dolt ls to use the appropriate sql engine to generate results. This change also creates a system variable that, when enabled, shows dolt system tables in show tables and information_schema.tables.

Related: #3922
Resolves: #2073

@stephkyou stephkyou marked this pull request as ready for review October 18, 2023 19:12
@stephkyou stephkyou requested a review from macneale4 October 18, 2023 19:12
Copy link
Contributor

@macneale4 macneale4 left a comment

Choose a reason for hiding this comment

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

Very close. The only objection I have at the moment is that we don't test the SQL directly with the new flag. I think a couple enginetests are in order because we are testing it exclusively as a part of dolt ls - there are going to be people who want this in the sql conext so we should test that way

integration-tests/bats/ls.bats Outdated Show resolved Hide resolved
}

r, err := cm.GetRootValue(ctx)

_, _, err = queryist.Query(sqlCtx, "set @@dolt_show_system_tables = 1")
Copy link
Contributor

Choose a reason for hiding this comment

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

Question - does it work if we just put this set call as a prefix on the main query?

"set @@dolt_show_system_tables = 1; show tables ... "

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No they don't work all in one string. I think the way dolt sql -q is set up doesn't allow multiple statements in one call.

Copy link
Contributor

Choose a reason for hiding this comment

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

huh. Sounds like a feature we should have. ok. never mind for now!

go/cmd/dolt/commands/ls.go Show resolved Hide resolved
go/cmd/dolt/commands/ls.go Outdated Show resolved Hide resolved
go/libraries/doltcore/sqle/database.go Show resolved Hide resolved
Copy link
Contributor

@macneale4 macneale4 left a comment

Choose a reason for hiding this comment

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

    __|__ |___| |\
    |o__| |___| | \
    |___| |___| |o \
   _|___| |___| |__o\
  /...\_____|___|____\_/
  \   o * o * * o o  /
~~~~~~~~~~~~~~~~~~~~~~~~~~

@stephkyou stephkyou merged commit 3688b83 into main Oct 19, 2023
@stephkyou stephkyou deleted the steph/ls branch October 19, 2023 16:50
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.

Need a way to list all dolt system tables in SQL context
2 participants