Skip to content

add a flag to indicate if a vindex handles null value#6425

Merged
sougou merged 3 commits intovitessio:masterfrom
hihihuhu:handle_null
Jul 9, 2020
Merged

add a flag to indicate if a vindex handles null value#6425
sougou merged 3 commits intovitessio:masterfrom
hihihuhu:handle_null

Conversation

@hihihuhu
Copy link
Copy Markdown

@hihihuhu hihihuhu commented Jul 8, 2020

there is a recent change to improve is null query #6133, it changed the behavior from always to fallback to scatter gather for null query, to treat null as an value and handle it in vindex.
however, not all vindexes could handle null value, especially, the lookup table based vindex doesn't store null value at all, so add a new flag to indicate if the vindex could handle null and fallback to scatter gather for null query if the vindex doesn't support it.

@hihihuhu hihihuhu requested a review from sougou as a code owner July 8, 2020 18:14
Signed-off-by: Chen Ding <chen.ding@airbnb.com>
@harshit-gangal
Copy link
Copy Markdown
Member

We already have function which does similar check directly use NeedsVCursor instead of HasNullTest

@hihihuhu
Copy link
Copy Markdown
Author

hihihuhu commented Jul 8, 2020

thanks for pointing out! do you have a printer about the NeedsVCursor check? does it mean the issue of null query against lookup vindex always return empty result is fixed already

@harshit-gangal
Copy link
Copy Markdown
Member

harshit-gangal commented Jul 8, 2020

Inserting null value into lookup does not give any value. You can update your code to use NeedVCursor

@hihihuhu
Copy link
Copy Markdown
Author

hihihuhu commented Jul 8, 2020

yes, this is not about inserting null value into lookup table, it is about query like select * from table where col is null, since lookup table doesn't have null value, the query needs to be a scatter gather, which is the behavior before the pr in the comment above.

@hihihuhu
Copy link
Copy Markdown
Author

hihihuhu commented Jul 8, 2020

oh, i see your comment, yes, i could change to use NeedVCursor, however, just feel NeedVCursor may not necessary equal to cannot handle null value, though they could be the same thing for all exist vindexes

@hihihuhu
Copy link
Copy Markdown
Author

hihihuhu commented Jul 8, 2020

updated to use NeedVCursor

Signed-off-by: Chen Ding <chen.ding@airbnb.com>
@sougou
Copy link
Copy Markdown
Contributor

sougou commented Jul 9, 2020

I recently added support for "ignore_nulls" as an option for lookups here #6222. Will it be better if we just returned the full keyrange from the lookup vindex if this flag was set?

The other question is: did you want this for a unique or non-unique lookup?

One risk about a unique lookup is that there are places where we assume that no more than one shard will be a target. So, I'm concerned about how that interpretation may play out with a change like this.

Having said that, there is one rare (and intended) situation where a unique lookup does return more than one shard: https://github.com/vitessio/vitess/blob/master/go/vt/vtgate/vindexes/lookup.go#L227-L232. But the use case is acceptable because it happens while a lookup is being back-filled.

In other words, will it be ok if we changed this where a non-unique lookup returned a full scatter if input was null and ignore_nulls was set?

Signed-off-by: Chen Ding <chen.ding@airbnb.com>
@sougou sougou merged commit 251fb04 into vitessio:master Jul 9, 2020
@deepthi deepthi added this to the v7.0 milestone Jul 27, 2020
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.

5 participants