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

[VL] Respect spark.gluten.sql.debug in native side #3748

Merged
merged 4 commits into from
Nov 21, 2023
Merged

[VL] Respect spark.gluten.sql.debug in native side #3748

merged 4 commits into from
Nov 21, 2023

Conversation

Yohahaha
Copy link
Contributor

Use spark.gluten.sql.debug to enable print static/session config, print json plan.

@github-actions GitHub Actions
Copy link

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:

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@Yohahaha
Copy link
Contributor Author

We could use velox config lib in whole velox dir.

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@Yohahaha
Copy link
Contributor Author

@rui-mo @PHILO-HE please help review this patch, thanks!

@Yohahaha Yohahaha mentioned this pull request Nov 20, 2023
Merged
@Yohahaha
Copy link
Contributor Author

I think CI failure does not related with this patch. @zhztheplayer

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

PHILO-HE
PHILO-HE previously approved these changes Nov 20, 2023
View reviewed changes
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!

@Yohahaha
Copy link
Contributor Author 

2023-11-20T08:02:28.5576407Z java.nio.file.FileSystemException: /tmp/blockmgr-e10fee5b-5d5f-4a2d-8e20-42c7dc891628/25: No space left on device

@PHILO-HE seems your CI machine has problems.

@PHILO-HE
Copy link
Contributor 

2023-11-20T08:02:28.5576407Z java.nio.file.FileSystemException: /tmp/blockmgr-e10fee5b-5d5f-4a2d-8e20-42c7dc891628/25: No space left on device

@PHILO-HE seems your CI machine has problems.

It should work now. Let's wait for the re-triggered CI jobs.

@Yohahaha
Copy link
Contributor Author

@PHILO-HE please review again, thank you!

@PHILO-HE
Copy link
Contributor

PHILO-HE commented Nov 21, 2023 •
edited
Loading

Looks good! Code conflict is reported. Please do a rebase again. Thanks!

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@github-actions GitHub Actions
Copy link

Run Gluten Clickhouse CI

@Yohahaha Yohahaha mentioned this pull request Nov 21, 2023
Merged
@Yohahaha Yohahaha requested a review from PHILO-HE November 21, 2023 06:13
@Yohahaha
Copy link
Contributor Author

cc @rui-mo @ulysses-you thank you!

PHILO-HE
PHILO-HE approved these changes Nov 21, 2023
View reviewed changes
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!

rui-mo
rui-mo reviewed Nov 21, 2023
View reviewed changes
cpp/core/compute/Runtime.h
/// Parse and cache the plan.
/// Return true if parsed successfully.
void parsePlan(const uint8_t* data, int32_t size, SparkTaskInfo taskInfo) {
taskInfo_ = taskInfo;
#ifdef GLUTEN_PRINT_DEBUG
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to control debug log through Gluten config in this PR? Previously, it is controlled by the compilation flag, see https://github.com/oap-project/gluten/blob/main/cpp/CMakeLists.txt#L76-L79. Maybe an option is to control the cpp compilation level through scala config.

rui-mo
rui-mo reviewed Nov 21, 2023
View reviewed changes
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.

Otherwise, we should consider removing RelWithDebInfo compilation option, and change all logics controlled by GLUTEN_PRINT_DEBUG to the new Scala config.

@Yohahaha
Copy link
Contributor Author

Yohahaha commented Nov 21, 2023 •
edited
Loading

This patch only move some debug info into scala config.

Otherwise, we should consider removing RelWithDebInfo compilation option, and change all logics controlled by GLUTEN_PRINT_DEBUG to the new Scala config.

It's what I'm going to do. CI does not check debug build and we encounter some build error in RelWithDebInfo when code changes, and it would be great to debug(not equal to cmake debug) without recompile.

@rui-mo
Copy link
Contributor

rui-mo commented Nov 21, 2023

@Yohahaha Makes sense to me. Could you open an issue on removing RelWithDebInfo and GLUTEN_PRINT_DEBUG?

@Yohahaha
Copy link
Contributor Author

@Yohahaha Makes sense to me. Could you open an issue on removing RelWithDebInfo and GLUTEN_PRINT_DEBUG?

#3794

@ulysses-you ulysses-you merged commit 018da4c into apache:main Nov 21, 2023
17 checks passed
@Yohahaha Yohahaha deleted the fix-conf branch November 21, 2023 07:14
@GlutenPerfBot
Copy link
Contributor

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

query log/native_3748_time.csv log/native_master_11_20_2023_60fc2a02e_time.csv difference percentage
q1 33.60 34.37 0.766 102.28%
q2 24.65 24.70 0.054 100.22%
q3 35.60 35.40 -0.204 99.43%
q4 37.92 36.40 -1.526 95.98%
q5 69.65 68.86 -0.796 98.86%
q6 7.89 7.01 -0.880 88.85%
q7 82.94 85.39 2.446 102.95%
q8 84.74 87.44 2.696 103.18%
q9 127.39 124.93 -2.463 98.07%
q10 46.44 46.83 0.386 100.83%
q11 20.01 19.99 -0.011 99.94%
q12 26.91 24.57 -2.341 91.30%
q13 45.42 44.85 -0.572 98.74%
q14 14.75 18.77 4.024 127.29%
q15 27.31 27.13 -0.179 99.35%
q16 15.65 15.41 -0.232 98.52%
q17 101.65 102.21 0.557 100.55%
q18 148.10 149.83 1.726 101.17%
q19 13.91 13.48 -0.431 96.90%
q20 27.71 28.10 0.390 101.41%
q21 220.45 220.09 -0.360 99.84%
q22 12.86 12.89 0.024 100.18%
total 1225.58 1228.65 3.073 100.25%

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rui-mo rui-mo rui-mo approved these changes

@PHILO-HE PHILO-HE PHILO-HE approved these changes

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

Successfully merging this pull request may close these issues.
5 participants
@Yohahaha @PHILO-HE @rui-mo @GlutenPerfBot @ulysses-you