Skip to content

Allow updating fields to null#4187

Merged
sougou merged 1 commit intovitessio:masterfrom
eeSeeGee:young.20180907.update_null
Sep 8, 2018
Merged

Allow updating fields to null#4187
sougou merged 1 commit intovitessio:masterfrom
eeSeeGee:young.20180907.update_null

Conversation

@eeSeeGee
Copy link
Copy Markdown
Contributor

@eeSeeGee eeSeeGee commented Sep 7, 2018

Signed-off-by: Aaron Young aaron.young@gmail.com

Signed-off-by: Aaron Young <aaron.young@gmail.com>
@eeSeeGee eeSeeGee force-pushed the young.20180907.update_null branch from e520b79 to e9b91d0 Compare September 8, 2018 12:37
@sougou
Copy link
Copy Markdown
Contributor

sougou commented Sep 8, 2018

It just occurred to me that we need to do more work on this. This is because the lookup query will end up generating where col = null, which will always be false. Also, inserting null in a pk will fail.

What we can do is convert nulls to 0 (in lookup_internal.go). In the case of a string key, it will get treated as "0", which should just work. However, this means that we can't allow a null and 0 for a unique lookup.

In other words, there's no clean way to support null as first class value, but the above solution seems to be a decent compromise.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Sep 8, 2018

The null issue was caused by the previous merge and not this one. So, I'll merge this, and we can fix it in a new PR.

@sougou sougou merged commit 684e132 into vitessio:master Sep 8, 2018
@tirsen
Copy link
Copy Markdown
Collaborator

tirsen commented Sep 10, 2018

Why don't we just generate where col is null?

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Sep 16, 2018

This was what I was going to originally propose until I realized that inserts with null values will anyway fail because of primary key constraints.

@eeSeeGee
Copy link
Copy Markdown
Contributor Author

The thing about null primary keys is they don't really matter for our use case because we use Hibernate. I'm also kind of of the mind that if you allow nulls in your primary key you deserve all the trouble you're asking for :)

@eeSeeGee eeSeeGee deleted the young.20180907.update_null branch September 19, 2018 19:40
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.

3 participants