[GLUTEN-6856][CH]Support arrays_overlap and fix array_join diff #6857

KevinyhZou · 2024-08-15T03:45:38Z

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

(Fixes: #6856 #6916 )

support function arrays_overlap
fix array_join diff

How was this patch tested?

test by spark ut

(If this patch involves UI changes, please attach a screenshot; otherwise, remove this)

github-actions · 2024-08-15T03:45:56Z

#6856

github-actions · 2024-08-15T03:46:10Z

Run Gluten Clickhouse CI

github-actions · 2024-08-15T03:51:23Z

Run Gluten Clickhouse CI

github-actions · 2024-08-16T06:31:12Z

Run Gluten Clickhouse CI

github-actions · 2024-08-16T11:18:23Z

Run Gluten Clickhouse CI

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

github-actions · 2024-08-20T06:53:29Z

Run Gluten Clickhouse CI

github-actions · 2024-08-22T09:14:46Z

Run Gluten Clickhouse CI

KevinyhZou · 2024-08-22T09:16:13Z

The performance of arrays_overlap in gluten

The performance of arrays_overlap in valina

the performance compare: gluten 6.2s vs valina 37s

github-actions · 2024-08-28T02:15:33Z

Run Gluten Clickhouse CI

github-actions · 2024-09-19T02:13:55Z

Run Gluten Clickhouse CI

github-actions · 2024-09-19T02:16:31Z

Run Gluten Clickhouse CI

github-actions · 2024-09-19T03:37:27Z

Run Gluten Clickhouse CI

github-actions · 2024-09-19T06:24:43Z

Run Gluten Clickhouse CI

github-actions · 2024-09-19T10:14:54Z

Run Gluten Clickhouse CI

taiyang-li · 2024-09-20T01:58:40Z

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

+    size_t getNumberOfArguments() const override { return 2; }
+    String getName() const override { return name; }
+    bool useDefaultImplementationForNulls() const override { return false; }
+    bool useDefaultImplementationForConstants() const override { return false; }


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.

if set to true, the input maybe const null, and clickhouse will replace this value , which would lead to differrent result

为什么clickhouse会替换const null? 没太理解

测试了下可以设置为true，代码已经修改。应该是之前的测试有问题

taiyang-li · 2024-09-20T02:05:04Z

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

+            return ColumnNullable::create(std::move(res), std::move(null_map));
+        }
+        if (const_col_1)
+            array_col_1 = checkAndGetColumn<ColumnArray>(const_col_1->getDataColumnPtr().get());


const里包的有可能是nullable array或array, 两种情况都需要考虑

taiyang-li · 2024-09-20T02:06:25Z

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

