Skip to content

Support for using TEXT as a foreign key target#1260

Closed
zachmu wants to merge 7 commits intomainfrom
zachmu/foreign-key-text-field
Closed

Support for using TEXT as a foreign key target#1260
zachmu wants to merge 7 commits intomainfrom
zachmu/foreign-key-text-field

Conversation

@zachmu
Copy link
Copy Markdown
Member

@zachmu zachmu commented Mar 6, 2025

This PR relies on dolthub/go-mysql-server#2882

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2025

Main PR
covering_index_scan_postgres 378.06/s 380.19/s +0.5%
index_join_postgres 146.20/s 148.01/s +1.2%
index_join_scan_postgres 179.57/s 180.95/s +0.7%
index_scan_postgres 12.13/s 12.16/s +0.2%
oltp_point_select 2730.27/s 2763.19/s +1.2%
oltp_read_only 1819.80/s 1810.59/s -0.6%
select_random_points 104.32/s 104.82/s +0.4%
select_random_ranges 131.28/s 130.78/s -0.4%
table_scan_postgres 10.01/s 10.10/s +0.8%
types_table_scan_postgres 5.27/s 5.31/s +0.7%

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 6, 2025

Main PR
Total 42090 42090
Successful 15693 15694
Failures 26397 26396
Partial Successes1 5201 5201
Main PR
Successful 37.2844% 37.2868%
Failures 62.7156% 62.7132%

${\color{lightgreen}Progressions (1)}$

triggers

QUERY: create table convslot_test_child (col1 text primary key,
	foreign key (col1) references convslot_test_parent(col1) on delete cascade on update cascade
);

Footnotes

  1. These are tests that we're marking as Successful, however they do not match the expected output in some way. This is due to small differences, such as different wording on the error messages, or the column names being incorrect while the data itself is correct.

@zachmu zachmu enabled auto-merge March 7, 2025 16:22
@zachmu zachmu requested a review from Hydrocharged March 7, 2025 16:54

// foreignKeyComparableTypes returns whether the two given types are able to be used as parent/child columns in a
// foreign key.
func foreignKeyComparableTypes(ctx *sql.Context, type1 sql.Type, type2 sql.Type) bool {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

These can be DoltgresTypes rather than sql.Type

// There seems to be a special case where CHAR/VARCHAR/BINARY/VARBINARY can have unequal lengths.
// Have not tested every type nor combination, but this seems specific to those 4 types.
if type1.Type() == type2.Type() {
switch type1.Type() {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Adding on to my other comment, we aren't too strict on ensuring that these are returned correctly, considering they don't map to all of our types so it's not really possible to do so. Instead, it's better to check for these specific types. You could even use type1.ID.TypeName() and switch on the string name. But like oid returns sqltypes.VarChar cause there's no oid type in MySQL so we're just returning something.

I think switch type1.ID with case pgtypes.Varchar.ID should work, but if not you can grab the case values using Id.CaseString() and match on the string directly (so you'd just use case "CaseString_Output").

@zachmu
Copy link
Copy Markdown
Member Author

zachmu commented Mar 12, 2025

Subsumed by #1277

@zachmu zachmu closed this Mar 12, 2025
auto-merge was automatically disabled March 12, 2025 00:56

Pull request was closed

@Hydrocharged Hydrocharged deleted the zachmu/foreign-key-text-field branch September 17, 2025 11:12
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