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 2017/11/24 16:47:27 UTC
[GitHub] spark pull request #19818: [SPARK-22604][SQL] remove the get address methods...
GitHub user cloud-fan opened a pull request:
https://github.com/apache/spark/pull/19818
[SPARK-22604][SQL] remove the get address methods from ColumnVector
## What changes were proposed in this pull request?
`nullsNativeAddress` and `valuesNativeAddress` are only used in tests and benchmark, no need to be top class API.
## How was this patch tested?
existing tests
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/cloud-fan/spark minor
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/19818.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 #19818
----
commit 5303b23dfae860029e36909d870f94e9b2c185e5
Author: Wenchen Fan <we...@databricks.com>
Date: 2017-11-24T16:43:46Z
remove the get address methods from ColumnVector
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19818: [SPARK-22604][SQL] remove the get address methods...
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/19818#discussion_r153096313
--- Diff: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVector.java ---
@@ -62,13 +62,6 @@
*/
public abstract boolean anyNullsSet();
- /**
- * Returns the off heap ptr for the arrays backing the NULLs and values buffer. Only valid
- * to call for off heap columns.
--- End diff --
good idea
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19818: [SPARK-22604][SQL] remove the get address methods...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on a diff in the pull request:
https://github.com/apache/spark/pull/19818#discussion_r153039303
--- Diff: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java ---
@@ -73,12 +75,12 @@ public OffHeapColumnVector(int capacity, DataType type) {
reset();
}
- @Override
+ @VisibleForTesting
public long valuesNativeAddress() {
return data;
}
- @Override
+ @VisibleForTesting
--- End diff --
Actually, this is not being called by any test.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19818: [SPARK-22604][SQL] remove the get address methods from C...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19818
**[Test build #84174 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84174/testReport)** for PR 19818 at commit [`5303b23`](https://github.com/apache/spark/commit/5303b23dfae860029e36909d870f94e9b2c185e5).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19818: [SPARK-22604][SQL] remove the get address methods from C...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19818
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 #19818: [SPARK-22604][SQL] remove the get address methods from C...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the issue:
https://github.com/apache/spark/pull/19818
cc @kiszk @viirya @gatorsmile
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19818: [SPARK-22604][SQL] remove the get address methods...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/19818#discussion_r153039386
--- Diff: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/ColumnVector.java ---
@@ -62,13 +62,6 @@
*/
public abstract boolean anyNullsSet();
- /**
- * Returns the off heap ptr for the arrays backing the NULLs and values buffer. Only valid
- * to call for off heap columns.
--- End diff --
I think maybe we should move the comment to `OffHeapColumnVector`.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19818: [SPARK-22604][SQL] remove the get address methods from C...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/19818
**[Test build #84174 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/84174/testReport)** for PR 19818 at commit [`5303b23`](https://github.com/apache/spark/commit/5303b23dfae860029e36909d870f94e9b2c185e5).
* 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 #19818: [SPARK-22604][SQL] remove the get address methods...
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/19818#discussion_r153096296
--- Diff: sql/core/src/main/java/org/apache/spark/sql/execution/vectorized/OffHeapColumnVector.java ---
@@ -73,12 +75,12 @@ public OffHeapColumnVector(int capacity, DataType type) {
reset();
}
- @Override
+ @VisibleForTesting
public long valuesNativeAddress() {
return data;
}
- @Override
+ @VisibleForTesting
--- End diff --
they are called in benchmark
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19818: [SPARK-22604][SQL] remove the get address methods...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/19818
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19818: [SPARK-22604][SQL] remove the get address methods from C...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/19818
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/84174/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19818: [SPARK-22604][SQL] remove the get address methods from C...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19818
LGTM
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #19818: [SPARK-22604][SQL] remove the get address methods from C...
Posted by gatorsmile <gi...@git.apache.org>.
Github user gatorsmile commented on the issue:
https://github.com/apache/spark/pull/19818
Thanks! Merged to master.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #19818: [SPARK-22604][SQL] remove the get address methods...
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/19818#discussion_r153004995
--- Diff: sql/core/src/test/scala/org/apache/spark/sql/execution/vectorized/ColumnarBatchSuite.scala ---
@@ -413,16 +399,12 @@ class ColumnarBatchSuite extends SparkFunSuite {
reference.zipWithIndex.foreach { v =>
assert(v._1 == column.getLong(v._2), "idx=" + v._2 +
- " Seed = " + seed + " MemMode=" + memMode)
- if (memMode == MemoryMode.OFF_HEAP) {
- val addr = column.valuesNativeAddress()
- assert(v._1 == Platform.getLong(null, addr + 8 * v._2))
--- End diff --
I don't think these are valid tests.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org