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 diff and dolt show to SQL implementation #6221

Merged
merged 32 commits into from
Jun 28, 2023

Conversation

PavelSafronov
Copy link
Contributor

This change migrates the current dolt diff implementation to use only SQL.
This is done by offloading most of the diffing logic to dolt_diff and dolt_schema_diff table functions, and making some of the common code generic, so we don't have to rely on DoltDb objects (which rely on dolt internals).

This change also partially migrates dolt show to SQL.
dolt show has the ability to log some of the storage internals in two cases:

  1. --no-pretty flag is specified
  2. dolt show is invoked against non-commit hashes

When either of the above cases occurs, we use DoltEnv to get these structures and log them. In all other cases, we use SQL to get the list of changes.

go/cmd/dolt/commands/diff.go Outdated Show resolved Hide resolved
go/cmd/dolt/commands/diff.go Outdated Show resolved Hide resolved
integration-tests/bats/rename-tables.bats Outdated Show resolved Hide resolved
integration-tests/bats/diff.bats Show resolved Hide resolved
go/cmd/dolt/commands/show.go Show resolved Hide resolved
go/cmd/dolt/commands/show.go Show resolved Hide resolved
go/cmd/dolt/commands/diff_output.go Show resolved Hide resolved
go/cmd/dolt/commands/diff.go Outdated Show resolved Hide resolved
go/cmd/dolt/commands/diff.go Outdated Show resolved Hide resolved
go/cmd/dolt/commands/diff.go Outdated Show resolved Hide resolved
@macneale4 macneale4 self-requested a review June 27, 2023 00:40
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.

Looking good. Get the tests passing and ship it!

PavelSafronov and others added 25 commits June 26, 2023 17:51
- temporarily disable `dolt show`
- introduce TableInfo struct to maintain SQL table state
- update diffWriter to use non-doltdb primitives
- update diff.go to use non-doltdb primitives and load most data from SQL
- got basic commit details SQL going
- able to print a rudimentary --no-pretty output for basic case
Update `dolt_log` table function to handle "HEAD" as a ref.
…t the create statement.

Also, only write out alter statements when they are non-empty.
Update diff.bats tests to remove foreign key resolution text.
When running the command, we determine if `DoltEnv` is required by checking if `--no-pretty` is set, or the user specified non-commit hashes as the input. These two scenarios result in us logging storage-specific data, for which we require `DoltEnv`.
- pass in the timestamp to the error that reports timestamp parsing issues
- fix a failing BATS test
- use dbr.InterpolateForDialect to construct queries
- remove commented-out lines
- fix function comments
- add comments to resolveNonCommitSpec
- rename all show.go `show*` functions to `print*`
- moved getTimestampColAsUint64 to utils
- if needing to execute in local mode, check the type of Queryist to figure this out
@PavelSafronov PavelSafronov force-pushed the pavel/dolt-diff-migration branch from 1d59d8d to dfdcbc4 Compare June 27, 2023 00:51
… `limit` in `dbr.Expr`, which avoids quoting the contents.
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.

Small change requested for how you set the password in the tests. fix and ship!

cd defaultDB

export DOLT_CLI_PASSWORD=""
Copy link
Contributor

Choose a reason for hiding this comment

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

We unset the DOLT_CLI_PASSWORD var in this file, so I'd prefer you just set the --password everywhere, OR you can specify no user/pwd (which should work now that my auth changes are in).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, dropping --user arguments in these tests.

@@ -384,6 +386,8 @@ EOF
@test "sql-local-remote: verify dolt show behavior" {
cd defaultDB

export DOLT_CLI_PASSWORD=""
Copy link
Contributor

Choose a reason for hiding this comment

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

See previous comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous response :)

@PavelSafronov PavelSafronov merged commit de4c645 into main Jun 28, 2023
@Hydrocharged Hydrocharged deleted the pavel/dolt-diff-migration branch August 24, 2023 00:08
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.

2 participants