Skip to content

vindexes: ignore_nulls option for lookups#6222

Merged
harshit-gangal merged 1 commit intovitessio:masterfrom
planetscale:ss-ignore-nulls
May 26, 2020
Merged

vindexes: ignore_nulls option for lookups#6222
harshit-gangal merged 1 commit intovitessio:masterfrom
planetscale:ss-ignore-nulls

Conversation

@sougou
Copy link
Copy Markdown
Contributor

@sougou sougou commented May 26, 2020

Fixes #6147

Signed-off-by: Sugu Sougoumarane ssougou@gmail.com

Fixes vitessio#6147

Signed-off-by: Sugu Sougoumarane <ssougou@gmail.com>
@harshit-gangal harshit-gangal merged commit 9b6f5d4 into vitessio:master May 26, 2020
@yborovikov
Copy link
Copy Markdown

@sougou, @harshit-gangal - thank you!

i'm not very familiar with vitess internal flow, yet, looking the method names in lookup_internal.go, i got a question:

does this change cover the case when an existing non-null (key) column gets updated to a null value?

i'd imagine a change to Update method would be needed (or a change in some other file to invoke Delete instead).

@sougou sougou deleted the ss-ignore-nulls branch May 26, 2020 06:08
@yborovikov
Copy link
Copy Markdown

yborovikov commented May 26, 2020

fyi: our existing test case (which inserts rows with null lookup-indexed column values) is now getting:
MySql.Data.MySqlClient.MySqlException : vtgate: http://somehost:someport/: execInsertSharded: getInsertShardedRoute: lookup.Create: input has null values: row: 0, col: 1
(without adding an ignore_nulls modifier to the lookup table or any other changes - just the fresh docker image) - which is probably (please forgive me if i'm wrong) not the expected behavior for existing customers.

works fine with ignore_nulls: "true".

@yborovikov
Copy link
Copy Markdown

also, it might be worth changing the ignore_nulls parameter type to a boolean ;)

@sougou
Copy link
Copy Markdown
Contributor Author

sougou commented May 26, 2020

does this change cover the case when an existing non-null (key) column gets updated to a null value?

This should be fine because Update is implemented as a Delete->Create. This is because the lookup row can move across shards on an update: https://github.com/vitessio/vitess/blob/master/go/vt/vtgate/vindexes/lookup_internal.go#L279-L284

(without adding an ignore_nulls modifier to the lookup table or any other changes - just the fresh docker image) - which is probably (please forgive me if i'm wrong) not the expected behavior for existing customers.

This is a breaking change. But I suspect many users may actually be misled because they expect the wrong behavior. Them switching to use ignore_nulls explicitly will then work as expected.

also, it might be worth changing the ignore_nulls parameter type to a boolean ;)

This is a limitation of the vindex design. It only accepts strings. as input values.

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.

RFC: add "ignore_nulls" option to lookup vindexes

5 participants