[SPARK-50157][SQL] Using SQLConf provided by SparkSession first. #48693

beliefer · 2024-10-29T10:43:03Z

What changes were proposed in this pull request?

This PR proposes to use SQLConf provided by SparkSession first.

Why are the changes needed?

SQLConf provided by SparkSession have better perf than SQLConf.get.

Does this PR introduce any user-facing change?

'No'.

How was this patch tested?

GA tests.
The benchmark test.

object SQLConfBenchmark extends SqlBasedBenchmark {
  override def runBenchmarkSuite(mainArgs: Array[String]): Unit = {
    runBenchmark("Get SQLConf") {
      val iters = 1000
      val benchmark = new Benchmark("Benchmark SQLConf", iters, output = output)
      benchmark.addCase("SQLConf.get") { _ =>
        for (_ <- 1 to iters) {
          val conf = SQLConf.get
        }
      }

      benchmark.addCase("sessionState.conf") { _ =>
        for (_ <- 1 to iters) {
          val conf = spark.sessionState.conf
        }
      }

      benchmark.run()
    }
  }
}

The benchmark output.

Benchmark SQLConf:                        Best Time(ms)   Avg Time(ms)   Stdev(ms)    Rate(M/s)   Per Row(ns)   Relative
------------------------------------------------------------------------------------------------------------------------
SQLConf.get                                           0              0           1         22.9          43.6       1.0X
sessionState.conf                                     0              0           0       1377.4           0.7      60.1X

Was this patch authored or co-authored using generative AI tooling?

'No'.

dongjoon-hyun

It would be great if you can provide some concrete examples or numbers with this, @beliefer .

In fact, SQLConf.get and spark.sessionState.conf are functionally equivalent. But spark.sessionState.conf is faster than SQLConf.get because the latter requires visit thread local. So we can see the usage scenarios:

The code rely on SQLConfHelper in Spark we can't find SparkSession in context.
All the places in Spark rely on spark.sessionState.conf if exists SparkSession.

It was hard to provide some perf number for me, the perf improvement looks very inconspicuous after all.

dongjoon-hyun · 2024-10-29T17:26:03Z

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/HiveQuerySuite.scala

@@ -231,7 +231,7 @@ class HiveQuerySuite extends HiveComparisonTest with SQLTestUtils with BeforeAnd
  createQueryTest("no from clause",
    "SELECT 1, +1, -1")

