Skip to content

Allow the option of using null values in vindex fields#4146

Merged
sougou merged 2 commits intovitessio:masterfrom
eeSeeGee:young.20180820.use_null_lookup
Sep 5, 2018
Merged

Allow the option of using null values in vindex fields#4146
sougou merged 2 commits intovitessio:masterfrom
eeSeeGee:young.20180820.use_null_lookup

Conversation

@eeSeeGee
Copy link
Copy Markdown
Contributor

No description provided.

@rafael
Copy link
Copy Markdown
Member

rafael commented Aug 24, 2018

This is a good idea!

I wanted to work on this at some point but never got a change to get to it. I was thinking that we don't need an extra option and we can relax the constraint entirely and let MySQL enforce it (i.e if you require some fields of the lookup table to not be null, you set it in the schema there).

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Aug 24, 2018

I like @rafael's suggestion. The check was previously there because the null value was previously used to figure out if a sequence number had to be generated. But the design has now changed and the sequence generation logic happens well before here. So, the null check is unnecessary.

A vindex could still choose to disallow nulls, but that will be up to the vindex implementor to make this decision. This means that we should revisit our vindexes to decide if we should allow them to accept nulls. I feel like lookup should not, but it's debatable.

TL;DR: Let's delete the null check altogether.

Signed-off-by: Aaron Young <young@squareup.com>
@eeSeeGee eeSeeGee force-pushed the young.20180820.use_null_lookup branch from 26ab342 to 9c1137c Compare August 26, 2018 18:34
@eeSeeGee
Copy link
Copy Markdown
Contributor Author

Cool, makes sense, done.

@sougou
Copy link
Copy Markdown
Contributor

sougou commented Aug 28, 2018

Tests need fixing.

Signed-off-by: Aaron Young <young@squareup.com>
@sougou sougou merged commit 032dd80 into vitessio:master Sep 5, 2018
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