You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by cloud-fan <gi...@git.apache.org> on 2016/09/13 17:54:54 UTC
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/15082
[SPARK-17528][SQL] MutableProjection should not cache content from the input row
## What changes were proposed in this pull request?
For performance reasons, `UnsafeRow.getString`, `getStruct`, etc. return a "pointer" that points to a memory region of this unsafe row. This makes the unsafe projection a little dangerous, because all of its output rows share one instance.
When we implement SQL operators, we should be careful to not cache the input rows because they may be produced by unsafe projection from child operator and thus its content may change overtime.
However, `GenerateMutableProjection` breaks this and may cache the content in input rows. The sort based aggregate use it, but this bug is not exposed because sort based aggregate always do an extra projection for the input row.
This PR fixes the bug of `GenerateMutableProjection` and some related bugs in complex data copy, and remove the useless projection in sort based aggregate.
## How was this patch tested?
some new tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark agg-bug
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/15082.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 #15082
----
commit 75b57490d1be2ca4d7ca88bcd4d6d9461d470ce8
Author: Wenchen Fan <we...@databricks.com>
Date: 2016-09-13T17:40:16Z
MutableProjection should not cache content from the input row
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/15082
After thinking more about it, it seems not a problem and we don't need to fix it. Currently `MutableProjection` is used in 3 places:
1. hash based aggregate. It's fine because hash aggregate only supports primitive type buffer, so it doesn't have the copy problem.
2. sort based aggregate. It's also fine because we always apply a safe projection to the input row before process it. I have a PR to fix the comment of it: https://github.com/apache/spark/pull/15095
3. window execution. It's also fine because its input rows are come from `RowBuffer`, not unsafe rows produced by unsafe projection.
This PR tried to fix `MutableProjection` so that we can remove the safe projection in sort based aggregate, but I was wrong because `MutableProjection` is not the only way to update the aggregation buffer, `ImperativeAggregate` can also update the buffer via the `update` and `merge` API, and it's not a good idea to fix all `ImperativeAggregate` to do copy correctly while updating the aggregate buffer(and it's not future proof, we may add more `ImperativeAggragate` implementations and miss this requirement).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/15082
cc @davies @yhuai @liancheng @clockfly
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65386/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65561/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65328 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65328/consoleFull)** for PR 15082 at commit [`75b5749`](https://github.com/apache/spark/commit/75b57490d1be2ca4d7ca88bcd4d6d9461d470ce8).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/15082
retest this please
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65561 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65561/consoleFull)** for PR 15082 at commit [`da6a285`](https://github.com/apache/spark/commit/da6a2859c8fe0c90b4aa14a68c2234e6ede36fce).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/15082
What's the status of this issue? I see that it's currently targeted for 2.0.1 in JIRA and wanted to ping to make sure that this doesn't miss the cut in case we prepare an RC soon.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65807/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on a diff in the pull request:
https://github.com/apache/spark/pull/15082#discussion_r78667613
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
@@ -276,6 +276,11 @@ class CodegenContext {
case t: DecimalType => s"$row.setDecimal($ordinal, $value, ${t.precision})"
// The UTF8String may came from UnsafeRow, otherwise clone is cheap (re-use the bytes)
case StringType => s"$row.update($ordinal, $value.clone())"
+ // InternalRow, ArrayData and MapData may came from UnsafeRow, we should copy it to avoid
+ // keeping a "pointer" to a memory region which is out of control from the updated row.
+ case _: StructType => s"$row.update($ordinal, $value.copy())"
--- End diff --
If `$row` could do the copy inside `update`, then we do need to do the copy here, right?
Maybe it's time to check all the MutableRow, MutableProjection, to see where is the best place to do the copy.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/15082#discussion_r79029655
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/RowTest.scala ---
@@ -121,10 +121,6 @@ class RowTest extends FunSpec with Matchers {
externalRow should be theSameInstanceAs externalRow.copy()
}
- it("copy should return same ref for internal rows") {
--- End diff --
As discussed offline: I don't see the point of having an immutable `GenericInternalRow` if we cannot guarantee its immutability. We could just make every InternalRow a mutable one, and simplify the class structure in the process. I am not sure if we should make that part of the current PR though.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/15082#discussion_r78915332
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/RowTest.scala ---
@@ -121,10 +121,6 @@ class RowTest extends FunSpec with Matchers {
externalRow should be theSameInstanceAs externalRow.copy()
}
- it("copy should return same ref for internal rows") {
--- End diff --
Copy returned the same ref because it is supposed to be immutable. See https://github.com/apache/spark/pull/10553 for more context.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65418/
Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65386 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65386/consoleFull)** for PR 15082 at commit [`f83e291`](https://github.com/apache/spark/commit/f83e2916c7da09be90c1cfad5387bab51512b0f0).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Merged build finished. Test PASSed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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/15082#discussion_r78940604
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/RowTest.scala ---
@@ -121,10 +121,6 @@ class RowTest extends FunSpec with Matchers {
externalRow should be theSameInstanceAs externalRow.copy()
}
- it("copy should return same ref for internal rows") {
--- End diff --
As long as `InternalRow` can have mutable implementation, `InternalRow` is not immutable anymore, because it can have a struct field, whose value can be a `MutableRow`.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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/15082#discussion_r78611782
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/SpecificMutableRow.scala ---
@@ -225,10 +227,20 @@ final class SpecificMutableRow(val values: Array[MutableValue])
val newValues = new Array[Any](values.length)
var i = 0
while (i < values.length) {
- newValues(i) = values(i).boxed
+ val value = values(i).boxed
+ if (value.isInstanceOf[UTF8String]) {
+ newValues(i) = value.asInstanceOf[UTF8String].clone()
+ } else if (value.isInstanceOf[InternalRow]) {
+ newValues(i) = value.asInstanceOf[InternalRow].copy()
+ } else if (value.isInstanceOf[ArrayData]) {
+ newValues(i) = value.asInstanceOf[ArrayData].copy()
+ } else if (value.isInstanceOf[MapData]) {
+ newValues(i) = value.asInstanceOf[MapData].copy()
+ } else {
+ newValues(i) = value
--- End diff --
this code is duplicated 4 times, should we introduce a trait for it? (I'm not able to create a util function for it because these codes differ a little)
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/65328/
Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan closed the pull request at:
https://github.com/apache/spark/pull/15082
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65807 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65807/consoleFull)** for PR 15082 at commit [`c56de6d`](https://github.com/apache/spark/commit/c56de6da72c18b2cd1f65eed956cdee89371b075).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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/15082#discussion_r78927126
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/RowTest.scala ---
@@ -121,10 +121,6 @@ class RowTest extends FunSpec with Matchers {
externalRow should be theSameInstanceAs externalRow.copy()
}
- it("copy should return same ref for internal rows") {
--- End diff --
ah got it, but it's not true for internal row right? It can be mutable so it's safe to remove this test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65418 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65418/consoleFull)** for PR 15082 at commit [`da6a285`](https://github.com/apache/spark/commit/da6a2859c8fe0c90b4aa14a68c2234e6ede36fce).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65386 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65386/consoleFull)** for PR 15082 at commit [`f83e291`](https://github.com/apache/spark/commit/f83e2916c7da09be90c1cfad5387bab51512b0f0).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65328 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65328/consoleFull)** for PR 15082 at commit [`75b5749`](https://github.com/apache/spark/commit/75b57490d1be2ca4d7ca88bcd4d6d9461d470ce8).
* This patch **fails Spark unit tests**.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65807 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65807/consoleFull)** for PR 15082 at commit [`c56de6d`](https://github.com/apache/spark/commit/c56de6da72c18b2cd1f65eed956cdee89371b075).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan closed the pull request at:
https://github.com/apache/spark/pull/15082
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/15082
After some more discussion, I'm going to reopen it:
1. `InternalRow.copy` breaks the copy contract and the fix in this PR is necessary
2. `MutableProjection` may bite us in the future if we don't fix it
3. It turns out `Collect` is the only `ImperativeAggregate` that we need to fix while updating aggregation buffer, so it's not a lot of work.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/15082
Merged build finished. Test FAILed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by liancheng <gi...@git.apache.org>.
Github user liancheng commented on the issue:
https://github.com/apache/spark/pull/15082
My understanding of the main concern of closing this PR is that:
1. Although this issue can be potentially dangerous, the current code work fine without fixing this issue.
1. We still can't remove the safe projection in `SortAggregateExec` after fixing this issue (as @cloud-fan explained above), thus data may be copied twice and leads to performance regression.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/15082#discussion_r78615957
--- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/SpecificMutableRow.scala ---
@@ -225,10 +227,20 @@ final class SpecificMutableRow(val values: Array[MutableValue])
val newValues = new Array[Any](values.length)
var i = 0
while (i < values.length) {
- newValues(i) = values(i).boxed
+ val value = values(i).boxed
+ if (value.isInstanceOf[UTF8String]) {
+ newValues(i) = value.asInstanceOf[UTF8String].clone()
+ } else if (value.isInstanceOf[InternalRow]) {
+ newValues(i) = value.asInstanceOf[InternalRow].copy()
+ } else if (value.isInstanceOf[ArrayData]) {
+ newValues(i) = value.asInstanceOf[ArrayData].copy()
+ } else if (value.isInstanceOf[MapData]) {
+ newValues(i) = value.asInstanceOf[MapData].copy()
+ } else {
+ newValues(i) = value
--- End diff --
We could move this into a util function right:
```scala
def copyValue(value: Any): Any = value match {
case v: UTF8String => v.clone()
case v: InternalRow => v.copy()
case v: ArrayData => v.copy()
case v: MapData => v.copy()
case v => v
}
```
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by davies <gi...@git.apache.org>.
Github user davies commented on the issue:
https://github.com/apache/spark/pull/15082
@JoshRosen I think this is a potential bug (not now).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by JoshRosen <gi...@git.apache.org>.
Github user JoshRosen commented on the issue:
https://github.com/apache/spark/pull/15082
Is this a "wrong answer" correctness bug? If so, let me label accordingly on the JIRA.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65418 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65418/consoleFull)** for PR 15082 at commit [`da6a285`](https://github.com/apache/spark/commit/da6a2859c8fe0c90b4aa14a68c2234e6ede36fce).
* This patch passes all tests.
* This patch merges cleanly.
* This patch adds no public classes.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/15082
I re-targeted it to 2.1 only.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by cloud-fan <gi...@git.apache.org>.
GitHub user cloud-fan reopened a pull request:
https://github.com/apache/spark/pull/15082
[SPARK-17528][SQL] MutableProjection should not cache content from the input row
## What changes were proposed in this pull request?
For performance reasons, `UnsafeRow.getString`, `getStruct`, etc. return a "pointer" that points to a memory region of this unsafe row. This makes the unsafe projection a little dangerous, because all of its output rows share one instance.
When we implement SQL operators, we should be careful to not cache the input rows because they may be produced by unsafe projection from child operator and thus its content may change overtime.
However, `GenerateMutableProjection` breaks this and may cache the content in input rows. The sort based aggregate use it, but this bug is not exposed because sort based aggregate always do an extra projection for the input row.
This PR fixes the bug of `GenerateMutableProjection` and some related bugs in complex data copy, and remove the useless projection in sort based aggregate.
## How was this patch tested?
some new tests.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark agg-bug
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/15082.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 #15082
----
commit 75b57490d1be2ca4d7ca88bcd4d6d9461d470ce8
Author: Wenchen Fan <we...@databricks.com>
Date: 2016-09-13T17:40:16Z
MutableProjection should not cache content from the input row
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
Posted by hvanhovell <gi...@git.apache.org>.
Github user hvanhovell commented on a diff in the pull request:
https://github.com/apache/spark/pull/15082#discussion_r78931633
--- Diff: sql/catalyst/src/test/scala/org/apache/spark/sql/RowTest.scala ---
@@ -121,10 +121,6 @@ class RowTest extends FunSpec with Matchers {
externalRow should be theSameInstanceAs externalRow.copy()
}
- it("copy should return same ref for internal rows") {
--- End diff --
Well `MutableRow` is mutable, so it shouldn't hold for those. The only exception is `GenericInternalRow`.
That being said, I don't if you remove/modify the test.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #15082: [SPARK-17528][SQL] MutableProjection should not cache co...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/15082
**[Test build #65561 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/65561/consoleFull)** for PR 15082 at commit [`da6a285`](https://github.com/apache/spark/commit/da6a2859c8fe0c90b4aa14a68c2234e6ede36fce).
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org