[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-26 Thread cloud-fan
Github user cloud-fan commented on the issue:

https://github.com/apache/spark/pull/19324
  
a late LGTM :)


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-25 Thread gatorsmile
Github user gatorsmile commented on the issue:

https://github.com/apache/spark/pull/19324
  
Merged to master


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-25 Thread AmplabJenkins
Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/19324
  
Merged build finished. Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-25 Thread AmplabJenkins
Github user AmplabJenkins commented on the issue:

https://github.com/apache/spark/pull/19324
  
Test PASSed.
Refer to this link for build results (access rights to CI server needed): 
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/82153/
Test PASSed.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-25 Thread SparkQA
Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/19324
  
**[Test build #82153 has 
finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/82153/testReport)**
 for PR 19324 at commit 
[`ca64368`](https://github.com/apache/spark/commit/ca64368b369e2afa6b49e4cdcfa0a3d80704cadb).
 * This patch passes all tests.
 * This patch merges cleanly.
 * This patch adds no public classes.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-25 Thread SparkQA
Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/19324
  
**[Test build #82153 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/82153/testReport)**
 for PR 19324 at commit 
[`ca64368`](https://github.com/apache/spark/commit/ca64368b369e2afa6b49e4cdcfa0a3d80704cadb).


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-25 Thread gatorsmile
Github user gatorsmile commented on the issue:

https://github.com/apache/spark/pull/19324
  
retest this please


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-24 Thread gatorsmile
Github user gatorsmile commented on the issue:

https://github.com/apache/spark/pull/19324
  
LGTM


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-24 Thread viirya
Github user viirya commented on the issue:

https://github.com/apache/spark/pull/19324
  
@juliuszsompolski Thanks for pinging me.

#18931 is an attempt to separate the consume function as it can as 
possible. With long chain of any operators, you can have a long consume 
function and fail JIT, this is the one reason it tries to split into functions 
at the root of codegen support, instead of in some operators. I'd avoid to 
duplicate the separate logic in all operators, IMO.

For the explicit delaying evaluation of projection, currently the strategy 
I take is not going to split it. I guess that you mean the evaluation that can 
be delayed by the compiler, I personally think it should not be a big impact 
under the whole-stage codegen framework. The reason is those evaluation are 
actually needed and can't be avoided in most of (if not all) cases. From the 
benchmark we can see there is no negative impact even in the cases where no 
long consume function exists.

Yeah, I think the simplifies for the use of `continue` is a good thing, if 
it is possible.


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-22 Thread juliuszsompolski
Github user juliuszsompolski commented on the issue:

https://github.com/apache/spark/pull/19324
  
@viirya This is related to https://github.com/apache/spark/pull/18931/, as 
it also separates out the consume function. Maybe it would be enough to do 
similar splits into functions in the codegen of some operators that are 
materialization points (sort, joins) to keep the function length in check?
Splitting out on every `consume` takes away some of compiler's 
opportunities to optimize, like e.g. delaying evaluation of some projection 
(which you mentioned in your PR).
Removing the use of `continue` also simplifies not needing to handle it in 
your PR.



---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-22 Thread juliuszsompolski
Github user juliuszsompolski commented on the issue:

https://github.com/apache/spark/pull/19324
  
@hvanhovell @gatorsmile @cloud-fan @rednaxelafx


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org



[GitHub] spark issue #19324: [SPARK-22103] Move HashAggregateExec parent consume to a...

2017-09-22 Thread SparkQA
Github user SparkQA commented on the issue:

https://github.com/apache/spark/pull/19324
  
**[Test build #82088 has 
started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/82088/testReport)**
 for PR 19324 at commit 
[`ca64368`](https://github.com/apache/spark/commit/ca64368b369e2afa6b49e4cdcfa0a3d80704cadb).


---

-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org