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.

Sign up for GitHub

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

Jump to bottom

[GLUTEN-5620][CORE] Remove check_overflow and refactor code #5654

Merged
merged 3 commits into from May 14, 2024
Merged

[GLUTEN-5620][CORE] Remove check_overflow and refactor code #5654

merged 3 commits into from May 14, 2024

Conversation

jinchengchenghh
Copy link
Contributor

No description provided.

@jinchengchenghh jinchengchenghh marked this pull request as draft May 8, 2024 06:37
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

#5620

@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

Run Gluten Clickhouse CI

3 similar comments
@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

github-actions bot commented May 8, 2024

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Run Gluten Clickhouse CI

@jinchengchenghh jinchengchenghh changed the title [GLUTEN-5620][CORE] Remove check_overflow [GLUTEN-5620][CORE] Remove check_overflow and refactor code May 9, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Run Gluten Clickhouse CI

1 similar comment
@github-actions GitHub Actions
Copy link

github-actions bot commented May 9, 2024

Run Gluten Clickhouse CI

@jinchengchenghh
Copy link
Contributor Author

/Benchmark Velox

@GlutenPerfBot
Copy link
Contributor

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

query log/native_5654_time.csv log/native_master_05_08_2024_e5f1e5edf_time.csv difference percentage
q1 33.82 35.03 1.206 103.57%
q2 25.86 21.91 -3.954 84.71%
q3 36.34 36.38 0.032 100.09%
q4 39.54 38.35 -1.187 97.00%
q5 70.59 71.54 0.956 101.35%
q6 7.40 5.99 -1.404 81.02%
q7 82.85 83.77 0.920 101.11%
q8 84.78 85.53 0.750 100.88%
q9 121.65 122.85 1.202 100.99%
q10 42.84 46.15 3.310 107.73%
q11 22.91 20.42 -2.485 89.15%
q12 27.53 23.69 -3.846 86.03%
q13 54.58 55.18 0.596 101.09%
q14 17.89 19.54 1.653 109.24%
q15 30.91 30.65 -0.261 99.15%
q16 13.25 13.73 0.485 103.66%
q17 103.40 103.15 -0.253 99.76%
q18 145.63 146.63 1.008 100.69%
q19 14.46 14.98 0.520 103.60%
q20 28.47 28.36 -0.109 99.62%
q21 286.28 283.86 -2.418 99.16%
q22 14.72 15.84 1.119 107.61%
total 1305.69 1303.53 -2.160 99.83%

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@jinchengchenghh jinchengchenghh marked this pull request as ready for review May 10, 2024 02:44
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

1 similar comment
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@jinchengchenghh
Copy link
Contributor Author

/Benchmark Velox TPCDS

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

1 similar comment
@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@GlutenPerfBot
Copy link
Contributor

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

query log/native_5654_time.csv log/native_master_05_09_2024_949650969_time.csv difference percentage
q1 33.61 34.78 1.173 103.49%
q2 24.01 23.41 -0.596 97.52%
q3 34.92 37.42 2.504 107.17%
q4 38.01 38.34 0.332 100.87%
q5 69.44 70.90 1.459 102.10%
q6 7.75 7.35 -0.405 94.78%
q7 81.46 80.34 -1.123 98.62%
q8 81.04 83.85 2.808 103.46%
q9 124.02 121.16 -2.857 97.70%
q10 45.37 46.73 1.356 102.99%
q11 20.33 19.94 -0.388 98.09%
q12 27.70 27.24 -0.462 98.33%
q13 54.43 53.72 -0.715 98.69%
q14 17.92 18.93 1.003 105.60%
q15 29.63 29.19 -0.440 98.52%
q16 15.96 13.91 -2.058 87.11%
q17 103.01 103.03 0.027 100.03%
q18 145.51 146.45 0.940 100.65%
q19 13.58 16.62 3.038 122.38%
q20 27.39 25.96 -1.424 94.80%
q21 287.26 280.52 -6.739 97.65%
q22 14.86 14.69 -0.169 98.86%
total 1297.19 1294.46 -2.735 99.79%

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@jinchengchenghh
Copy link
Contributor Author

