Skip to content
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 empty array handling in arraySplit #7747

Merged
merged 3 commits into from
Dec 3, 2019

Conversation

hczhcz
Copy link
Contributor

@hczhcz hczhcz commented Nov 13, 2019

I hereby agree to the terms of the CLA available at: https://yandex.ru/legal/cla/?lang=en

Changelog category (leave one):

  • Bug Fix

Changelog entry (up to few sentences, required except for Non-significant/Documentation categories):

Fix #7708 empty array handling in arraySplit

@hczhcz hczhcz changed the title [WIP] Fix empty array handling [WIP] Fix empty array handling in arraySplit Nov 13, 2019
@hczhcz hczhcz changed the title [WIP] Fix empty array handling in arraySplit Fix empty array handling in arraySplit Nov 13, 2019
@tavplubix tavplubix self-requested a review November 27, 2019 12:52
Copy link
Member

@tavplubix tavplubix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In case like SELECT arraySplit(x -> 0, []) column mapped is ColumnConst and mapped->size() is zero. However we access it's value in if (column_cut_const->getValue<UInt8>()) (line 68). It works because nested column actually has size 1 and ColumnConst just returns value of nested column without any checks, but it looks very suspicious.

@@ -6,8 +6,10 @@
[[1],[2],[3],[4],[5]]
[[1,2],[3,4],[5]]
[[1],[2,3],[4,5]]
[[]]
[[]]
[]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be it's better to return [[]] for empty array to make result more consistent with arraySplit(x -> 0, [1, 2, 3, 4, 5]), which returns [[1,2,3,4,5]] (array, which contains the source array)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for now.

@hczhcz
Copy link
Contributor Author

hczhcz commented Nov 29, 2019

@tavplubix
...->getValue<UInt8>() is used in many high-order functions (arrayFilter, arrayCount, arrayAll, etc).
It might be good to make developers aware the suspicious thing by adding some comments & docs.

@alexey-milovidov alexey-milovidov merged commit edede56 into ClickHouse:master Dec 3, 2019
@hczhcz hczhcz deleted the patch-1113 branch December 3, 2019 03:28
@nikitamikhaylov nikitamikhaylov added the pr-bugfix Pull request with bugfix, not backported by default label Dec 5, 2019
nikitamikhaylov pushed a commit that referenced this pull request Dec 5, 2019
Fix empty array handling in arraySplit

(cherry picked from commit edede56)
@nikitamikhaylov
Copy link
Member

Conflicts with 19.16

nikitamikhaylov pushed a commit that referenced this pull request Dec 5, 2019
Fix empty array handling in arraySplit

(cherry picked from commit edede56)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr-bugfix Pull request with bugfix, not backported by default
Projects
None yet
Development

Successfully merging this pull request may close these issues.

arraySplit usage lead to: Data compressed with different methods exception
5 participants