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

implement implicit commits and fix import behavior #8767

Merged
merged 19 commits into from
Jan 22, 2025
Merged

Conversation

jycor
Copy link
Contributor

@jycor jycor commented Jan 17, 2025

This PR implements the implicit commit logic introduced in this PR dolthub/go-mysql-server#2818
On error, dolt table import -c does not create the table, and we did that by simply rolling back the existing transaction.
Since DDL statements now implcitly COMMIT, we need to start a new transaction, and possibly drop any tables created.

fixes #7485
maybe fixes: #8716

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
213cf1c ok 5937457
version total_tests
213cf1c 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
4d191da ok 5937457
version total_tests
4d191da 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
0b52336 ok 5937457
version total_tests
0b52336 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
a271d15 ok 5937457
version total_tests
a271d15 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
f238a57 ok 5937457
version total_tests
f238a57 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
7a52606 ok 5937457
version total_tests
7a52606 5937457
correctness_percentage
100.0

@jycor jycor changed the title bump implement implicit commits and fix import behavior Jan 21, 2025
Copy link
Member

@zachmu zachmu left a comment

Choose a reason for hiding this comment

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

LGTM

line := 1
// If there were create table statements, they are automatically committed, so we need to start a new transaction
if s.importOption == CreateOp {
_, _, _, err = s.se.Query(s.sqlCtx, "START TRANSACTION")
Copy link
Member

Choose a reason for hiding this comment

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

Always drain the row iter for these things, we often don't throw any error until iteration or even Close()

// quitting import that created table, should drop table
if s.importOption == CreateOp {
var err error
_, _, _, err = s.se.Query(s.sqlCtx, fmt.Sprintf("DROP TABLE IF EXISTS `%s`", s.tableName))
Copy link
Member

Choose a reason for hiding this comment

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

Same comment

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
916031a ok 5937457
version total_tests
916031a 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
ccb71e4 ok 5937457
version total_tests
ccb71e4 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@coffeegoddd DOLT

comparing_percentages
100.000000 to 100.000000
version result total
c0d2b1c ok 5937457
version total_tests
c0d2b1c 5937457
correctness_percentage
100.0

@coffeegoddd
Copy link
Contributor

@jycor DOLT

comparing_percentages
100.000000 to 100.000000
version result total
3c1987e ok 5937457
version total_tests
3c1987e 5937457
correctness_percentage
100.0

@jycor jycor merged commit 758627e into main Jan 22, 2025
21 checks passed
@jycor jycor deleted the james/implicit-bump branch January 22, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants