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

[CORE] Remove a redundant is-adaptive check #4618

Merged
merged 3 commits into from
Feb 4, 2024

Conversation

zhztheplayer
Copy link
Member

@zhztheplayer zhztheplayer commented Feb 2, 2024

We have a check on isAdaptive when replacing reusable (broadcast) sub-queries in the case of AQE is on.

case a: AdaptiveSparkPlanExec if (isAdaptiveContext && conf.subqueryReuseEnabled) =>
  // cache the subquery

It's an attempt to replace the code with

case a: AdaptiveSparkPlanExec if (conf.subqueryReuseEnabled) =>
  // cache the subquery

Since ideally case a: AdaptiveSparkPlanExec should imply isAdaptiveContext == true.

Related to #1851

Copy link

github-actions bot commented Feb 2, 2024

Run Gluten Clickhouse CI

1 similar comment
Copy link

github-actions bot commented Feb 2, 2024

Run Gluten Clickhouse CI

@zhztheplayer zhztheplayer changed the title PoC: [CORE] Avoid checking is-adaptive state when finding reusable sub-queries [CORE] Avoid checking is-adaptive state when finding reusable sub-queries Feb 2, 2024
@apache apache deleted a comment from github-actions bot Feb 2, 2024
Copy link

github-actions bot commented Feb 2, 2024

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_4618_time.csv log/native_master_01_31_2024_f1ed68005_time.csv difference percentage
q1 33.95 32.75 -1.191 96.49%
q2 24.20 24.25 0.050 100.21%
q3 37.72 37.43 -0.286 99.24%
q4 37.40 37.40 0.003 100.01%
q5 69.68 70.15 0.473 100.68%
q6 8.32 7.00 -1.321 84.12%
q7 83.62 83.66 0.043 100.05%
q8 84.61 85.87 1.256 101.48%
q9 120.86 124.41 3.547 102.94%
q10 44.20 43.68 -0.523 98.82%
q11 20.56 20.32 -0.241 98.83%
q12 25.14 26.52 1.381 105.49%
q13 45.02 45.62 0.599 101.33%
q14 21.88 19.16 -2.719 87.57%
q15 27.15 28.85 1.706 106.28%
q16 14.05 14.00 -0.051 99.64%
q17 100.84 102.02 1.174 101.16%
q18 148.22 149.84 1.612 101.09%
q19 13.42 12.62 -0.797 94.06%
q20 26.37 26.35 -0.021 99.92%
q21 222.14 228.32 6.184 102.78%
q22 13.73 13.50 -0.231 98.32%
total 1223.07 1233.72 10.649 100.87%

@zhztheplayer
Copy link
Member Author

@rui-mo @zzcclp

@zhztheplayer zhztheplayer marked this pull request as ready for review February 2, 2024 08:00
@zhztheplayer zhztheplayer changed the title [CORE] Avoid checking is-adaptive state when finding reusable sub-queries [CORE] Remove a redundant isAdaptive check Feb 2, 2024
@zhztheplayer zhztheplayer changed the title [CORE] Remove a redundant isAdaptive check [CORE] Remove a redundant is-adaptive check Feb 2, 2024
@zhztheplayer
Copy link
Member Author

@PHILO-HE

PHILO-HE
PHILO-HE previously approved these changes Feb 4, 2024
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.

Looks good! Thanks! cc @zzcclp

zzcclp
zzcclp previously approved these changes Feb 4, 2024
Copy link
Contributor

@zzcclp zzcclp left a comment

Choose a reason for hiding this comment

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

LGTM, please fix conflicts

Copy link

github-actions bot commented Feb 4, 2024

Run Gluten Clickhouse CI

@zzcclp zzcclp merged commit 52484cf into apache:main Feb 4, 2024
19 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_4618_time.csv log/native_master_02_03_2024_75285bcb8_time.csv difference percentage
q1 32.58 33.28 0.696 102.14%
q2 26.42 24.26 -2.152 91.85%
q3 37.59 38.78 1.194 103.18%
q4 38.97 37.64 -1.332 96.58%
q5 69.91 70.97 1.053 101.51%
q6 7.16 8.56 1.393 119.45%
q7 84.75 84.29 -0.452 99.47%
q8 84.57 84.63 0.061 100.07%
q9 122.76 121.75 -1.008 99.18%
q10 44.48 43.29 -1.196 97.31%
q11 19.89 20.58 0.698 103.51%
q12 28.39 26.43 -1.959 93.10%
q13 44.78 45.82 1.034 102.31%
q14 16.37 21.75 5.376 132.84%
q15 29.20 29.07 -0.130 99.56%
q16 12.93 14.30 1.365 110.56%
q17 101.37 103.31 1.935 101.91%
q18 149.11 149.45 0.340 100.23%
q19 12.52 13.09 0.564 104.50%
q20 27.51 26.67 -0.839 96.95%
q21 223.36 222.74 -0.617 99.72%
q22 13.41 13.69 0.276 102.06%
total 1228.04 1234.34 6.299 100.51%

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

4 participants