You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by heary-cao <gi...@git.apache.org> on 2018/07/31 11:22:38 UTC
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
GitHub user heary-cao opened a pull request:
https://github.com/apache/spark/pull/21931
[SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row.max.capacity to configure the capacity of fast aggregation.
## What changes were proposed in this pull request?
this pr add a configuration parameter to configure the capacity of fast aggregation.
Performance comparison:
/*
Java HotSpot(TM) 64-Bit Server VM 1.8.0_60-b27 on Windows 7 6.1
Intel64 Family 6 Model 94 Stepping 3, GenuineIntel
Aggregate w multiple keys: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
------------------------------------------------------------------------------------------------
fasthash = default 5612 / 5882 3.7 267.6 1.0X
fasthash = config 3586 / 3595 5.8 171.0 1.6X
*/
## How was this patch tested?
the existed test cases.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/heary-cao/spark FastHashCapacity
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/21931.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 #21931
----
commit 46e6c4320f43c440fcf9ad8f1286b2b10d00ad00
Author: caoxuewen <ca...@...>
Date: 2018-07-31T11:00:41Z
Add a configuration parameter to configure the capacity of fast hash aggregate
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r211089716
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/AggregateBenchmark.scala ---
@@ -366,6 +366,43 @@ class AggregateBenchmark extends BenchmarkBase {
*/
}
+ ignore("capacity for fast hash aggregate") {
+ val N = 20 << 20
+ val M = 1 << 19
+
+ val benchmark = new Benchmark("Aggregate w multiple keys", N)
+ sparkSession.range(N)
+ .selectExpr(
+ "id",
+ s"(id % $M) as k1",
+ s"cast(id % $M as int) as k2",
+ s"cast(id % $M as double) as k3",
+ s"cast(id % $M as float) as k4") .createOrReplaceTempView("test")
+
+ def f(): Unit = sparkSession.sql("select k1, k2, k3, k4, sum(k1), sum(k2), sum(k3), " +
+ "sum(k4) from test group by k1, k2, k3, k4").collect()
+
+ benchmark.addCase(s"fasthash = default") { iter =>
--- End diff --
"fasthash = 16"?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/21931
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on the issue:
https://github.com/apache/spark/pull/21931
cc @cloud-fan @hvanhovell
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r211089705
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/AggregateBenchmark.scala ---
@@ -366,6 +366,43 @@ class AggregateBenchmark extends BenchmarkBase {
*/
}
+ ignore("capacity for fast hash aggregate") {
+ val N = 20 << 20
+ val M = 1 << 19
+
+ val benchmark = new Benchmark("Aggregate w multiple keys", N)
+ sparkSession.range(N)
+ .selectExpr(
+ "id",
+ s"(id % $M) as k1",
+ s"cast(id % $M as int) as k2",
+ s"cast(id % $M as double) as k3",
+ s"cast(id % $M as float) as k4") .createOrReplaceTempView("test")
+
+ def f(): Unit = sparkSession.sql("select k1, k2, k3, k4, sum(k1), sum(k2), sum(k3), " +
+ "sum(k4) from test group by k1, k2, k3, k4").collect()
+
+ benchmark.addCase(s"fasthash = default") { iter =>
+ sparkSession.conf.set("spark.sql.codegen.aggregate.fastHashMap.capacityBit", "16")
+ f()
+ }
+
+ benchmark.addCase(s"fasthash = config") { iter =>
--- End diff --
"fasthash = 20"?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21931
What does the benchmark result suggest? The result should be `1048576` by default?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21931
thanks, merging to master!
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95157 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95157/testReport)** for PR 21931 at commit [`af2078c`](https://github.com/apache/spark/commit/af2078cacdd17ec8f30b270c95aed469aaa08e8b).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r212284374
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1444,6 +1444,31 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.codegen.aggregate.fastHashMap.capacityBit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator. The bit is not for actual value, but the actual numBuckets is determined " +
+ "by loadFactor (e.g: default bit value 16 , the actual numBuckets is ((1 << 16) / 0.5).")
+ .intConf
+ .checkValue(bit => bit >= 10 && bit <= 30, "The bit value must be in [10, 30].")
--- End diff --
I am not sure about the minimum value. The initial configuration is 1. but I agree with @maropu suggestion. e.g., 2^1, 2^2,these are meaningless.
for the maximum is 30, because the integer bit is 31 and the actual numBuckets in fast hash map are determined by loadFactor and loadFactor is 0.5 too.
code is:
private double loadFactor = 0.5;
private int numBuckets = (int) (capacity / loadFactor);
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r207803961
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1437,6 +1437,15 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.fast.hash.aggregate.row.max.capacity.bit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator (e.g: configuration 16 capacity size is 65536).")
--- End diff --
Can we describe something about this value? (e.g. `bit` not for `value`).
Can we describe about the default value?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r208561224
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1437,6 +1437,16 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.codegen.aggregate.map.row.capacitybit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator. the bit not for actual value, but the actual numBuckets is determined " +
+ "by loadFactor (e.g: default bit value 16 , the actual numBuckets is ((1 >> 16) / 0.5).")
--- End diff --
nit: `1 >> 16` -> `1 << 16`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on the issue:
https://github.com/apache/spark/pull/21931
cc @maropu
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21931
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r212282015
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1444,6 +1444,31 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.codegen.aggregate.fastHashMap.capacityBit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator. The bit is not for actual value, but the actual numBuckets is determined " +
+ "by loadFactor (e.g: default bit value 16 , the actual numBuckets is ((1 << 16) / 0.5).")
+ .intConf
+ .checkValue(bit => bit >= 10 && bit <= 30, "The bit value must be in [10, 30].")
+ .createWithDefault(16)
+
+ object AvroOutputTimestampType extends Enumeration {
--- End diff --
I'm sorry, update it. thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95154 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95154/testReport)** for PR 21931 at commit [`03c0a80`](https://github.com/apache/spark/commit/03c0a806c6bcaf7553cf65f0aba73ba5c13fc56a).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95107/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r211991751
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/AggregateBenchmark.scala ---
@@ -366,6 +366,43 @@ class AggregateBenchmark extends BenchmarkBase {
*/
}
+ ignore("Capacity for fast hash aggregate") {
--- End diff --
I don't see why we need to add benchmark, we just make the capacity configurable.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r208832383
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1437,6 +1437,16 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.codegen.aggregate.map.row.capacitybit")
--- End diff --
spark.sql.codegen.aggregate.fastHashMap.capacityBit?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95190/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r207802603
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1437,6 +1437,15 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.fast.hash.aggregate.row.max.capacity.bit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator (e.g: configuration 16 capacity size is 65536).")
+ .intConf
+ .checkValue(bit => bit >= 1 && bit <= 30, "The bit value must be in [1, 30].")
--- End diff --
We need to accept these small values, e.g., 2^1, 2^2, ..? I think these are meaningless...
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r208561981
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/AggregateBenchmark.scala ---
@@ -366,6 +366,43 @@ class AggregateBenchmark extends BenchmarkBase {
*/
}
+ ignore("capacity for fast hash aggregate") {
+ val N = 20 << 20
+ val M = 1 << 19
+
+ val benchmark = new Benchmark("Aggregate w multiple keys", N)
+ sparkSession.range(N)
+ .selectExpr(
+ "id",
+ s"(id % $M) as k1",
+ s"cast(id % $M as int) as k2",
+ s"cast(id % $M as double) as k3",
+ s"cast(id % $M as float) as k4") .createOrReplaceTempView("test")
+
+ def f(): Unit = sparkSession.sql("select k1, k2, k3, k4, sum(k1), sum(k2), sum(k3), " +
+ "sum(k4) from test group by k1, k2, k3, k4").collect()
+
+ benchmark.addCase(s"fasthash = default") { iter =>
+ sparkSession.conf.set("spark.sql.codegen.aggregate.map.row.capacitybit", "16")
+ f()
+ }
+
+ benchmark.addCase(s"fasthash = config") { iter =>
+ sparkSession.conf.set("spark.sql.codegen.aggregate.map.row.capacitybit", "20")
+ f()
+ }
+
+ benchmark.run()
+
+ /*
+ Java HotSpot(TM) 64-Bit Server VM 1.8.0_60-b27 on Windows 7 6.1
+ Intel64 Family 6 Model 94 Stepping 3, GenuineIntel
+ Aggregate w multiple keys: Best/Avg Time(ms) Rate(M/s) Per Row(ns) Relative
--- End diff --
nit: we need to reduce # of characters per line up to 100. IIUC, the number is more than 100.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by maropu <gi...@git.apache.org>.
Github user maropu commented on the issue:
https://github.com/apache/spark/pull/21931
cc: @cloud-fan @hvanhovell
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on the issue:
https://github.com/apache/spark/pull/21931
@kiszk, Thank you for your suggestion. I have update it. Can you review it again if you have some time. thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21931
Does this work when we set `30` into the parameter? I am afraid that several arrays with size `0x7fffffff` are allocated.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r207803415
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1437,6 +1437,15 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.fast.hash.aggregate.row.max.capacity.bit")
--- End diff --
This name looks too long. How about `spark.sql.codegen.aggregate.map.row.capacitybit` or others?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95154 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95154/testReport)** for PR 21931 at commit [`03c0a80`](https://github.com/apache/spark/commit/03c0a806c6bcaf7553cf65f0aba73ba5c13fc56a).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by holdensmagicalunicorn <gi...@git.apache.org>.
Github user holdensmagicalunicorn commented on the issue:
https://github.com/apache/spark/pull/21931
@heary-cao, thanks! I am a bot who has found some folks who might be able to help with the review:@rxin, @cloud-fan and @yhuai
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95190 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95190/testReport)** for PR 21931 at commit [`6abeb06`](https://github.com/apache/spark/commit/6abeb06e52ed51ff9a30acc5801b0794ce778e2c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95107 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95107/testReport)** for PR 21931 at commit [`5ed0fda`](https://github.com/apache/spark/commit/5ed0fda0cdba3ecf86a2815bc083c2307466a4d4).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r211089695
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1437,6 +1437,16 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.codegen.aggregate.fastHashMap.capacityBit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator. the bit not for actual value, but the actual numBuckets is determined " +
--- End diff --
`the bit` -> `The bit is`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on the issue:
https://github.com/apache/spark/pull/21931
@maropu, The test results show that we can make a configuration parameter for the capacity of fast hash. Currently capacity of our fast hash is related to the length of the recorded data. so I'm not sure how much the default value is configured, but it is unreasonable to configure it in CodeGen as a fixed value(int capacity = 1 << 16;).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r211990712
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1444,6 +1444,31 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.codegen.aggregate.fastHashMap.capacityBit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator. The bit is not for actual value, but the actual numBuckets is determined " +
+ "by loadFactor (e.g: default bit value 16 , the actual numBuckets is ((1 << 16) / 0.5).")
+ .intConf
+ .checkValue(bit => bit >= 10 && bit <= 30, "The bit value must be in [10, 30].")
+ .createWithDefault(16)
+
+ object AvroOutputTimestampType extends Enumeration {
--- End diff --
why add it?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95107 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95107/testReport)** for PR 21931 at commit [`5ed0fda`](https://github.com/apache/spark/commit/5ed0fda0cdba3ecf86a2815bc083c2307466a4d4).
* This patch **fails Scala style tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r211089742
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/benchmark/AggregateBenchmark.scala ---
@@ -366,6 +366,43 @@ class AggregateBenchmark extends BenchmarkBase {
*/
}
+ ignore("capacity for fast hash aggregate") {
+ val N = 20 << 20
+ val M = 1 << 19
+
+ val benchmark = new Benchmark("Aggregate w multiple keys", N)
--- End diff --
`Benchmark("Capacity for fast hash aggregate")`
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/21931
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95154/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21931
LGTM, cc @cloud-fan @hvanhovell
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95157 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95157/testReport)** for PR 21931 at commit [`af2078c`](https://github.com/apache/spark/commit/af2078cacdd17ec8f30b270c95aed469aaa08e8b).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/21931
**[Test build #95190 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95190/testReport)** for PR 21931 at commit [`6abeb06`](https://github.com/apache/spark/commit/6abeb06e52ed51ff9a30acc5801b0794ce778e2c).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on the issue:
https://github.com/apache/spark/pull/21931
@kiszk ,I'm not sure how much the maximum is set, and the size of 1G is the maximum value accepted by numBuckets. Of course, buckets is the memory of 8G.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95157/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r207519280
--- Diff: sql/core/src/main/scala/org/apache/spark/sql/execution/aggregate/VectorizedHashMapGenerator.scala ---
@@ -83,7 +84,7 @@ class VectorizedHashMapGenerator(
| private ${classOf[ColumnarBatch].getName} batch;
| private ${classOf[MutableColumnarRow].getName} aggBufferRow;
| private int[] buckets;
- | private int capacity = 1 << 16;
+ | private int capacity = $maxCapacity;
--- End diff --
We can see the following code at L226. If a user specify `2^n` value (e.g. 1024), it works functionally correct. What happens if a user specified non `2^n` value (e.g. 127)?
```
idx = (idx + 1) & (numBuckets - 1);
```
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Merged build finished. Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by kiszk <gi...@git.apache.org>.
Github user kiszk commented on the issue:
https://github.com/apache/spark/pull/21931
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/21931
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on the issue:
https://github.com/apache/spark/pull/21931
cc @cloud-fan @hvanhovell
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggrega...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on a diff in the pull request:
https://github.com/apache/spark/pull/21931#discussion_r211992125
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/internal/SQLConf.scala ---
@@ -1444,6 +1444,31 @@ object SQLConf {
.intConf
.createWithDefault(20)
+ val FAST_HASH_AGGREGATE_MAX_ROWS_CAPACITY_BIT =
+ buildConf("spark.sql.codegen.aggregate.fastHashMap.capacityBit")
+ .internal()
+ .doc("Capacity for the max number of rows to be held in memory by the fast hash aggregate " +
+ "product operator. The bit is not for actual value, but the actual numBuckets is determined " +
+ "by loadFactor (e.g: default bit value 16 , the actual numBuckets is ((1 << 16) / 0.5).")
+ .intConf
+ .checkValue(bit => bit >= 10 && bit <= 30, "The bit value must be in [10, 30].")
--- End diff --
can you explain how to calculate this valid range?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by heary-cao <gi...@git.apache.org>.
Github user heary-cao commented on the issue:
https://github.com/apache/spark/pull/21931
@kiszk , @cloud-fan
I was on vacation some time ago, I'm sorry to delayed reply.
I have update it, Can you help to review it again if your have some times. thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #21931: [SPARK-24978][SQL]Add spark.sql.fast.hash.aggregate.row....
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on the issue:
https://github.com/apache/spark/pull/21931
Minor comments. LGTM.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org