[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user asfgit closed the pull request at: https://github.com/apache/spark/pull/21705 --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user kiszk commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199986545 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/ExecutorSideSQLConfSuite.scala --- @@ -63,4 +71,15 @@ class ExecutorSideSQLConfSuite extends SparkFunSuite with SQLTestUtils { } } } + + test("SPARK-24727 CODEGEN_CACHE_SIZE is correctly referenced at the executor side") { --- End diff -- very nit: `CODEGEN_CACHE_SIZE` -> `CODEGEN_CACHE_MAX_ENTRIES` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user rxin commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199940775 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala --- @@ -66,6 +66,12 @@ object StaticSQLConf { .checkValue(cacheSize => cacheSize >= 0, "The maximum size of the cache must not be negative") .createWithDefault(1000) + val CODEGEN_CACHE_SIZE = buildStaticConf("spark.sql.codegen.cacheSize") + .doc("The maximum cache size for generated classes.") --- End diff -- spark.sql.codegen.cache.maxEntries? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user gatorsmile commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199935428 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala --- @@ -66,6 +66,12 @@ object StaticSQLConf { .checkValue(cacheSize => cacheSize >= 0, "The maximum size of the cache must not be negative") .createWithDefault(1000) + val CODEGEN_CACHE_SIZE = buildStaticConf("spark.sql.codegen.cacheSize") + .doc("The maximum cache size for generated classes.") + .intConf --- End diff -- this is internal, right? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user gatorsmile commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199936887 --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/StaticSQLConf.scala --- @@ -66,6 +66,12 @@ object StaticSQLConf { .checkValue(cacheSize => cacheSize >= 0, "The maximum size of the cache must not be negative") .createWithDefault(1000) + val CODEGEN_CACHE_SIZE = buildStaticConf("spark.sql.codegen.cacheSize") + .doc("The maximum cache size for generated classes.") --- End diff -- We need to explain what is the cache is. It is hard to tell based on this description. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user cloud-fan commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199858281 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/ExecutorSideSQLConfSuite.scala --- @@ -40,16 +40,24 @@ class ExecutorSideSQLConfSuite extends SparkFunSuite with SQLTestUtils { spark = null } + private def withStaticSQLConf(pairs: (String, String)*)(f: => Unit): Unit = { --- End diff -- ah sorry for the confusion about "call", I mean rename this method... --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199820857 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/ExecutorSideSQLConfSuite.scala --- @@ -40,16 +40,24 @@ class ExecutorSideSQLConfSuite extends SparkFunSuite with SQLTestUtils { spark = null } + private def withStaticSQLConf(pairs: (String, String)*)(f: => Unit): Unit = { --- End diff -- In case of `withSQLConf`, I got an exception; ``` - SPARK-24727 CODEGEN_CACHE_SIZE is correctly referenced at the executor side *** FAILED *** org.apache.spark.sql.AnalysisException: Cannot modify the value of a static config: spark.sql.codegen.cacheSize; at org.apache.spark.sql.catalyst.plans.PlanTestBase$$anonfun$withSQLConf$1.apply(PlanTest.scala:172) ``` --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user maropu commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199817849 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/ExecutorSideSQLConfSuite.scala --- @@ -40,16 +40,24 @@ class ExecutorSideSQLConfSuite extends SparkFunSuite with SQLTestUtils { spark = null } + private def withStaticSQLConf(pairs: (String, String)*)(f: => Unit): Unit = { --- End diff -- will check --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
Github user cloud-fan commented on a diff in the pull request: https://github.com/apache/spark/pull/21705#discussion_r199817343 --- Diff: sql/core/src/test/scala/org/apache/spark/sql/internal/ExecutorSideSQLConfSuite.scala --- @@ -40,16 +40,24 @@ class ExecutorSideSQLConfSuite extends SparkFunSuite with SQLTestUtils { spark = null } + private def withStaticSQLConf(pairs: (String, String)*)(f: => Unit): Unit = { --- End diff -- ah sorry I was wrong. Static conf is no different from normal conf, it's just immutable during runtime. Maybe just call this method `withSQLConf`? --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #21705: [SPARK-24727][SQL] Add a static config to control...
GitHub user maropu opened a pull request: https://github.com/apache/spark/pull/21705 [SPARK-24727][SQL] Add a static config to control cache size for generated classes ## What changes were proposed in this pull request? Since SPARK-24250 has been resolved, executors correctly references user-defined configurations. So, this pr added a static config to control cache size for generated classes in `CodeGenerator`. ## How was this patch tested? Manually checked that executors referenced `spark.sql.cacheSize` correctly. You can merge this pull request into a Git repository by running: $ git pull https://github.com/maropu/spark SPARK-24727 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/spark/pull/21705.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #21705 commit 8ee8e00f156e577b32b01d015b8bd24f72ae7340 Author: Takeshi Yamamuro Date: 2018-07-03T13:14:35Z Fix --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org