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-3049] Introduce a local property to control the usage of gluten at thread level #3357

Merged
merged 4 commits into from
Oct 13, 2023
Merged

[GLUTEN-3049] Introduce a local property to control the usage of gluten at thread level #3357

merged 4 commits into from
Oct 13, 2023

Conversation

loneylee
Copy link
Member

@loneylee loneylee commented Oct 9, 2023

What changes were proposed in this pull request?

Support local properties to controller whole query fallback

How was this patch tested?

Test by ut

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

#3049

@github-actions
Copy link

github-actions bot commented Oct 9, 2023

Run Gluten Clickhouse CI

@loneylee
Copy link
Member Author

@PHILO-HE Please help me with a review ?

PHILO-HE
PHILO-HE reviewed Oct 10, 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.

I'm assuming end user needs to get a sparkContext to set this local property to control the usage of gluten. Another thought, is it workable to you in practice by just dynamically setting spark.gluten.enabled? It looks a context level controlling.

SET spark.gluten.enabled=false;

SELECT count(*) from example;

SET spark.gluten.enabled=true;

SELECT count(*) from example;

...ickhouse/src/test/scala/io/glutenproject/execution/GlutenClickHouseTPCHParquetAQESuite.scala Outdated Show resolved Hide resolved
@loneylee
Copy link
Member Author

I'm assuming end user needs to get a sparkContext to set this local property to control the usage of gluten. Another thought, is it workable to you in practice by just dynamically setting spark.gluten.enabled? It looks a context level controlling.

SET spark.gluten.enabled=false;

SELECT count(*) from example;

SET spark.gluten.enabled=true;

SELECT count(*) from example;

In my case, all users will shared the same context.

@github-actions
Copy link

Run Gluten Clickhouse CI

@zzcclp zzcclp requested a review from ulysses-you October 10, 2023 12:19
@PHILO-HE PHILO-HE changed the title [GLUTEN-3049] Add statement level controls fallback by local properties [GLUTEN-3049] Introduce a local property to control the usage of gluten at thread level Oct 11, 2023
@PHILO-HE
Copy link
Contributor

Just modified the title. It looks not that correct to use "statement level". Please feel free to re-modify it if you have a better expression.

@github-actions
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

PHILO-HE
PHILO-HE reviewed Oct 11, 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.

A few comments. Thanks!

gluten-core/src/main/scala/io/glutenproject/utils/QueryPlanSelector.scala Outdated Show resolved Hide resolved
gluten-core/src/main/scala/io/glutenproject/utils/QueryPlanSelector.scala Outdated Show resolved Hide resolved
...ickhouse/src/test/scala/io/glutenproject/execution/GlutenClickHouseTPCHParquetAQESuite.scala Outdated Show resolved Hide resolved
shims/common/src/main/scala/io/glutenproject/GlutenConfig.scala Outdated Show resolved Hide resolved
@github-actions
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

1 similar comment
@github-actions
Copy link

Run Gluten Clickhouse CI

@github-actions
Copy link

Run Gluten Clickhouse CI

PHILO-HE
PHILO-HE reviewed Oct 11, 2023
View reviewed changes
docs/Configuration.md
@@ -63,3 +63,9 @@ spark.driver.extraClassPath ${GLUTEN_HOME}/package/target/gluten-<>-jar-with-
spark.executor.extraClassPath ${GLUTEN_HOME}/package/target/gluten-<>-jar-with-dependencies.jar
######
```

Additionally, you can control the configurations of gluten at thread level by local property.
Copy link
Contributor

Choose a reason for hiding this comment

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

Assume users have to get a spark context to set local property. Could you add more details for how to set such property? Thanks!

gluten-ut/spark32/src/test/scala/org/apache/spark/sql/execution/FallbackStrategiesSuite.scala Outdated Show resolved Hide resolved
@github-actions
Copy link

Run Gluten Clickhouse CI

PHILO-HE
PHILO-HE approved these changes Oct 13, 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! @zhztheplayer, do you have any comment? Thanks!

zhztheplayer
zhztheplayer approved these changes Oct 13, 2023
View reviewed changes
Copy link
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.

Thanks!

@PHILO-HE PHILO-HE merged commit 60d2946 into apache:main Oct 13, 2023
14 checks passed
@loneylee loneylee deleted the 3049 branch October 13, 2023 05:29
lhuang09287750 added a commit to Kyligence/gluten that referenced this pull request Oct 13, 2023
@lhuang09287750
apache#3357 Introduce a local property to control the usage of glute…
7d20f72 
…n at thread level
@GlutenPerfBot
Copy link
Contributor

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

query log/native_3357_time.csv log/native_master_10_12_2023_becfd1d13_time.csv difference percentage
q1 42.30 42.54 0.240 100.57%
q2 24.55 24.58 0.035 100.14%
q3 37.50 37.68 0.182 100.48%
q4 40.69 41.71 1.014 102.49%
q5 69.85 69.95 0.098 100.14%
q6 5.96 5.44 -0.515 91.35%
q7 84.62 86.47 1.857 102.19%
q8 82.53 82.02 -0.505 99.39%
q9 114.35 119.24 4.891 104.28%
q10 49.68 48.00 -1.674 96.63%
q11 19.30 19.26 -0.042 99.78%
q12 24.02 24.58 0.562 102.34%
q13 49.09 52.68 3.596 107.33%
q14 17.87 15.32 -2.546 85.75%
q15 31.46 29.53 -1.922 93.89%
q16 15.71 16.15 0.447 102.84%
q17 121.88 121.78 -0.105 99.91%
q18 160.66 163.32 2.659 101.65%
q19 13.02 12.31 -0.712 94.53%
q20 29.60 27.43 -2.167 92.68%
q21 242.43 241.93 -0.500 99.79%
q22 15.76 15.40 -0.361 97.71%
total 1292.81 1297.34 4.532 100.35%

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

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

@zzcclp zzcclp zzcclp left review comments

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

@zhztheplayer zhztheplayer zhztheplayer 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.
6 participants
@loneylee @PHILO-HE @GlutenPerfBot @zzcclp @zhztheplayer @ulysses-you