/Benchmark Velox TPCDS

@GlutenPerfBot
Copy link
Contributor

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

query log/native_5654_time.csv log/native_master_05_09_2024_949650969_time.csv difference percentage
q1 33.83 34.78 0.955 102.82%
q2 23.67 23.41 -0.262 98.89%
q3 37.17 37.42 0.257 100.69%
q4 38.55 38.34 -0.206 99.47%
q5 68.72 70.90 2.180 103.17%
q6 6.94 7.35 0.411 105.92%
q7 81.45 80.34 -1.110 98.64%
q8 85.27 83.85 -1.419 98.34%
q9 122.18 121.16 -1.021 99.16%
q10 46.23 46.73 0.497 101.08%
q11 20.30 19.94 -0.355 98.25%
q12 27.97 27.24 -0.736 97.37%
q13 54.22 53.72 -0.508 99.06%
q14 19.46 18.93 -0.529 97.28%
q15 30.61 29.19 -1.421 95.36%
q16 13.87 13.91 0.034 100.24%
q17 103.51 103.03 -0.479 99.54%
q18 142.01 146.45 4.448 103.13%
q19 13.46 16.62 3.154 123.43%
q20 25.91 25.96 0.047 100.18%
q21 283.73 280.52 -3.214 98.87%
q22 14.38 14.69 0.302 102.10%
total 1293.43 1294.46 1.026 100.08%

@GlutenPerfBot
Copy link
Contributor

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

query log/native_5654_time.csv log/native_master_05_09_2024_949650969_time.csv difference percentage
q1 14.91 14.56 -0.345 97.69%
q2 17.42 18.73 1.308 107.51%
q3 4.06 4.31 0.250 106.15%
q4 61.85 61.90 0.054 100.09%
q5 8.02 8.13 0.109 101.36%
q6 3.94 3.89 -0.045 98.86%
q7 6.14 4.33 -1.813 70.48%
q8 5.46 4.66 -0.802 85.32%
q9 17.30 17.57 0.272 101.57%
q10 9.47 10.29 0.816 108.61%
q11 39.24 40.34 1.097 102.80%
q12 1.37 1.51 0.140 110.27%
q13 5.82 5.94 0.117 102.01%
q14a 43.98 42.91 -1.073 97.56%
q14b 43.39 42.82 -0.578 98.67%
q15 4.31 3.82 -0.493 88.57%
q16 40.71 40.76 0.051 100.13%
q17 5.99 4.47 -1.525 74.55%
q18 5.99 7.91 1.923 132.12%
q19 5.15 5.26 0.118 102.29%
q20 1.33 1.55 0.223 116.81%
q21 1.10 2.96 1.863 269.12%
q22 8.09 8.05 -0.044 99.46%
q23a 83.08 81.95 -1.136 98.63%
q23b 102.58 101.99 -0.590 99.42%
q24a 76.80 74.39 -2.416 96.85%
q24b 79.28 69.24 -10.039 87.34%
q25 4.28 4.17 -0.107 97.51%
q26 2.48 2.63 0.146 105.89%
q27 2.74 2.82 0.087 103.16%
q28 22.87 19.86 -3.011 86.83%
q29 7.03 9.17 2.141 130.45%
q30 4.14 10.67 6.528 257.50%
q31 5.68 6.18 0.500 108.81%
q32 1.07 1.06 -0.004 99.62%
q33 5.89 5.76 -0.131 97.78%
q34 4.91 4.79 -0.120 97.55%
q35 6.27 6.33 0.064 101.02%
q36 3.05 2.97 -0.085 97.23%
q37 3.91 3.81 -0.099 97.47%
q38 11.77 11.72 -0.044 99.62%
q39a 3.21 3.36 0.147 104.57%
q39b 4.81 3.04 -1.767 63.26%
q40 3.91 6.95 3.037 177.72%
q41 0.59 0.54 -0.056 90.55%
q42 0.84 0.88 0.041 104.89%
q43 3.43 3.55 0.121 103.53%
q44 6.80 6.75 -0.056 99.18%
q45 3.32 3.34 0.019 100.58%
q46 2.86 2.92 0.053 101.87%
q47 14.12 14.06 -0.052 99.63%
q48 4.34 4.33 -0.006 99.86%
q49 7.17 7.13 -0.041 99.43%
q50 24.13 24.02 -0.105 99.57%
q51 8.31 8.39 0.082 100.98%
q52 1.01 0.98 -0.027 97.34%
q53 2.77 1.59 -1.179 57.47%
q54 3.11 2.98 -0.124 96.00%
q55 0.98 0.99 0.017 101.75%
q56 5.44 5.22 -0.216 96.03%
q57 8.67 8.34 -0.327 96.23%
q58 2.34 2.46 0.119 105.06%
q59 13.98 14.22 0.245 101.75%
q60 8.52 5.78 -2.742 67.81%
q61 6.00 6.36 0.364 106.06%
q62 3.60 4.00 0.400 111.09%
q63 1.76 1.87 0.107 106.11%
q64 51.64 48.61 -3.033 94.13%
q65 13.85 13.95 0.096 100.69%
q66 2.76 5.54 2.781 200.70%
q67 355.62 352.79 -2.825 99.21%
q68 3.30 3.36 0.060 101.82%
q69 6.38 6.40 0.015 100.24%
q70 8.37 10.62 2.248 126.87%
q71 3.72 2.30 -1.415 61.92%
q72 191.18 187.38 -3.807 98.01%
q73 2.10 3.72 1.616 176.86%
q74 21.04 21.40 0.366 101.74%
q75 25.14 24.64 -0.500 98.01%
q76 10.30 7.50 -2.803 72.80%
q77 1.80 1.56 -0.247 86.33%
q78 38.54 40.67 2.134 105.54%
q79 3.27 3.36 0.085 102.60%
q80 11.35 10.96 -0.389 96.57%
q81 7.97 4.30 -3.668 53.96%
q82 6.86 6.48 -0.380 94.46%
q83 1.37 3.37 2.004 246.54%
q84 3.00 3.02 0.013 100.43%
q85 7.09 6.87 -0.223 96.85%
q86 3.87 3.25 -0.616 84.07%
q87 13.76 12.14 -1.623 88.21%
q88 16.64 16.60 -0.036 99.79%
q89 2.77 2.74 -0.027 99.03%
q90 5.78 3.24 -2.538 56.07%
q91 2.72 2.70 -0.011 99.60%
q92 1.22 1.18 -0.040 96.75%
q93 29.09 31.60 2.511 108.63%
q94 21.31 21.59 0.281 101.32%
q9 86.95 84.38 -2.569 97.05%
q5 2.28 4.76 2.480 208.76%
q96 12.29 12.50 0.210 101.71%
q97 1.95 1.88 -0.068 96.49%
q98 8.40 8.26 -0.139 98.34%
q99 8.40 8.26 -0.139 98.34%
total 1914.56 1895.86 -18.696 99.02%

@jinchengchenghh
Copy link
Contributor Author

Spark 33 result:

