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.

Sign up for GitHub

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

Jump to bottom

Added database collation support #4629

Merged
merged 1 commit into from
Oct 31, 2022
Merged

Added database collation support #4629

merged 1 commit into from
Oct 31, 2022

Conversation

Hydrocharged
Copy link
Contributor

This allows setting the database collation, such that all newly created tables within a database (that do not explicitly set their collation) will inherit the database collation.

Builds on

Fixes

@Hydrocharged
Copy link
Contributor Author

@andrew-wm-arthur Tests are failing. I know what's wrong, fixing it is just non-trivial. Ignore the failures for now (they're on the GMS side of things) and review the PR as though it's all succeeding.

@andy-wm-arthur
Copy link
Contributor

andy-wm-arthur commented Oct 27, 2022 •
edited

I think this warrants a conversation about how we store database-level schema info. It's not clear to me if this should be written to the datas.Database or to the doltdb.RootValue.

What other pieces of database-level state might we add in the future?

@Hydrocharged Hydrocharged force-pushed the daylon/db-collation branch 3 times, most recently from 7f58d40 to 8133fa1 Compare October 27, 2022 14:20
@Hydrocharged Hydrocharged linked an issue Oct 27, 2022 that may be closed by this pull request
Create database with charset and collation not supported #4170
Closed
@Hydrocharged
Copy link
Contributor Author

@andrew-wm-arthur Moved the logic to *RootValue. Currently fails tests due to some strange interaction with transactions that I'm trying to figure out. Take a look at the *RootValue changes though, assuming no further feedback, I'll get this merged as soon as I find what's causing the transaction issues.

andy-wm-arthur
andy-wm-arthur approved these changes Oct 28, 2022
View reviewed changes
Copy link
Contributor

@andy-wm-arthur andy-wm-arthur left a comment

Choose a reason for hiding this comment

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

LGTM!

@timsehn
Copy link
Sponsor Contributor

timsehn commented Oct 28, 2022

You should unskip the bats test for this one:

dolt/integration-tests/bats/sql-charsets-collations.bats

Line 43 in f028cc4

skip "Defining charsets and collations on a database not supported"

@timsehn timsehn self-requested a review October 28, 2022 22:38
timsehn
timsehn requested changes Oct 28, 2022
View reviewed changes
Copy link
Sponsor Contributor

@timsehn timsehn left a comment

Choose a reason for hiding this comment

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

Also unskip the bats test!

@Hydrocharged Hydrocharged force-pushed the daylon/db-collation branch 2 times, most recently from b4023f7 to 4cd20b2 Compare October 31, 2022 12:57
@Hydrocharged Hydrocharged merged commit b6b1741 into main Oct 31, 2022
@Hydrocharged Hydrocharged deleted the daylon/db-collation branch October 31, 2022 14:48
@timsehn timsehn mentioned this pull request Dec 13, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timsehn timsehn timsehn requested changes

@andy-wm-arthur andy-wm-arthur andy-wm-arthur approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create database with charset and collation not supported
3 participants
@Hydrocharged @andy-wm-arthur @timsehn