Skip to content

[CORE] Optimize duplicated code for create rel node#8548

Merged
zml1206 merged 2 commits intoapache:mainfrom
zml1206:optimize_createRel
Jan 17, 2025
Merged

[CORE] Optimize duplicated code for create rel node#8548
zml1206 merged 2 commits intoapache:mainfrom
zml1206:optimize_createRel

Conversation

@zml1206
Copy link
Copy Markdown
Contributor

@zml1206 zml1206 commented Jan 16, 2025

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)

@github-actions github-actions bot added CORE works for Gluten Core VELOX labels Jan 16, 2025
@github-actions
Copy link
Copy Markdown

Thanks for opening a pull request!

Could you open an issue for this pull request on Github Issues?

https://github.com/apache/incubator-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:

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@zml1206 zml1206 requested a review from zhztheplayer January 17, 2025 02:20
Copy link
Copy Markdown
Member

@zhztheplayer zhztheplayer left a comment

Choose a reason for hiding this comment

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

Just a minor. Thanks


import scala.collection.JavaConverters._

object RelBuilderUtil {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Any consideration not adding the APIs to RelBuilder directly?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

RelBuilder is a java class, add it in requires additional type conversion, such as scala Seq.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There is also Java classes like JList[ExpressionNode] in this file. Can we just put them to RelBuilder? Then make caller call "asJava" when passing the Scala collections which should be trivial. Otherwise developer will have to know 2 different helper classes for similar purposes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Mainly it seems inappropriate to call replaceWithExpressionTransformer in RelBuild. Here I extracted part of the logic, stay up replaceWithExpressionTransformer, CI failure unrelated, please take a look again, thank you.

@github-actions
Copy link
Copy Markdown

Run Gluten Clickhouse CI on x86

@zml1206 zml1206 merged commit 764ca97 into apache:main Jan 17, 2025
@GlutenPerfBot
Copy link
Copy Markdown
Contributor

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

query log/native_master_01_17_2025_time.csv log/native_master_01_16_2025_ac8e03a1d6_time.csv difference percentage
q1 15.74 15.58 -0.162 98.97%
q2 15.74 16.34 0.601 103.82%
q3 4.51 2.84 -1.662 63.11%
q4 86.19 84.52 -1.673 98.06%
q5 13.36 13.39 0.035 100.26%
q6 5.16 5.39 0.230 104.46%
q7 8.37 8.08 -0.294 96.49%
q8 4.34 7.15 2.801 164.46%
q9 28.04 28.87 0.832 102.97%
q10 11.97 12.37 0.396 103.30%
q11 41.60 42.82 1.220 102.93%
q12 2.10 1.93 -0.168 91.98%
q13 11.25 9.87 -1.374 87.79%
q14a 66.90 69.54 2.644 103.95%
q14b 56.74 56.90 0.164 100.29%
q15 3.54 3.22 -0.320 90.98%
q16 29.20 30.63 1.436 104.92%
q17 7.58 7.29 -0.286 96.23%
q18 10.21 10.62 0.409 104.01%
q19 3.72 3.77 0.043 101.16%
q20 2.22 2.74 0.520 123.47%
q21 1.82 1.82 -0.008 99.55%
q22 9.71 11.54 1.833 118.88%
q23a 137.85 141.10 3.253 102.36%
q23b 163.36 162.32 -1.043 99.36%
q24a 107.60 101.40 -6.198 94.24%
q24b 93.90 99.97 6.065 106.46%
q25 6.41 7.10 0.694 110.82%
q26 4.22 3.98 -0.245 94.21%
q27 6.58 6.53 -0.055 99.17%
q28 36.67 37.15 0.479 101.31%
q29 16.98 18.49 1.512 108.91%
q30 6.35 6.47 0.115 101.81%
q31 11.02 11.12 0.092 100.84%
q32 2.19 2.32 0.123 105.61%
q33 7.01 7.37 0.367 105.24%
q34 4.93 4.07 -0.864 82.50%
q35 9.82 10.52 0.699 107.12%
q36 5.68 5.85 0.165 102.90%
q37 5.31 5.49 0.177 103.32%
q38 17.40 19.10 1.698 109.76%
q39a 4.38 5.08 0.704 116.09%
q39b 4.77 4.60 -0.166 96.51%
q40 5.61 6.30 0.689 112.29%
q41 1.14 0.84 -0.298 73.86%
q42 1.36 1.24 -0.127 90.69%
q43 4.56 4.59 0.033 100.72%
q44 12.74 12.98 0.238 101.87%
q45 7.33 5.34 -1.991 72.85%
q46 5.40 5.16 -0.242 95.52%
q47 20.61 20.69 0.079 100.38%
q48 6.68 7.91 1.227 118.36%
q49 10.89 10.49 -0.409 96.25%
q50 38.76 37.89 -0.868 97.76%
q51 14.60 14.98 0.379 102.59%
q52 1.25 1.20 -0.057 95.48%
q53 2.93 3.31 0.384 113.12%
q54 6.85 7.00 0.151 102.20%
q55 1.33 1.41 0.087 106.53%
q56 7.22 7.08 -0.137 98.10%
q57 13.04 12.97 -0.070 99.46%
q58 3.39 3.69 0.296 108.73%
q59 6.76 6.71 -0.052 99.23%
q60 7.62 8.99 1.375 118.04%
q61 8.17 7.41 -0.754 90.77%
q62 5.34 4.84 -0.505 90.56%
q63 3.16 3.10 -0.064 97.96%
q64 62.41 60.43 -1.980 96.83%
q65 29.81 30.10 0.287 100.96%
q66 4.42 4.27 -0.150 96.60%
q67 227.89 229.36 1.470 100.65%
q68 4.41 4.41 -0.001 99.97%
q69 7.10 7.18 0.076 101.07%
q70 12.78 12.20 -0.586 95.41%
q71 5.75 3.95 -1.802 68.67%
q72 37.66 38.86 1.199 103.18%
q73 3.38 3.19 -0.188 94.45%
q74 26.63 26.30 -0.334 98.75%
q75 43.63 44.10 0.475 101.09%
q76 14.42 14.31 -0.109 99.24%
q77 3.33 3.46 0.129 103.86%
q78 84.94 84.97 0.031 100.04%
q79 5.26 5.16 -0.105 98.01%
q80 17.01 17.00 -0.013 99.93%
q81 7.90 8.21 0.307 103.89%
q82 10.18 10.35 0.167 101.64%
q83 2.51 2.63 0.126 105.01%
q84 3.70 3.62 -0.082 97.79%
q85 10.02 9.87 -0.156 98.45%
q86 4.55 4.51 -0.040 99.11%
q87 20.44 17.63 -2.813 86.24%
q88 23.13 24.01 0.882 103.81%
q89 4.38 4.31 -0.065 98.51%
q90 3.55 3.40 -0.148 95.84%
q91 6.51 4.84 -1.674 74.31%
q92 1.88 3.00 1.118 159.46%
q93 52.03 54.71 2.679 105.15%
q94 18.00 17.76 -0.241 98.66%
q9 98.15 98.94 0.788 100.80%
q5 3.01 2.99 -0.027 99.11%
q96 28.01 28.47 0.456 101.63%
q97 3.10 2.77 -0.337 89.14%
q98 10.38 9.81 -0.574 94.47%
q99 10.38 9.81 -0.574 94.47%
total 2191.53 2204.45 12.919 100.59%

baibaichen pushed a commit to baibaichen/gluten that referenced this pull request Feb 1, 2025
@zml1206 zml1206 deleted the optimize_createRel branch December 9, 2025 08:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CORE works for Gluten Core VELOX

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants