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] Fix a bug that may break bhj + exchange validation in a corner case #3595

Merged
merged 1 commit into from
Nov 10, 2023

Conversation

zhztheplayer
Copy link
Member

@zhztheplayer zhztheplayer commented Nov 2, 2023

Previous bhj + exchange joint validation may not work because of bugs. The patch is to fix the broken logic.

Copy link

github-actions bot commented Nov 2, 2023

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

github-actions bot commented Nov 2, 2023

Run Gluten Clickhouse CI

@zhztheplayer
Copy link
Member Author

/Benchmark Velox

@GlutenPerfBot
Copy link
Contributor

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

query log/native_3595_time.csv log/native_master_11_01_2023_697b2bda2_time.csv difference percentage
q1 35.23 34.18 -1.057 97.00%
q2 25.65 24.84 -0.808 96.85%
q3 39.43 40.06 0.635 101.61%
q4 39.11 37.29 -1.827 95.33%
q5 70.07 71.36 1.292 101.84%
q6 7.13 8.95 1.822 125.56%
q7 86.31 86.23 -0.074 99.91%
q8 87.31 84.56 -2.753 96.85%
q9 121.47 121.30 -0.173 99.86%
q10 50.73 50.93 0.205 100.40%
q11 20.49 20.30 -0.183 99.11%
q12 28.60 26.50 -2.092 92.68%
q13 48.32 48.85 0.536 101.11%
q14 19.22 18.13 -1.089 94.33%
q15 33.79 35.06 1.274 103.77%
q16 16.42 16.16 -0.260 98.42%
q17 101.28 102.07 0.787 100.78%
q18 145.64 149.11 3.465 102.38%
q19 18.23 18.26 0.029 100.16%
q20 31.49 31.06 -0.428 98.64%
q21 223.58 223.74 0.156 100.07%
q22 13.41 13.21 -0.203 98.48%
total 1262.90 1262.16 -0.744 99.94%

Comment on lines 555 to +558
TransformHints.tag(bhj, isBhjTransformable.toTransformHint)
TransformHints.tagNotTransformable(exchange, isBhjTransformable)
if (!isBhjTransformable.isValid) {
TransformHints.tagNotTransformable(exchange, isBhjTransformable)
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Once exchange was already tagged as transformable, This code may still re-tag it as non-transformable if bhj is considered non-transformable in validation at the time.

Comment on lines +317 to +319
val out = plan.withNewChildren(plan.children.map(addTransformableTags))
addTransformableTag(out)
out
Copy link
Member Author

@zhztheplayer zhztheplayer Nov 3, 2023

Choose a reason for hiding this comment

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

This ensures bhj is validated after the corresponding exchange is validated

@zhztheplayer zhztheplayer changed the title WIP: [VL] Fix a bug that may break bhj + exchange validation in a corner case [VL] Fix a bug that may break bhj + exchange validation in a corner case Nov 10, 2023
@zhztheplayer zhztheplayer marked this pull request as ready for review November 10, 2023 02:26
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!

@zhztheplayer zhztheplayer merged commit 337b9d9 into apache:main Nov 10, 2023
18 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_3595_time.csv log/native_master_11_09_2023_745ecb383_time.csv difference percentage
q1 34.55 33.80 -0.754 97.82%
q2 25.35 24.95 -0.404 98.41%
q3 37.79 38.87 1.078 102.85%
q4 37.53 36.96 -0.567 98.49%
q5 71.01 69.94 -1.072 98.49%
q6 7.88 7.87 -0.013 99.84%
q7 83.99 85.40 1.417 101.69%
q8 87.03 88.61 1.583 101.82%
q9 120.62 121.14 0.516 100.43%
q10 52.17 54.35 2.177 104.17%
q11 20.15 19.52 -0.636 96.84%
q12 27.08 24.93 -2.157 92.03%
q13 49.94 50.92 0.984 101.97%
q14 16.68 18.17 1.483 108.89%
q15 31.16 30.79 -0.372 98.81%
q16 16.05 16.06 0.015 100.09%
q17 100.23 101.51 1.288 101.28%
q18 149.71 148.23 -1.477 99.01%
q19 14.82 14.95 0.130 100.88%
q20 29.85 31.24 1.389 104.65%
q21 224.26 223.30 -0.959 99.57%
q22 13.58 14.24 0.655 104.82%
total 1251.43 1255.74 4.304 100.34%

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.

3 participants