[GLUTEN-4772][VL] Support empty map/array literal #4771

WangGuangxin · 2024-02-25T13:30:49Z

What changes were proposed in this pull request?

Support array or map literal without value.
For example

create table map_table(a map<bigint, string>) using parquet;
insert into table map_table select map(1, 'hello');
select size(coalesce(a, map())) from map_table;

How was this patch tested?

Added UT

github-actions · 2024-02-25T13:31:09Z

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/oap-project/gluten/issues

Then could you also rename commit message and pull request title in the following format?

[GLUTEN-${ISSUES_ID}][COMPONENT]feat/fix: ${detailed message}

See also:

Other pull requests

github-actions · 2024-02-25T13:31:25Z

Run Gluten Clickhouse CI

github-actions · 2024-02-25T13:40:50Z

Run Gluten Clickhouse CI

github-actions · 2024-02-25T13:43:09Z

#4772

github-actions · 2024-02-26T01:46:37Z

Run Gluten Clickhouse CI

WangGuangxin · 2024-02-26T03:52:27Z

@rui-mo @ulysses-you

ulysses-you · 2024-02-26T04:31:13Z

it seems the CH backend failed test is related.

10:50:44  - test literals *** FAILED ***
10:50:44    org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 1285.0 failed 1 times, most recent failure: Lost task 0.0 in stage 1285.0 (TID 2287) (gluten-gluten-ci-7197-7bl43-bdv22-rrs5h executor driver): io.glutenproject.exception.GlutenException: Unsupported spark literal type kEmptyList
10:50:44  0. /home/jenkins/agent/workspace/gluten/gluten-ci/gluten/cpp-ch/build/../ClickHouse/src/Common/Exception.cpp:96: DB::Exception::Exception(DB::Exception::MessageMasked&&, int, bool) @ 0x000000000b8320fb in /home/jenkins/agent/workspace/gluten/gluten-ci/gluten/cpp-ch/build/utils/extern-local-engine/libch.so
10:50:44  1. DB::Exception::Exception<std::basic_string_view<char, std::char_traits<char>>>(int, FormatStringHelperImpl<std::type_identity<std::basic_string_view<char, std::char_traits<char>>>::type>, std::basic_string_view<char, std::char_traits<char>>&&) @ 0x00000000060cf590 in /home/jenkins/agent/workspace/gluten/gluten-ci/gluten/cpp-ch/build/utils/extern-local-engine/libch.so
10:50:44  2. /home/jenkins/agent/workspace/gluten/gluten-ci/gluten/cpp-ch/build/../local-engine/Parser/SerializedPlanParser.cpp:1537: local_engine::SerializedPlanParser::parseLiteral(substrait::Expression_Literal const&) @ 0x000000000bb58a9f in /home/jenkins/agent/workspace/gluten/gluten-ci/gluten/cpp-ch/build/utils/extern-local-engine/libch.so

ulysses-you · 2024-02-26T05:57:38Z

gluten-core/src/main/java/io/glutenproject/substrait/expression/ListLiteralNode.java

