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

[GLUTEN-4835][CORE] Match metric names with Spark #4834

Merged
merged 4 commits into from
Mar 11, 2024

Conversation

clee704
Copy link
Contributor

@clee704 clee704 commented Mar 1, 2024

What changes were proposed in this pull request?

Rename metrics "inputRows" and "outputRows" to "numInputRows" and "numOutputRows" respectively, to match the names used in Spark.

(Fixes: #4835)

How was this patch tested?

Added a test case under GlutenSQLQuerySuite

Copy link

github-actions bot commented Mar 1, 2024

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 Mar 1, 2024

Run Gluten Clickhouse CI

@zhli1142015
Copy link
Contributor

Thanks, could you open an issue for this, to describe why need this change.

@clee704
Copy link
Contributor Author

clee704 commented Mar 1, 2024

I'll create one. Here's the short answer - it breaks some Spark internal things due to the mismatch of metric names.

@clee704 clee704 changed the title Match metric names with Spark [WIP][Draft][DoNotMerge] Match metric names with Spark Mar 1, 2024
@clee704 clee704 changed the title [WIP][Draft][DoNotMerge] Match metric names with Spark [WIP][Draft][DoNotMerge][GLUTEN-4835][CORE] Match metric names with Spark Mar 2, 2024
Copy link

github-actions bot commented Mar 2, 2024

Run Gluten Clickhouse CI

@zhli1142015 zhli1142015 changed the title [WIP][Draft][DoNotMerge][GLUTEN-4835][CORE] Match metric names with Spark [WIP][GLUTEN-4835][CORE] Match metric names with Spark Mar 2, 2024
Copy link

github-actions bot commented Mar 5, 2024

Run Gluten Clickhouse CI

@clee704 clee704 changed the title [WIP][GLUTEN-4835][CORE] Match metric names with Spark [GLUTEN-4835][CORE] Match metric names with Spark Mar 5, 2024
@clee704 clee704 marked this pull request as ready for review March 5, 2024 01:52
Copy link

github-actions bot commented Mar 5, 2024

#4835

Copy link

github-actions bot commented Mar 5, 2024

Run Gluten Clickhouse CI

Copy link
Contributor

@zhli1142015 zhli1142015 left a comment

Choose a reason for hiding this comment

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

Thanks.

@zhli1142015
Copy link
Contributor

cc @zhouyuan and @zzcclp , thanks.

Copy link

github-actions bot commented Mar 6, 2024

Run Gluten Clickhouse CI

Copy link

github-actions bot commented Mar 6, 2024

Run Gluten Clickhouse CI

@zhouyuan
Copy link
Contributor

zhouyuan commented Mar 7, 2024

The patch looks good to me. Indeed we didnt pay much attention on the Spark built metrics/instrumentation system.
CC @rui-mo can you please also take a look?

@rui-mo
Copy link
Contributor

rui-mo commented Mar 7, 2024

@FelixYBW Do you think this change makes sense? Thanks.

@FelixYBW
Copy link
Contributor

FelixYBW commented Mar 7, 2024

Yes, looks the names are inherited from Gazelle. Not sure why we don't use spark standard name

@rui-mo rui-mo requested a review from zzcclp March 7, 2024 03:44
Copy link
Contributor

@ulysses-you ulysses-you left a comment

Choose a reason for hiding this comment

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

+1, lgtm

Copy link
Contributor

@zhli1142015 zhli1142015 left a comment

Choose a reason for hiding this comment

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

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.

@zzcclp Please help confirm on CH backend, thanks!

@ulysses-you ulysses-you merged commit cfa9afa into apache:main Mar 11, 2024
20 checks passed
@GlutenPerfBot
Copy link
Contributor

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

query log/native_4834_time.csv log/native_master_03_10_2024_3ad58ce14_time.csv difference percentage
q1 35.67 35.86 0.189 100.53%
q2 23.59 25.95 2.357 109.99%
q3 38.26 36.82 -1.439 96.24%
q4 37.67 38.96 1.291 103.43%
q5 69.63 70.83 1.193 101.71%
q6 7.47 7.41 -0.057 99.23%
q7 82.40 83.10 0.700 100.85%
q8 85.74 85.35 -0.385 99.55%
q9 121.63 118.73 -2.892 97.62%
q10 45.86 42.98 -2.879 93.72%
q11 20.19 21.60 1.413 107.00%
q12 26.90 25.06 -1.841 93.15%
q13 47.84 46.55 -1.289 97.31%
q14 16.67 19.73 3.058 118.34%
q15 29.17 32.39 3.213 111.02%
q16 12.79 13.72 0.931 107.28%
q17 101.53 100.46 -1.071 98.95%
q18 142.37 142.43 0.069 100.05%
q19 15.20 13.62 -1.581 89.60%
q20 27.38 26.87 -0.512 98.13%
q21 224.48 225.05 0.564 100.25%
q22 13.91 14.04 0.130 100.94%
total 1226.32 1227.48 1.162 100.09%

taiyang-li pushed a commit to bigo-sg/gluten that referenced this pull request Mar 25, 2024
taiyang-li pushed a commit to bigo-sg/gluten that referenced this pull request Oct 8, 2024
taiyang-li pushed a commit to bigo-sg/gluten that referenced this pull request Oct 9, 2024
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.

[Gluten] Change metric names to match Spark's
8 participants