<style> </style>
query log/native_5654_time.csv log/native_master_05_10_2024_time.csv difference percentage
q1 7.49 9.88 2.392 131.94%
q2 8.94 8.15 -0.798 91.08%
q3 2.78 2.65 -0.123 95.56%
q4 46.66 47.23 0.565 101.21%
q5 6.56 6.49 -0.069 98.95%
q6 2.45 3.13 0.681 127.79%
q7 3.13 4.76 1.629 152.08%
q8 3.20 3.53 0.329 110.28%
q9 15.51 14.83 -0.686 95.58%
q10 7.71 7.17 -0.542 92.98%
q11 24.71 23.85 -0.860 96.52%
q12 0.96 0.87 -0.094 90.26%
q13 4.40 4.26 -0.138 96.87%
q14a 32.63 33.05 0.420 101.29%
q14b 30.64 30.59 -0.051 99.83%
q15 1.58 1.59 0.005 100.33%
q16 6.67 6.32 -0.345 94.83%
q17 3.37 3.35 -0.018 99.48%
q18 4.88 3.92 -0.958 80.37%
q19 1.12 1.14 0.025 102.23%
q20 0.82 0.76 -0.058 92.87%
q21 0.58 0.57 -0.012 97.97%
q22 2.13 2.95 0.819 138.50%
q23a 61.91 61.29 -0.618 99.00%
q23b 76.75 76.42 -0.322 99.58%
q24a 78.77 80.80 2.028 102.57%
q24b 76.76 77.91 1.145 101.49%
q25 2.79 2.82 0.031 101.11%
q26 3.10 2.62 -0.478 84.59%
q27 1.87 1.84 -0.027 98.57%
q28 18.06 18.28 0.219 101.21%
q29 6.12 6.64 0.517 108.45%
q30 3.02 2.85 -0.177 94.16%
q31 3.98 4.30 0.321 108.07%
q32 0.56 0.54 -0.014 97.40%
q33 1.15 1.21 0.057 104.92%
q34 2.87 2.83 -0.047 98.36%
q35 4.77 4.69 -0.079 98.35%
q36 1.91 2.61 0.700 136.74%
q37 2.72 2.75 0.022 100.82%
q38 9.01 8.86 -0.147 98.37%
q39a 2.42 2.18 -0.245 89.89%
q39b 2.26 2.23 -0.032 98.58%
q40 2.70 2.81 0.113 104.19%
q41 0.29 0.30 0.004 101.45%
q42 0.41 0.39 -0.017 95.92%
q43 1.92 2.22 0.306 115.97%
q44 5.82 5.70 -0.122 97.90%
q45 1.80 1.83 0.033 101.84%
q46 1.93 1.89 -0.045 97.68%
q47 9.90 10.26 0.359 103.63%
q48 3.27 3.20 -0.066 97.98%
q49 3.41 3.21 -0.198 94.20%
q50 16.24 16.10 -0.140 99.14%
q51 5.28 5.43 0.148 102.80%
q52 0.56 0.52 -0.038 93.17%
q53 1.07 1.11 0.039 103.63%
q54 1.98 2.01 0.029 101.44%
q55 0.46 0.50 0.040 108.80%
q56 1.18 1.01 -0.170 85.57%
q57 5.72 5.81 0.091 101.60%
q58 1.34 1.37 0.029 102.14%
q59 4.06 4.11 0.050 101.22%
q60 1.44 1.49 0.051 103.56%
q61 1.44 1.49 0.049 103.39%
q62 3.46 3.16 -0.306 91.17%
q63 1.10 1.11 0.005 100.43%
q64 24.32 24.09 -0.232 99.05%
q65 10.11 10.55 0.444 104.39%
q66 2.28 2.18 -0.101 95.56%
q67 274.68 325.86 51.176 118.63%
q68 2.19 2.17 -0.023 98.95%
q69 4.00 5.13 1.131 128.29%
q70 5.09 5.10 0.007 100.14%
q71 1.29 1.59 0.299 123.08%
q72 25.59 23.36 -2.233 91.27%
q73 1.46 1.51 0.045 103.08%
q74 15.64 15.22 -0.421 97.31%
q75 20.32 20.13 -0.185 99.09%
q76 6.69 6.83 0.144 102.15%
q77 0.99 1.09 0.100 110.14%
q78 31.91 35.61 3.697 111.59%
q79 2.46 2.64 0.179 107.28%
q80 8.68 8.22 -0.463 94.67%
q81 3.04 3.06 0.023 100.74%
q82 4.54 4.88 0.334 107.35%
q83 0.94 0.82 -0.119 87.31%
q84 1.58 1.65 0.064 104.03%
q85 3.60 3.80 0.203 105.63%
q86 1.89 2.03 0.137 107.27%
q87 9.24 9.09 -0.154 98.34%
q88 12.93 12.69 -0.234 98.19%
q89 1.68 1.75 0.072 104.26%
q90 1.61 1.46 -0.146 90.94%
q91 1.51 1.62 0.105 106.96%
q92 0.56 0.51 -0.041 92.57%
q93 21.35 23.94 2.591 112.14%
q94 8.96 9.44 0.484 105.40%
q95 63.36 66.02 2.662 104.20%
q96 1.74 1.81 0.072 104.13%
q97 9.70 9.89 0.192 101.98%
q98 1.25 1.34 0.082 106.56%
q99 8.31 8.10 -0.207 97.51%
total 1252.33 1317.23 64.899 105.18%

