-
Notifications
You must be signed in to change notification settings - Fork 6.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix has()
function with Nullable
column
#61249
Conversation
Previous implementation didn't check for `null_map` when the given argument was not `Null`. This commit adds the missing check. Fixes ClickHouse#60214
This is an automated comment for commit 9da0360 with description of existing statuses. It's updated for the latest CI running ⏳ Click here to open a full report in a separate page Successful checks
|
DROP TABLE IF EXISTS 00662_has_nullable; | ||
CREATE TABLE 00662_has_nullable(a Nullable(UInt64)) ENGINE = Memory; | ||
|
||
INSERT INTO 00662_has_nullable VALUES (1), (Null); | ||
SELECT a, has([0, 1], a) FROM 00662_has_nullable; | ||
|
||
DROP TABLE 00662_has_nullable; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would be also good to add more tests cases (more values in nullable table, put nullable arguments as left/both arguments in has()
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yariks5s also JFYI the cases you described actually exist in https://github.com/ClickHouse/ClickHouse/blob/master/tests/queries/0_stateless/00662_array_has_nullable.sql
I was thinking about expanding it instead of writing the new file, but that test didn't have any table creation, hence I decided to create a new one.
Previous implementation didn't check for
null_map
when the given argument was notNull
. This commit adds the missing check.Fixes #60214
Changelog category (leave one):
Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):
Fix
has()
function withNullable
column (fixes #60214)Documentation entry for user-facing changes