-  if (!conf.ansiEnabled) {
+  if (!spark.sessionState.conf.ansiEnabled) {


Kindly revert all modifications made to this test suite, as the test code is inconsequential to the performance evaluation.

beliefer · 2024-11-01T03:12:27Z

@dongjoon-hyun Please take a look again. Thanks.

HyukjinKwon · 2024-11-01T03:42:27Z

How much does it get improved?

beliefer · 2024-11-01T11:51:02Z

How much does it get improved?

The benchmark added into PR's description.

dongjoon-hyun

This PR technically proposed to avoid the use of SQLConfHelper.conf method, right?

If that method is useless, can we remove the method? Or, when do we need to use that method still?

beliefer · 2024-11-03T09:01:17Z

If that method is useless, can we remove the method? Or, when do we need to use that method still?

AFAIK, we need SQLConfHelper.conf in Spark if we can't find SparkSession in context.

HyukjinKwon · 2024-11-03T20:58:37Z

Does the benchmark result say session.conf is slower than SQLConf.get?

beliefer · 2024-11-04T03:08:20Z

Does the benchmark result say session.conf is slower than SQLConf.get?

session.conf is quicker than SQLConf.get.

cloud-fan · 2024-11-11T13:39:00Z

In general I think it's better to use the SQLConf in SparkSession if it's available. Maybe we should just override def conf in those rules that have SparkSession available

beliefer · 2024-11-13T11:21:27Z

@cloud-fan override def conf finished, do you have any other comments?

beliefer · 2024-12-05T05:01:25Z

@dongjoon-hyun @HyukjinKwon The GA failure seems unrelated. Please take a look again.

LuciferYang · 2024-12-12T11:39:08Z

sql/core/src/main/scala/org/apache/spark/sql/execution/adaptive/CoalesceShufflePartitions.scala

@@ -33,6 +33,8 @@ import org.apache.spark.util.Utils
 */
 case class CoalesceShufflePartitions(session: SparkSession) extends AQEShuffleReadRule {

+  override def conf: SQLConf = session.sessionState.conf


Can we change it to override val conf: SQLConf = session.sessionState.conf in cases where conf needs to be called multiple times?

From the test results, there are other issues with changing to val. It seems we need to revert the last commit.

Yes. I think so.

sorry for misleading

LuciferYang

+1, LGTM

beliefer · 2024-12-13T11:10:07Z

@LuciferYang @dongjoon-hyun @cloud-fan @HyukjinKwon Thank you all.

### What changes were proposed in this pull request? This PR proposes to use `SQLConf` provided by `SparkSession` first. ### Why are the changes needed? `SQLConf` provided by `SparkSession` have better perf than `SQLConf.get`. ### Does this PR introduce _any_ user-facing change? 'No'. ### How was this patch tested? GA tests. The benchmark test. ``` object SQLConfBenchmark extends SqlBasedBenchmark { override def runBenchmarkSuite(mainArgs: Array[String]): Unit = { runBenchmark("Get SQLConf") { val iters = 1000 val benchmark = new Benchmark("Benchmark SQLConf", iters, output = output) benchmark.addCase("SQLConf.get") { _ => for (_ <- 1 to iters) { val conf = SQLConf.get } } benchmark.addCase("sessionState.conf") { _ => for (_ <- 1 to iters) { val conf = spark.sessionState.conf } } benchmark.run() } } } ``` The benchmark output. ``` Benchmark SQLConf: Best Time(ms) Avg Time(ms) Stdev(ms) Rate(M/s) Per Row(ns) Relative ------------------------------------------------------------------------------------------------------------------------ SQLConf.get 0 0 1 22.9 43.6 1.0X sessionState.conf 0 0 0 1377.4 0.7 60.1X ``` ### Was this patch authored or co-authored using generative AI tooling? 'No'. Closes apache#48693 from beliefer/SPARK-50157. Authored-by: beliefer <[email protected]> Signed-off-by: beliefer <[email protected]>

github-actions bot added the SQL label Oct 29, 2024

dongjoon-hyun reviewed Oct 29, 2024

View reviewed changes

beliefer force-pushed the SPARK-50157 branch from 37cb218 to 034ddc2 Compare October 30, 2024 04:23

dongjoon-hyun reviewed Nov 3, 2024

View reviewed changes

beliefer force-pushed the SPARK-50157 branch from fc15ef3 to db65e8b Compare November 12, 2024 05:38

beliefer force-pushed the SPARK-50157 branch from db65e8b to d41be9d Compare November 22, 2024 04:00

beliefer force-pushed the SPARK-50157 branch from d41be9d to 15018fb Compare December 4, 2024 08:48

beliefer added 2 commits December 6, 2024 15:59

[SPARK-50157][SQL] Using SQLConf provided by SparkSession first.

ac99d71

Revert HiveQuerySuite

ab740a1

beliefer force-pushed the SPARK-50157 branch from 15018fb to 1764de8 Compare December 6, 2024 08:00

LuciferYang reviewed Dec 12, 2024

View reviewed changes

beliefer force-pushed the SPARK-50157 branch from 1764de8 to d02ef58 Compare December 12, 2024 12:36

LuciferYang approved these changes Dec 12, 2024

View reviewed changes

override def conf

79a9445

beliefer force-pushed the SPARK-50157 branch from d02ef58 to 79a9445 Compare December 13, 2024 03:10

LuciferYang approved these changes Dec 13, 2024

View reviewed changes

beliefer closed this in 819bac9 Dec 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-50157][SQL] Using SQLConf provided by SparkSession first. #48693

[SPARK-50157][SQL] Using SQLConf provided by SparkSession first. #48693

beliefer commented Oct 29, 2024 •

edited

Loading

dongjoon-hyun left a comment •

edited by beliefer

Loading

dongjoon-hyun Oct 29, 2024

beliefer commented Nov 1, 2024 •

edited

Loading

HyukjinKwon commented Nov 1, 2024

beliefer commented Nov 1, 2024

dongjoon-hyun left a comment

beliefer commented Nov 3, 2024

HyukjinKwon commented Nov 3, 2024

beliefer commented Nov 4, 2024

cloud-fan commented Nov 11, 2024 •

edited

Loading

beliefer commented Nov 13, 2024

beliefer commented Dec 5, 2024

LuciferYang Dec 12, 2024

LuciferYang Dec 12, 2024

beliefer Dec 13, 2024

LuciferYang Dec 13, 2024

LuciferYang left a comment

beliefer commented Dec 13, 2024

[SPARK-50157][SQL] Using SQLConf provided by SparkSession first. #48693

[SPARK-50157][SQL] Using SQLConf provided by SparkSession first. #48693

Conversation

beliefer commented Oct 29, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Was this patch authored or co-authored using generative AI tooling?

dongjoon-hyun left a comment • edited by beliefer Loading

Choose a reason for hiding this comment

dongjoon-hyun Oct 29, 2024

Choose a reason for hiding this comment

beliefer commented Nov 1, 2024 • edited Loading

HyukjinKwon commented Nov 1, 2024

beliefer commented Nov 1, 2024

dongjoon-hyun left a comment

Choose a reason for hiding this comment

beliefer commented Nov 3, 2024

HyukjinKwon commented Nov 3, 2024

beliefer commented Nov 4, 2024

cloud-fan commented Nov 11, 2024 • edited Loading

beliefer commented Nov 13, 2024

beliefer commented Dec 5, 2024

LuciferYang Dec 12, 2024

Choose a reason for hiding this comment

LuciferYang Dec 12, 2024

Choose a reason for hiding this comment

beliefer Dec 13, 2024

Choose a reason for hiding this comment

LuciferYang Dec 13, 2024

Choose a reason for hiding this comment

LuciferYang left a comment

Choose a reason for hiding this comment

beliefer commented Dec 13, 2024

beliefer commented Oct 29, 2024 •

edited

Loading

dongjoon-hyun left a comment •

edited by beliefer

Loading

beliefer commented Nov 1, 2024 •

edited

Loading

cloud-fan commented Nov 11, 2024 •

edited

Loading