@jinchengchenghh
Copy link
Contributor Author

Can you help rerun the failed test? This failure is not related to the PR. @zhouyuan

@zhouyuan
Copy link
Contributor

#5679

@jinchengchenghh
Copy link
Contributor Author

Can you help merge it? Thanks! @zhouyuan

ulysses-you
ulysses-you reviewed May 13, 2024
View reviewed changes
gluten-core/src/main/scala/org/apache/gluten/utils/DecimalArithmeticUtil.scala
operationType: OperationType.Config,
type1: DecimalType,
type2: DecimalType): DecimalType = {
def getResultType(expr: BinaryArithmetic, type1: DecimalType, type2: DecimalType): DecimalType = {
Copy link
Contributor

Choose a reason for hiding this comment

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

If we are going into here, that means there is no checkoverflow for decimal binary arithmetic. If there is no PromotePrecision, should the result decimal type be b.dataType ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, this may run into by Checkoverflow(child: BinaryArithmetic), if we match this one like this version https://github.com/apache/incubator-gluten/compare/3c88fd40395c94505d76b5c146cd9498fe1a33b6..8110523f356e7356d1ff0a590610699f016e52d0, the tests can be passed, but performance regression, so I change it to current minimal change version.

baibaichen
baibaichen reviewed May 13, 2024
View reviewed changes
gluten-core/src/main/scala/org/apache/gluten/expression/ExpressionConverter.scala
@@ -492,7 +514,6 @@ object ExpressionConverter extends SQLConfHelper with Logging {
expr.children.map(
replaceWithExpressionTransformerInternal(_, attributeSeq, expressionsMap)),
expr)

case CheckOverflow(b: BinaryArithmetic, decimalType, _)
if !BackendsApiManager.getSettings.transformCheckOverflow &&
DecimalArithmeticUtil.isDecimalArithmetic(b) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

@jinchengchenghh if you update codes at the following

      case b: BinaryArithmetic if DecimalArithmeticUtil.isDecimalArithmetic(b) =>
        DecimalArithmeticUtil.checkAllowDecimalArithmetic()
       ///...

The same logical should apply there

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Velox transformCheckOverflow is true, so it won't go here. And rescale and remove cast only needs in velox backend

@zhouyuan
Copy link
Contributor

@baibaichen @ulysses-you
I assume the questions on this refactor work is solved. chengcheng and I will follow up if required

@zhouyuan zhouyuan merged commit e807856 into apache:main May 14, 2024
43 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@baibaichen baibaichen baibaichen left review comments

@ulysses-you ulysses-you ulysses-you left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@jinchengchenghh @GlutenPerfBot @zhouyuan @baibaichen @ulysses-you