[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[GitHub] spark pull request #15082: [SPARK-17528][SQL] MutableProjection should not c...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
-
To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org
For additional commands, e-mail: reviews-h...@spark.apache.org
[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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: reviews-unsubscr...@spark.apache.org For additional commands, e-mail: reviews-h...@spark.apache.org
