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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-4830][VL] Support MapType substrait signature #4833

Merged
merged 3 commits into from
Mar 21, 2024
Merged

[GLUTEN-4830][VL] Support MapType substrait signature #4833

merged 3 commits into from
Mar 21, 2024

Conversation

WangGuangxin
Copy link
Contributor

What changes were proposed in this pull request?

Support MapType substrait signature

(Fixes: #4830)

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 1, 2024

#4830

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 1, 2024

Run Gluten Clickhouse CI

2 similar comments
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 1, 2024

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 1, 2024

Run Gluten Clickhouse CI

jinchengchenghh
jinchengchenghh reviewed Mar 2, 2024
View reviewed changes
cpp/velox/substrait/VeloxSubstraitSignature.cc Outdated
@@ -182,6 +183,43 @@ TypePtr VeloxSubstraitSignature::fromSubstraitSignature(const std::string& signa
return ARRAY(elementType);
}

if (startWith(signature, "map")) {
// Map type name is in the format of map<T1,T2>.
auto mapStart = signature.find_first_of('<');
Copy link
Contributor

Choose a reason for hiding this comment

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

There are so much code which copied from struct type, can we extract them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 4, 2024

Run Gluten Clickhouse CI

@WangGuangxin WangGuangxin force-pushed the feat_support_maptype branch from 508431a to 465baf4 Compare March 5, 2024 00:22
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 5, 2024

Run Gluten Clickhouse CI

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 5, 2024

Run Gluten Clickhouse CI

@WangGuangxin WangGuangxin force-pushed the feat_support_maptype branch from a678422 to 8cae041 Compare March 7, 2024 00:07
@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 7, 2024

Run Gluten Clickhouse CI

@WangGuangxin WangGuangxin force-pushed the feat_support_maptype branch from 8cae041 to 5e71836 Compare March 18, 2024 15:45
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@WangGuangxin WangGuangxin force-pushed the feat_support_maptype branch from 5e71836 to d841c12 Compare March 18, 2024 15:47
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@WangGuangxin
Copy link
Contributor Author

@jinchengchenghh @PHILO-HE @rui-mo Can you please help review this?

rui-mo
rui-mo reviewed Mar 19, 2024
View reviewed changes
Copy link
Contributor

@rui-mo rui-mo left a comment

Choose a reason for hiding this comment

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

Thanks. Looks good to me.

PHILO-HE
PHILO-HE reviewed Mar 19, 2024
View reviewed changes
Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

@WangGuangxin, thanks for your work! Just one small refactor suggestion for list type. Please check whether it makes sense. Thanks!

cpp/velox/substrait/VeloxSubstraitSignature.cc
}
return MAP(std::move(types)[0], std::move(types)[1]);
}

if (startWith(signature, "list")) {
auto listStart = signature.find_first_of('<');
Copy link
Contributor

Choose a reason for hiding this comment

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

@WangGuangxin, it seems we can also use parseNestedTypeSignature to handle list type. Right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems that the parseNestedTypeSignature can't properly handle list type, since list type's child doesn't have comma, so for types like list<map<str,str>> it will not work

Copy link
Contributor

Choose a reason for hiding this comment

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

@WangGuangxin, I see. Thanks!

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@PHILO-HE
Copy link
Contributor

@WangGuangxin, CI reported the below failure.

- Support Map type signature *** FAILED ***
  org.apache.spark.SparkException: Job aborted due to stage failure: Task 0 in stage 1062.0 failed 1 times, most recent failure: Lost task 0.0 in stage 1062.0 (TID 817) (2150020d79e2 executor driver): java.lang.NullPointerException
	at org.apache.spark.sql.catalyst.expressions.aggregate.Collect.deserialize(collect.scala:83)
	at org.apache.spark.sql.catalyst.expressions.aggregate.Collect.deserialize(collect.scala:37)
	at org.apache.spark.sql.catalyst.expressions.aggregate.TypedImperativeAggregate.merge(interfaces.scala:589)
	at org.apache.spark.sql.execution.aggregate.AggregationIterator$$anonfun$1.$anonfun$applyOrElse$3(AggregationIterator.scala:200)
	at org.apache.spark.sql.execution.aggregate.AggregationIterator$$anonfun$1.$anonfun$applyOrElse$3$adapted(AggregationIterator.scala:200)
	at

@WangGuangxin
Revert "address comments"
74a3d96 
This reverts commit 8545826.
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

PHILO-HE
PHILO-HE approved these changes Mar 21, 2024
View reviewed changes
Copy link
Contributor

@PHILO-HE PHILO-HE left a comment

Choose a reason for hiding this comment

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

Thanks!

@PHILO-HE PHILO-HE merged commit 998bce1 into apache:main Mar 21, 2024
17 of 20 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_4833_time.csv log/native_master_03_20_2024_e1f0c01a7_time.csv difference percentage
q1 36.32 38.53 2.209 106.08%
q2 24.20 24.00 -0.197 99.19%
q3 37.00 37.50 0.498 101.35%
q4 38.39 38.61 0.215 100.56%
q5 68.13 71.63 3.497 105.13%
q6 7.34 7.37 0.029 100.39%
q7 85.07 83.96 -1.112 98.69%
q8 84.79 84.46 -0.330 99.61%
q9 121.45 125.15 3.692 103.04%
q10 43.42 45.58 2.157 104.97%
q11 20.39 20.52 0.130 100.64%
q12 28.56 28.80 0.242 100.85%
q13 47.73 48.70 0.970 102.03%
q14 20.15 22.25 2.098 110.41%
q15 32.70 31.14 -1.555 95.25%
q16 13.79 15.42 1.631 111.83%
q17 101.54 102.47 0.927 100.91%
q18 145.75 143.60 -2.152 98.52%
q19 14.18 13.64 -0.541 96.19%
q20 31.84 27.02 -4.816 84.87%
q21 225.62 225.52 -0.098 99.96%
q22 14.12 13.96 -0.157 98.89%
total 1242.47 1249.81 7.339 100.59%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jinchengchenghh jinchengchenghh jinchengchenghh left review comments

@rui-mo rui-mo rui-mo approved these changes

@PHILO-HE PHILO-HE PHILO-HE approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[VL] Support MapType signature
5 participants
@WangGuangxin @PHILO-HE @GlutenPerfBot @rui-mo @jinchengchenghh