Skip to content

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

Merged
ulysses-you merged 4 commits into
apache:mainfrom
Yohahaha:fix-conf
Nov 21, 2023
Merged

[VL] Respect spark.gluten.sql.debug in native side#3748
ulysses-you merged 4 commits into
apache:mainfrom
Yohahaha:fix-conf

Conversation

@Yohahaha
Copy link
Copy Markdown
Contributor

@Yohahaha Yohahaha commented Nov 16, 2023

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 16, 2023

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
Copy link
Copy Markdown

github-actions Bot commented Nov 16, 2023

Run Gluten Clickhouse CI

@Yohahaha
Copy link
Copy Markdown
Contributor Author

Yohahaha commented Nov 16, 2023

We could use velox config lib in whole velox dir.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 17, 2023

Run Gluten Clickhouse CI

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 17, 2023

Run Gluten Clickhouse CI

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 20, 2023

Run Gluten Clickhouse CI

@Yohahaha
Copy link
Copy Markdown
Contributor Author

Yohahaha commented Nov 20, 2023

@rui-mo @philo-he please help review this patch, thanks!

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

Yohahaha commented Nov 20, 2023

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 20, 2023

Run Gluten Clickhouse CI

philo-he
philo-he previously approved these changes Nov 20, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@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
Copy Markdown
Contributor Author

Yohahaha commented Nov 20, 2023 

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
Copy Markdown
Member

philo-he commented Nov 20, 2023 

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
Copy Markdown
Contributor Author

Yohahaha commented Nov 21, 2023

@philo-he please review again, thank you!

@philo-he
Copy link
Copy Markdown
Member

philo-he commented Nov 21, 2023 •
edited
Loading

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

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 21, 2023

Run Gluten Clickhouse CI

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Nov 21, 2023

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
Copy Markdown
Contributor Author

Yohahaha commented Nov 21, 2023

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

philo-he
philo-he approved these changes Nov 21, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@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
Comment thread cpp/core/compute/Runtime.h
/// Return true if parsed successfully.
void parsePlan(const uint8_t* data, int32_t size, SparkTaskInfo taskInfo) {
taskInfo_ = taskInfo;
#ifdef GLUTEN_PRINT_DEBUG
Copy link
Copy Markdown
Contributor

@rui-mo rui-mo Nov 21, 2023

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor Author

Yohahaha commented Nov 21, 2023

@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
@Yohahaha Yohahaha deleted the fix-conf branch November 21, 2023 07:14
@GlutenPerfBot
Copy link
Copy Markdown
Contributor

GlutenPerfBot commented Nov 21, 2023

===== 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

@philo-he philo-he philo-he approved these changes

@rui-mo rui-mo rui-mo 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