Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[GLUTEN-6856][CH]Support arrays_overlap and fix array_join diff #6857
[GLUTEN-6856][CH]Support arrays_overlap and fix array_join diff #6857
Changes from 16 commits
71963b5
fe7d99d
bfd4e05
2f607c4
1f616dc
46ee8d4
f126f3c
f123ff5
b06f171
74253a7
b6c1902
a69a591
5b45900
25d2217
8b53605
a2a58c3
24e9a2d
afdc6de
b267a2c
d876737
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
这里设置成false的话,就必须自己处理arr1[i] = null或arr2[i] = null的问题了,增加了很多不必要的麻烦
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.
如果设置为true,ch 自己处理null 相关的逻辑,会导致两边的结果不一样。arrays_overlap里面如果有一个array含有null,就需要返回null。而ch里面,会返回true,因为两边同时含有NULL。
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.
上面写的不是很清楚,设置成true的话,ch会处理arr1 = null or arr2 = null的逻辑, 不影响其他case.
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.
已经设置为true
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.
why is it false? if it is false, we need to handle the case that all input arguments are constant, which could be avoided by setting it to true.
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.
if set to true, the input maybe const null, and clickhouse will replace this value , which would lead to differrent result
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.
为什么clickhouse会替换const null? 没太理解
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.
测试了下可以设置为true,代码已经修改。应该是之前的测试有问题
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.
尽量不要把所有的逻辑放在一个大函数里。参考src/Functions/regexpExtract.cpp的实现,按照两个参数是否为constant分成若干种情况,每种情况对应一个函数。
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.
const里包的有可能是nullable array或array, 两种情况都需要考虑
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.
arguments[0]也许是nullable array或array, 两种情况都需要考虑。arguments[1]也一样
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.
for循环中高频调用虚函数 isNullAt会影响性能。既然拿到了null_map_data这里为什么不用?
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.
此处是多余逻辑,已经删除
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.
两个for循环性能会比较差,可以用这个数据结构:
using Set = HashSetWithStackMemory<StringRef, StringRefHash, 4>;
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.
使用HashSetWithStackMemory,考虑到多层array,struct,map的复杂嵌套,会导致代码逻辑十分复杂,并且无法通过getDataAt(i) 从array(string) 的column中获取数据。不是一个通用的方法
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.
只是举个例子,如果上面的set不行还可以用std::unordered_set
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.
使用std::unordered_set 也是一样,对于复杂类型来说,需要判断是否有column 嵌套,然后再递归比较。相比较于两次for循环,可以直接使用ch 内置的column.compare 函数,直接在ch内部实现了复杂类型column的比较。使用set,也只是空间换时间。