+            array_col_1 = checkAndGetColumn<ColumnArray>(const_col_1->getDataColumnPtr().get());
+        else
+        {
+            const auto * null_col_1 = checkAndGetColumn<ColumnNullable>(arguments[0].column.get());


arguments[0]也许是nullable array或array, 两种情况都需要考虑。arguments[1]也一样

taiyang-li · 2024-09-20T02:12:44Z

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

+            size_t array_size_2 = array_offsets_2[i] - current_offset_2;
+            auto executeCompare = [&](const IColumn & col1, const IColumn & col2, const ColumnUInt8 * null_map1, const ColumnUInt8 * null_map2) -> void
+            {
+                for (size_t j = 0; j < array_size_1 && !res_data[i]; ++j)


两个for循环性能会比较差，可以用这个数据结构：
using Set = HashSetWithStackMemory<StringRef, StringRefHash, 4>;

使用HashSetWithStackMemory，考虑到多层array，struct，map的复杂嵌套，会导致代码逻辑十分复杂，并且无法通过getDataAt(i) 从array(string) 的column中获取数据。不是一个通用的方法

只是举个例子，如果上面的set不行还可以用std::unordered_set

使用std::unordered_set 也是一样，对于复杂类型来说，需要判断是否有column 嵌套，然后再递归比较。相比较于两次for循环，可以直接使用ch 内置的column.compare 函数，直接在ch内部实现了复杂类型column的比较。使用set，也只是空间换时间。

taiyang-li · 2024-09-20T02:14:44Z

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

+        return makeNullable(data_type);
+    }
+
+     ColumnPtr executeImpl(const ColumnsWithTypeAndName & arguments, const DataTypePtr &, size_t input_rows_count) const override


尽量不要把所有的逻辑放在一个大函数里。参考src/Functions/regexpExtract.cpp的实现，按照两个参数是否为constant分成若干种情况，每种情况对应一个函数。

taiyang-li · 2024-09-20T02:18:34Z

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

+    bool isSuitableForShortCircuitArgumentsExecution(const DataTypesWithConstInfo &) const override { return true; }
+    size_t getNumberOfArguments() const override { return 2; }
+    String getName() const override { return name; }
+    bool useDefaultImplementationForNulls() const override { return false; }


这里设置成false的话，就必须自己处理arr1[i] = null或arr2[i] = null的问题了，增加了很多不必要的麻烦

如果设置为true，ch 自己处理null 相关的逻辑，会导致两边的结果不一样。arrays_overlap里面如果有一个array含有null，就需要返回null。而ch里面，会返回true，因为两边同时含有NULL。

上面写的不是很清楚，设置成true的话，ch会处理arr1 = null or arr2 = null的逻辑, 不影响其他case.

已经设置为true

taiyang-li · 2024-09-20T02:20:24Z

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp

+        size_t array_pos_1 = 0, array_pos_2 = 0;
+        for (size_t i = 0; i < array_col_1->size(); ++i)
+        {
+            if (arguments[0].column->isNullAt(i) || arguments[1].column->isNullAt(i))


for循环中高频调用虚函数 isNullAt会影响性能。既然拿到了null_map_data这里为什么不用？

此处是多余逻辑，已经删除

github-actions · 2024-09-20T07:14:42Z

Run Gluten Clickhouse CI

github-actions · 2024-09-20T07:18:49Z

Run Gluten Clickhouse CI

github-actions · 2024-09-20T08:43:10Z

Run Gluten Clickhouse CI

github-actions · 2024-09-20T09:00:03Z

Run Gluten Clickhouse CI

taiyang-li

LGTM

…he#6857) * Fix orc timezone read * on test * fix ci * fix ci * ci build error * support arrays_overlap * remove useless code * fix ci * fix ci * fix array_join diff * remove useless code * remove useless code * solve conflict * ci fix * remove useless code * use default impl for array join * remove useless code

KevinyhZou added 8 commits August 9, 2024 15:14

Fix orc timezone read

71963b5

on test

fe7d99d

fix ci

bfd4e05

fix ci

2f607c4

ci build error

1f616dc

Merge branch 'main' of https://github.com/oap-project/gluten into main

46ee8d4

Merge branch 'main' of https://github.com/oap-project/gluten into main

f126f3c

support arrays_overlap

f123ff5

github-actions bot added CORE works for Gluten Core CLICKHOUSE labels Aug 15, 2024

KevinyhZou changed the title ~~[GLUTEN-6856][CH]Support arrays overlap~~ [GLUTEN-6856][CH]Support arrays_overlap Aug 15, 2024

remove useless code

b06f171

fix ci

74253a7

fix ci

b6c1902

KevinyhZou added 2 commits August 19, 2024 10:38

fix array_join diff

a69a591

remove useless code

5b45900

KevinyhZou changed the title ~~[GLUTEN-6856][CH]Support arrays_overlap~~ [GLUTEN-6856][CH]Support arrays_overlap and fix array_join diff Aug 19, 2024

remove useless code

25d2217

taiyang-li reviewed Aug 20, 2024

View reviewed changes

cpp-ch/local-engine/Functions/SparkFunctionArraysOverlap.cpp Show resolved Hide resolved

KevinyhZou force-pushed the support_arrays_overlap branch from 0039b63 to d4fd958 Compare August 22, 2024 09:14

KevinyhZou force-pushed the support_arrays_overlap branch from d2e4c2b to df7331a Compare September 19, 2024 02:13

github-actions bot removed the CORE works for Gluten Core label Sep 19, 2024

KevinyhZou added 2 commits September 19, 2024 16:31

Merge branch 'main' into support_arrays_overlap_bak

8b53605

solve conflict

a2a58c3

KevinyhZou force-pushed the support_arrays_overlap branch from d552874 to a2a58c3 Compare September 19, 2024 10:14

taiyang-li requested changes Sep 20, 2024

View reviewed changes

ci fix

24e9a2d

remove useless code

afdc6de

use default impl for array join

b267a2c

remove useless code

d876737

taiyang-li approved these changes Oct 9, 2024

View reviewed changes

taiyang-li merged commit 92d3793 into apache:main Oct 9, 2024
8 checks passed

[GLUTEN-6856][CH]Support arrays_overlap and fix array_join diff #6857

[GLUTEN-6856][CH]Support arrays_overlap and fix array_join diff #6857

Conversation

KevinyhZou commented Aug 15, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Aug 15, 2024

github-actions bot commented Aug 15, 2024

github-actions bot commented Aug 15, 2024

github-actions bot commented Aug 16, 2024

github-actions bot commented Aug 16, 2024

github-actions bot commented Aug 20, 2024

github-actions bot commented Aug 22, 2024

KevinyhZou commented Aug 22, 2024 • edited Loading

github-actions bot commented Aug 28, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

github-actions bot commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

taiyang-li Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinyhZou Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

KevinyhZou Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

github-actions bot commented Sep 20, 2024

taiyang-li left a comment

Choose a reason for hiding this comment

KevinyhZou commented Aug 15, 2024 •

edited

Loading

KevinyhZou commented Aug 22, 2024 •

edited

Loading

taiyang-li Sep 20, 2024 •

edited

Loading

KevinyhZou Sep 20, 2024 •

edited

Loading

KevinyhZou Sep 20, 2024 •

edited

Loading