You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by rxin <gi...@git.apache.org> on 2016/03/22 08:26:05 UTC
[GitHub] spark pull request: [SPARK-14063][SQL] SQLContext.range should ret...
GitHub user rxin opened a pull request:
https://github.com/apache/spark/pull/11880
[SPARK-14063][SQL] SQLContext.range should return Dataset[java.lang.Long]
## What changes were proposed in this pull request?
This patch changed the return type for SQLContext.range from Dataset[Long] (Scala primitive) to Dataset[java.lang.Long] (Java boxed long).
Previously, SPARK-13894 changed the return type of range from Dataset[Row] to Dataset[Long]. The problem is that due to https://issues.scala-lang.org/browse/SI-4388, Scala compiles primitive types in generics into just Object, i.e. range at bytecode level now just returns Dataset[Object]. This is really bad for Java users because they are losing type safety and also need to add a type cast every time they use range.
Talked to Jason Zaugg who suggested the best approach is to return Dataset[java.lang.Long]. The small downside is that when Scala users want to explicitly type a closure used on the dataset returned by range, they would need to use java.lang.Long instead of the Scala Long.
## How was this patch tested?
The signature change should be covered by existing unit tests and API tests. I also added a new test case in DatasetSuite for range.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/rxin/spark SPARK-14063
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/11880.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 #11880
----
commit 8272a1fc8204a1124ef0aad40a3077caedb72d10
Author: Reynold Xin <rx...@databricks.com>
Date: 2016-03-22T07:24:21Z
[SPARK-14063][SQL] SQLContext.range should return Dataset[java.lang.Long]
----
---
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11880#issuecomment-199709421
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/53754/
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on a diff in the pull request:
https://github.com/apache/spark/pull/11880#discussion_r56947412
--- Diff: sql/core/src/test/java/test/org/apache/spark/sql/JavaDataFrameSuite.java ---
@@ -354,7 +354,7 @@ public void testCountMinSketch() {
@Test
public void testBloomFilter() {
- Dataset df = context.range(1000);
--- End diff --
Yes
---
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11880#issuecomment-199709157
**[Test build #53754 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53754/consoleFull)** for PR 11880 at commit [`8272a1f`](https://github.com/apache/spark/commit/8272a1fc8204a1124ef0aad40a3077caedb72d10).
* 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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by cloud-fan <gi...@git.apache.org>.
Github user cloud-fan commented on the pull request:
https://github.com/apache/spark/pull/11880#issuecomment-199681386
LGTM
---
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the pull request:
https://github.com/apache/spark/pull/11880#issuecomment-199677938
**[Test build #53754 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/53754/consoleFull)** for PR 11880 at commit [`8272a1f`](https://github.com/apache/spark/commit/8272a1fc8204a1124ef0aad40a3077caedb72d10).
---
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: [SPARK-14063][SQL] SQLContext.range should ret...
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/11880#discussion_r56946962
--- Diff: sql/core/src/test/java/test/org/apache/spark/sql/JavaDataFrameSuite.java ---
@@ -354,7 +354,7 @@ public void testCountMinSketch() {
@Test
public void testBloomFilter() {
- Dataset df = context.range(1000);
--- End diff --
Why does this code pass compile? Is it because we only turn scalac warnings to errors, but not javac warning?
---
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/11880
---
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the pull request:
https://github.com/apache/spark/pull/11880#issuecomment-199709410
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by srowen <gi...@git.apache.org>.
Github user srowen commented on the pull request:
https://github.com/apache/spark/pull/11880#issuecomment-199835390
Good catch of course. I grepped around for similar issues and noticed this in `JavaDoubleRDD`:
```
def histogram(bucketCount: Int): (Array[scala.Double], Array[Long]) = {
val result = srdd.histogram(bucketCount)
(result._1, result._2)
}
def histogram(buckets: Array[scala.Double]): Array[Long] = {
srdd.histogram(buckets, false)
}
def histogram(buckets: Array[JDouble], evenBuckets: Boolean): Array[Long] = {
srdd.histogram(buckets.map(_.toDouble), evenBuckets)
}
```
For a second I thought it was the same problem, but I suppose an `Array[scala.Long]` is actually `long[]` and so is already fine in Java. Different from the example you're fixing.
---
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: [SPARK-14063][SQL] SQLContext.range should ret...
Posted by rxin <gi...@git.apache.org>.
Github user rxin commented on the pull request:
https://github.com/apache/spark/pull/11880#issuecomment-199954909
@srowen yea I think arrays are fine since those are not generics.
Merging in master.
---
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