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

[VL] Explode support Literal array and map #4019

Merged
merged 3 commits into from
Dec 16, 2023
Merged

Conversation

liujiayi771
Copy link
Contributor

@liujiayi771 liujiayi771 commented Dec 12, 2023

What changes were proposed in this pull request?

After my tests, I have found that the Velox backend does support the explode function for Literal inputs of both array and map types. We can lift the restrictions on validation.

How was this patch tested?

Add a test case in TestOperator.

Copy link

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:

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

@liujiayi771 liujiayi771 marked this pull request as draft December 13, 2023 15:06
Copy link

Run Gluten Clickhouse CI

Copy link

Run Gluten Clickhouse CI

@liujiayi771 liujiayi771 marked this pull request as ready for review December 14, 2023 15:02
@liujiayi771 liujiayi771 changed the title [VL] Explode support array and map [VL] Explode support Literal array and map Dec 15, 2023
@liujiayi771
Copy link
Contributor Author

@zhouyuan Could you help to review?

Copy link
Contributor

@zhouyuan zhouyuan left a comment

Choose a reason for hiding this comment

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

@liujiayi771 thanks for helping on this. Just not sure if the nested type is considered.

unnestNames.emplace_back(fmt::format("C{}", unnestIndex++));
} else {
VELOX_FAIL(
"Unexpected type of unnest variable. Expected ARRAY or MAP, but got {}.", variable->type()->toString());
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can we also apply this check logic in native validation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

}
runQueryAndCompare("""
|SELECT explode(map(1, 'a', 2, 'b'));
|""".stripMargin) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: can you please also add a test for nested array/map? I'm not sure if this would work.
e.g. array(map(1, "2"))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it can be executed normally, and two nested test cases have been added.

Copy link
Contributor

@zhouyuan zhouyuan left a comment

Choose a reason for hiding this comment

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

👍

@zhouyuan zhouyuan merged commit 06a7a6a into apache:main Dec 16, 2023
15 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_4019_time.csv log/native_master_12_15_2023_dee511dbb_time.csv difference percentage
q1 32.93 34.66 1.730 105.25%
q2 26.83 25.04 -1.786 93.34%
q3 37.75 38.08 0.334 100.88%
q4 38.94 39.96 1.020 102.62%
q5 72.86 72.01 -0.852 98.83%
q6 7.08 6.34 -0.744 89.49%
q7 86.48 84.32 -2.160 97.50%
q8 87.06 85.64 -1.419 98.37%
q9 129.36 127.25 -2.119 98.36%
q10 45.75 46.05 0.300 100.66%
q11 20.04 19.81 -0.221 98.90%
q12 28.17 27.20 -0.962 96.58%
q13 47.11 46.12 -0.983 97.91%
q14 14.75 14.01 -0.734 95.02%
q15 29.58 28.88 -0.702 97.63%
q16 15.44 15.74 0.303 101.96%
q17 102.07 103.19 1.119 101.10%
q18 151.17 150.61 -0.558 99.63%
q19 12.71 13.91 1.205 109.48%
q20 27.60 27.33 -0.267 99.03%
q21 227.66 228.65 0.985 100.43%
q22 13.82 13.92 0.107 100.77%
total 1255.13 1248.73 -6.405 99.49%

@liujiayi771 liujiayi771 deleted the explode branch December 16, 2023 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants