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] support spark nanvl function #4446

Merged
merged 2 commits into from
Jan 19, 2024
Merged

Conversation

zhli1142015
Copy link
Contributor

What changes were proposed in this pull request?

(Please fill in changes proposed in this fix)

(Fixes: #ISSUE-ID)

How was this patch tested?

(Please explain how this patch was tested. E.g. unit tests, integration tests, manual tests)

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

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

@zhli1142015 zhli1142015 marked this pull request as ready for review January 18, 2024 23:18
@zhli1142015
Copy link
Contributor Author

cc @rui-mo , thanks

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!

@@ -236,6 +236,7 @@ object ExpressionMappings {
Sig[CheckOverflow](CHECK_OVERFLOW),
Sig[MakeDecimal](MAKE_DECIMAL),
Sig[PromotePrecision](PROMOTE_PRECISION),
Sig[NaNvl](NANVL),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this mapping if we replace nanvl with if and isnan?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add this func to mappings for velox only now. Thanks.

@@ -253,6 +253,7 @@ object ExpressionNames {
final val CHECK_OVERFLOW = "check_overflow"
final val MAKE_DECIMAL = "make_decimal"
final val PROMOTE_PRECISION = "promote_precision"
final val NANVL = "nanvl"
Copy link
Contributor

Choose a reason for hiding this comment

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

Same.

replaceWithExpressionTransformerInternal(n.right, attributeSeq, expressionsMap),
replaceWithExpressionTransformerInternal(n.left, attributeSeq, expressionsMap),
n
)
Copy link
Contributor

@rui-mo rui-mo Jan 19, 2024

Choose a reason for hiding this comment

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

Does CH backend need this replacement? cc @zzcclp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know, but let me to limit this replacement for velox only now, thanks.

Copy link

Run Gluten Clickhouse CI

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!

@rui-mo rui-mo requested a review from zzcclp January 19, 2024 08:56
@zhli1142015 zhli1142015 merged commit 55e21df into apache:main Jan 19, 2024
19 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_4446_time.csv log/native_master_01_18_2024_7c853c61f_time.csv difference percentage
q1 32.92 31.92 -0.992 96.99%
q2 25.70 25.94 0.238 100.92%
q3 34.92 36.00 1.077 103.08%
q4 37.62 39.52 1.895 105.04%
q5 68.36 68.31 -0.055 99.92%
q6 7.09 5.33 -1.753 75.26%
q7 83.59 83.20 -0.395 99.53%
q8 82.60 84.62 2.021 102.45%
q9 124.22 123.96 -0.256 99.79%
q10 43.08 42.82 -0.264 99.39%
q11 19.78 20.16 0.382 101.93%
q12 27.42 26.88 -0.543 98.02%
q13 46.84 44.35 -2.482 94.70%
q14 20.80 16.22 -4.580 77.98%
q15 28.62 28.70 0.082 100.29%
q16 14.27 13.92 -0.345 97.58%
q17 99.46 99.40 -0.064 99.94%
q18 145.97 145.71 -0.256 99.82%
q19 12.38 12.60 0.224 101.81%
q20 27.24 26.33 -0.909 96.66%
q21 222.72 225.76 3.039 101.36%
q22 13.44 13.59 0.152 101.13%
total 1219.03 1215.24 -3.786 99.69%

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