+      literalBuilder.setList(listBuilder.build());
+    } else {
+      Type.List.Builder listTypeBuilder = Type.List.newBuilder();
+      listTypeBuilder.setType(elementType.toProtobuf());


if if element type is NullType, we will still fallback it right ? e.g., select array();

It's related to #2996. It will not fallback once NullType is supported

rui-mo · 2024-02-26T07:18:57Z

cpp/velox/substrait/SubstraitToVeloxExpr.cc

@@ -382,7 +393,8 @@ std::shared_ptr<const core::ConstantTypedExpr> SubstraitVeloxExprConverter::toVe
 ArrayVectorPtr SubstraitVeloxExprConverter::literalsToArrayVector(const ::substrait::Expression::Literal& literal) {
  auto childSize = literal.list().values().size();
  if (childSize == 0) {
-    return makeEmptyArrayVector(pool_);
+    // childSize == 0 should not happend here but just for integrity check
+    return makeEmptyArrayVector(pool_, UNKNOWN());


Maybe it's better to throw for unexpected behavior.

+1, if the child size is 0 that means it is a empty list, we should not go into this method. @WangGuangxin can you address this comment ?

rui-mo · 2024-02-26T07:19:05Z

cpp/velox/substrait/SubstraitToVeloxExpr.cc

@@ -393,7 +405,8 @@ ArrayVectorPtr SubstraitVeloxExprConverter::literalsToArrayVector(const ::substr
 MapVectorPtr SubstraitVeloxExprConverter::literalsToMapVector(const ::substrait::Expression::Literal& literal) {
  auto childSize = literal.map().key_values().size();
  if (childSize == 0) {
-    return makeEmptyMapVector(pool_);
+    // childSize == 0 should not happend here but just for integrity check
+    return makeEmptyMapVector(pool_, UNKNOWN(), UNKNOWN());


rui-mo · 2024-02-26T07:19:29Z

cpp/velox/substrait/SubstraitToVeloxExpr.cc

@@ -497,6 +510,7 @@ RowVectorPtr SubstraitVeloxExprConverter::literalsToRowVector(const ::substrait:
        vectors.emplace_back(literalsToMapVector(child));
        break;
      }
+


nit: remove this empty line?

github-actions · 2024-02-26T16:14:01Z

Run Gluten Clickhouse CI

github-actions · 2024-02-27T14:14:32Z

Run Gluten Clickhouse CI

github-actions · 2024-02-28T00:08:33Z

Run Gluten Clickhouse CI

ulysses-you · 2024-03-01T04:18:21Z

@WangGuangxin can you take a look at ch backend test ? it seems still failed

WangGuangxin · 2024-03-01T05:31:23Z

@WangGuangxin can you take a look at ch backend test ? it seems still failed

@ulysses-you sure. you mean this one ?

I cannot login it since it requires username and password.

Can you paste the failed Test?

ulysses-you · 2024-03-01T05:35:02Z

@WangGuangxin there is a public read-only account
https://github.com/oap-project/gluten/blob/main/docs/get-started/ClickHouse.md#new-ci-system

github-actions · 2024-03-02T01:43:45Z

Run Gluten Clickhouse CI

This reverts commit 61fd55b.

github-actions · 2024-03-02T02:18:47Z

Run Gluten Clickhouse CI

github-actions · 2024-03-02T11:53:13Z

Run Gluten Clickhouse CI

github-actions · 2024-03-02T12:02:59Z

Run Gluten Clickhouse CI

github-actions · 2024-03-02T13:01:01Z

Run Gluten Clickhouse CI

WangGuangxin · 2024-03-04T01:41:27Z

It's ok now, cc @ulysses-you @rui-mo

ulysses-you · 2024-03-04T02:01:22Z

backends-velox/src/test/scala/io/glutenproject/execution/VeloxLiteralSuite.scala

-    validateFallbackResult("SELECT array(map())")
-    validateFallbackResult("SELECT map()")
-    validateFallbackResult("SELECT map(1, array())")
-    validateFallbackResult("SELECT map(1, map())")
    validateFallbackResult("SELECT array(null)")


Do you know why does it fallback ?

yes, it's not related to empty literal, but NullType support.
I'll fix it in another PR, otherwise this PR is too big.

ulysses-you · 2024-03-04T02:11:06Z

@WangGuangxin can you resolve this comment ? #4771 (comment)

WangGuangxin · 2024-03-04T02:21:13Z

@WangGuangxin can you resolve this comment ? #4771 (comment)

ok

zhouyuan · 2024-03-04T02:21:42Z

CC: @PHILO-HE

github-actions · 2024-03-04T13:59:27Z

Run Gluten Clickhouse CI

rui-mo

If CI passes.

WangGuangxin · 2024-03-05T02:02:20Z

There is a failed test VeloxSubstraitRoundTripTest.arrayLiteral and I'll fix it

github-actions · 2024-03-05T11:04:55Z

Run Gluten Clickhouse CI

github-actions · 2024-03-05T11:07:55Z

Run Gluten Clickhouse CI

github-actions · 2024-03-05T11:10:27Z

Run Gluten Clickhouse CI

WangGuangxin · 2024-03-05T13:52:44Z

issues resolved @ulysses-you @rui-mo

rui-mo

Thanks!

GlutenPerfBot · 2024-03-06T03:20:44Z

===== Performance report for TPCH SF2000 with Velox backend, for reference only ====

query	log/native_4771_time.csv	log/native_master_03_06_2024_10910e900_time.csv	difference	percentage
q1	34.77	35.69	0.920	102.65%
q2	26.27	24.09	-2.178	91.71%
q3	37.54	37.17	-0.367	99.02%
q4	38.11	37.31	-0.800	97.90%
q5	71.46	70.29	-1.162	98.37%
q6	7.16	7.07	-0.087	98.79%
q7	84.52	84.52	-0.001	100.00%
q8	87.11	86.24	-0.877	98.99%
q9	124.63	125.77	1.144	100.92%
q10	44.26	42.91	-1.349	96.95%
q11	20.77	20.70	-0.072	99.65%
q12	29.19	29.30	0.106	100.36%
q13	45.88	45.63	-0.251	99.45%
q14	15.59	20.55	4.957	131.79%
q15	28.78	29.07	0.291	101.01%
q16	12.73	13.92	1.193	109.37%
q17	104.63	102.68	-1.952	98.13%
q18	144.11	145.05	0.938	100.65%
q19	12.67	12.61	-0.065	99.49%
q20	27.58	25.67	-1.908	93.08%
q21	226.00	224.29	-1.708	99.24%
q22	15.05	13.74	-1.306	91.32%
total	1238.81	1234.28	-4.534	99.63%

WangGuangxin changed the title ~~support empty map/array literal~~ [VL] support empty map/array literal Feb 25, 2024

WangGuangxin changed the title ~~[VL] support empty map/array literal~~ [GLUTEN-4772][VL] Support empty map/array literal Feb 25, 2024

ulysses-you reviewed Feb 26, 2024

View reviewed changes

rui-mo reviewed Feb 26, 2024

View reviewed changes

WangGuangxin force-pushed the fix_array_map_literal branch from cb8a1cb to 512767f Compare February 28, 2024 00:07

WangGuangxin added 7 commits March 2, 2024 10:18

support empty map/array literal

e7ed77d

fix style

32053b0

fix typo

a7cff81

fix ut

e218a77

address comments

112fedb

Revert "address comments"

2e3889e

This reverts commit 61fd55b.

fix ch ci

5b83029

WangGuangxin force-pushed the fix_array_map_literal branch from d258684 to 5b83029 Compare March 2, 2024 02:18

fix

5800076

fix

4d5bda4

fix nest

e7efd20

ulysses-you reviewed Mar 4, 2024

View reviewed changes

throw exception when there are no values

3a28733

rui-mo previously approved these changes Mar 5, 2024

View reviewed changes

fix ut

5bdd9fa

WangGuangxin dismissed rui-mo’s stale review via 5bdd9fa March 5, 2024 11:04

fix style

2c630ae

fix style

28dc0ef

rui-mo approved these changes Mar 6, 2024

View reviewed changes

ulysses-you approved these changes Mar 6, 2024

View reviewed changes

ulysses-you merged commit c2b0805 into apache:main Mar 6, 2024
20 checks passed

taiyang-li pushed a commit to bigo-sg/gluten that referenced this pull request Mar 25, 2024

[GLUTEN-4772][VL] Support empty map/array literal (apache#4771)

cd7f8df

WangGuangxin added a commit to WangGuangxin/gluten that referenced this pull request May 14, 2024

[GLUTEN-4772][VL] Support empty map/array literal (apache#4771)

8f09545

[GLUTEN-4772][VL] Support empty map/array literal #4771

[GLUTEN-4772][VL] Support empty map/array literal #4771

Conversation

WangGuangxin commented Feb 25, 2024 • edited Loading

What changes were proposed in this pull request?

How was this patch tested?

github-actions bot commented Feb 25, 2024

github-actions bot commented Feb 25, 2024

github-actions bot commented Feb 25, 2024

github-actions bot commented Feb 25, 2024

github-actions bot commented Feb 26, 2024

WangGuangxin commented Feb 26, 2024

ulysses-you commented Feb 26, 2024

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 Feb 26, 2024

github-actions bot commented Feb 27, 2024

github-actions bot commented Feb 28, 2024

ulysses-you commented Mar 1, 2024

WangGuangxin commented Mar 1, 2024

ulysses-you commented Mar 1, 2024

github-actions bot commented Mar 2, 2024

github-actions bot commented Mar 2, 2024

github-actions bot commented Mar 2, 2024

github-actions bot commented Mar 2, 2024

github-actions bot commented Mar 2, 2024

WangGuangxin commented Mar 4, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ulysses-you commented Mar 4, 2024

WangGuangxin commented Mar 4, 2024

zhouyuan commented Mar 4, 2024

github-actions bot commented Mar 4, 2024

rui-mo left a comment

Choose a reason for hiding this comment

WangGuangxin commented Mar 5, 2024

github-actions bot commented Mar 5, 2024

github-actions bot commented Mar 5, 2024

github-actions bot commented Mar 5, 2024

WangGuangxin commented Mar 5, 2024

rui-mo left a comment

Choose a reason for hiding this comment

GlutenPerfBot commented Mar 6, 2024

WangGuangxin commented Feb 25, 2024 •

edited

Loading