You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@spark.apache.org by aai95 <gi...@git.apache.org> on 2018/08/28 11:34:36 UTC
[GitHub] spark pull request #22253: [SPARK-24411][SQL] Adding native Java tests for '...
GitHub user aai95 opened a pull request:
https://github.com/apache/spark/pull/22253
[SPARK-24411][SQL] Adding native Java tests for 'isInCollection'
## What changes were proposed in this pull request?
Conversion of Scala Seq to Java collections were replaced by native Java collections.
## How was this patch tested?
Test was updated.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/aai95/spark isInCollectionJavaTest
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/spark/pull/22253.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 #22253
----
commit 0d0a4ec8e723df808e10fd487038d3e7a4b9ca26
Author: aai95 <aa...@...>
Date: 2018-08-28T11:09:08Z
[SPARK-24411][SQL] Adding native Java tests for 'isInCollection'
----
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22253
**[Test build #95463 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95463/testReport)** for PR 22253 at commit [`51521ed`](https://github.com/apache/spark/commit/51521ed1389ce1739621ca2df0ad5855ff305c0c).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95467/
Test PASSed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22253: [SPARK-24411] [SQL] Adding native Java tests for ...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/spark/pull/22253
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411][SQL] Adding native Java tests for 'isInCol...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by aai95 <gi...@git.apache.org>.
Github user aai95 commented on the issue:
https://github.com/apache/spark/pull/22253
@dbtsai review my new changes please.
Should I delete `test("isInCollection: Java Collection")` from `ColumnExpressionSuite.scala` or make changes in `JavaColumnExpressionSuite.java`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on the issue:
https://github.com/apache/spark/pull/22253
LGTM. Merged into master. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22253: [SPARK-24411] [SQL] Adding native Java tests for ...
Posted by aai95 <gi...@git.apache.org>.
Github user aai95 commented on a diff in the pull request:
https://github.com/apache/spark/pull/22253#discussion_r213944953
--- Diff: sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java ---
@@ -0,0 +1,80 @@
+package test.org.apache.spark.sql;
+
+import org.apache.spark.api.java.function.FilterFunction;
+import org.apache.spark.sql.Column;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.test.TestSparkSession;
+import org.apache.spark.sql.types.StructType;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.*;
+
+import static org.apache.spark.sql.types.DataTypes.*;
--- End diff --
Hello @HyukjinKwon, I tried to remove the wildcard import and got the following result:
`import java.util.ArrayList;`
`import java.util.Arrays;`
`import java.util.HashSet;`
`import java.util.List;`
`import java.util.Locale;`
`import static org.apache.spark.sql.types.DataTypes.IntegerType;`
`import static org.apache.spark.sql.types.DataTypes.StringType;`
`import static org.apache.spark.sql.types.DataTypes.createArrayType;`
`import static org.apache.spark.sql.types.DataTypes.createStructField;`
`import static org.apache.spark.sql.types.DataTypes.createStructType;`
Is it OK to use wildcard here if exactly 5 imports were used in every case? Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95463/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
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 #22253: [SPARK-24411][SQL] Adding native Java tests for 'isInCol...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on the issue:
https://github.com/apache/spark/pull/22253
@aai95 The goal of this JIRA is moving the tests to .java files. Thanks.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22253: [SPARK-24411] [SQL] Adding native Java tests for ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/22253#discussion_r213881608
--- Diff: sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java ---
@@ -0,0 +1,80 @@
+package test.org.apache.spark.sql;
+
+import org.apache.spark.api.java.function.FilterFunction;
+import org.apache.spark.sql.Column;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.test.TestSparkSession;
+import org.apache.spark.sql.types.StructType;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.*;
+
+import static org.apache.spark.sql.types.DataTypes.*;
+
+public class JavaColumnExpressionSuite {
+
+ private transient TestSparkSession spark;
--- End diff --
Java file also should be two space indentation
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22253
**[Test build #95430 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95430/testReport)** for PR 22253 at commit [`d7eea40`](https://github.com/apache/spark/commit/d7eea4001f8abe8ff2e496a0c41b72afe2d29666).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411][SQL] Adding native Java tests for 'isInCol...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on the issue:
https://github.com/apache/spark/pull/22253
Historically, we test many Java APIs in Scala suite, and hope it will work in Java. But that fails us sometime. The goal is to add `JavaColumnExpressionSuite.java`, and move `test("isInCollection: Java Collection")` to Java suite.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Merged build finished. Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411][SQL] Adding native Java tests for 'isInCol...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by aai95 <gi...@git.apache.org>.
Github user aai95 commented on the issue:
https://github.com/apache/spark/pull/22253
I took into account all your remarks. Thanks.
@dbtsai @viirya @HyukjinKwon review my changes please.
1. Scala test was removed.
2. ASF License Header was added.
3. The name of the 2nd method was shortened.
4. Changes were made in the Java code style.
5. A bug was fixed if the method did not throw an exception.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22253: [SPARK-24411] [SQL] Adding native Java tests for ...
Posted by HyukjinKwon <gi...@git.apache.org>.
Github user HyukjinKwon commented on a diff in the pull request:
https://github.com/apache/spark/pull/22253#discussion_r213881717
--- Diff: sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java ---
@@ -0,0 +1,80 @@
+package test.org.apache.spark.sql;
+
+import org.apache.spark.api.java.function.FilterFunction;
+import org.apache.spark.sql.Column;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.test.TestSparkSession;
+import org.apache.spark.sql.types.StructType;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.*;
+
+import static org.apache.spark.sql.types.DataTypes.*;
--- End diff --
Shall we avoid wlidcard import if they are less then 5 imports
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22253: [SPARK-24411] [SQL] Adding native Java tests for ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22253#discussion_r213864459
--- Diff: sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java ---
@@ -0,0 +1,80 @@
+package test.org.apache.spark.sql;
+
+import org.apache.spark.api.java.function.FilterFunction;
+import org.apache.spark.sql.Column;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.RowFactory;
+import org.apache.spark.sql.test.TestSparkSession;
+import org.apache.spark.sql.types.StructType;
+import org.junit.After;
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+
+import java.util.*;
+
+import static org.apache.spark.sql.types.DataTypes.*;
+
+public class JavaColumnExpressionSuite {
+
+ private transient TestSparkSession spark;
+
+ @Before
+ public void setUp() {
+ spark = new TestSparkSession();
+ }
+
+ @After
+ public void tearDown() {
+ spark.stop();
+ spark = null;
+ }
+
+ @Test
+ public void isInCollectionWorksCorrectlyOnJava() {
+ List<Row> rows = Arrays.asList(
+ RowFactory.create(1, "x"),
+ RowFactory.create(2, "y"),
+ RowFactory.create(3, "z")
+ );
+ StructType schema = createStructType(Arrays.asList(
+ createStructField("a", IntegerType, false),
+ createStructField("b", StringType, false)
+ ));
+ Dataset<Row> df = spark.createDataFrame(rows, schema);
+ // Test with different types of collections
+ Assert.assertTrue(Arrays.equals(
+ (Row[]) df.filter(df.col("a").isInCollection(Arrays.asList(1, 2))).collect(),
+ (Row[]) df.filter((FilterFunction<Row>) r -> r.getInt(0) == 1 || r.getInt(0) == 2).collect()
+ ));
+ Assert.assertTrue(Arrays.equals(
+ (Row[]) df.filter(df.col("a").isInCollection(new HashSet<>(Arrays.asList(1, 2)))).collect(),
+ (Row[]) df.filter((FilterFunction<Row>) r -> r.getInt(0) == 1 || r.getInt(0) == 2).collect()
+ ));
+ Assert.assertTrue(Arrays.equals(
+ (Row[]) df.filter(df.col("a").isInCollection(new ArrayList<>(Arrays.asList(3, 1)))).collect(),
+ (Row[]) df.filter((FilterFunction<Row>) r -> r.getInt(0) == 3 || r.getInt(0) == 1).collect()
+ ));
+ }
+
+ @Test
+ public void isInCollectionThrowsExceptionWithCorrectMessageOnJava() {
--- End diff --
Can we shorten this method name? E.g., `checkExceptionMessage`?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark pull request #22253: [SPARK-24411] [SQL] Adding native Java tests for ...
Posted by viirya <gi...@git.apache.org>.
Github user viirya commented on a diff in the pull request:
https://github.com/apache/spark/pull/22253#discussion_r213863748
--- Diff: sql/core/src/test/java/test/org/apache/spark/sql/JavaColumnExpressionSuite.java ---
@@ -0,0 +1,80 @@
+package test.org.apache.spark.sql;
--- End diff --
You need to add the Apache License header at the beginning of a file.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/SparkPullRequestBuilder/95430/
Test FAILed.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22253
**[Test build #95463 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95463/testReport)** for PR 22253 at commit [`51521ed`](https://github.com/apache/spark/commit/51521ed1389ce1739621ca2df0ad5855ff305c0c).
* This patch **fails Java style 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 issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22253
**[Test build #95430 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95430/testReport)** for PR 22253 at commit [`d7eea40`](https://github.com/apache/spark/commit/d7eea4001f8abe8ff2e496a0c41b72afe2d29666).
* This patch **fails RAT tests**.
* This patch merges cleanly.
* This patch adds the following public classes _(experimental)_:
* `public class JavaColumnExpressionSuite `
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on the issue:
https://github.com/apache/spark/pull/22253
Remove `test("isInCollection: Java Collection")` from the scala code, and please follow the code style in other java code. Remember to add the ASF Source Header.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22253
**[Test build #95467 has finished](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95467/testReport)** for PR 22253 at commit [`576600e`](https://github.com/apache/spark/commit/576600e2dcf9a58161a566e181f7fbc3ac94ae14).
* 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 issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by dbtsai <gi...@git.apache.org>.
Github user dbtsai commented on the issue:
https://github.com/apache/spark/pull/22253
ok to test
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411][SQL] Adding native Java tests for 'isInCol...
Posted by aai95 <gi...@git.apache.org>.
Github user aai95 commented on the issue:
https://github.com/apache/spark/pull/22253
Sorry for misunderstanding, @dbtsai do you mean that the goal of this issue is in creation of a new Java test class with one test which will replace current Scala test "isInCollection: Java Collection"?
Or the goal is in rewriting all tests from Scala class "ColumnExpressionSuite" to Java code?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411] [SQL] Adding native Java tests for 'isInCo...
Posted by SparkQA <gi...@git.apache.org>.
Github user SparkQA commented on the issue:
https://github.com/apache/spark/pull/22253
**[Test build #95467 has started](https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/95467/testReport)** for PR 22253 at commit [`576600e`](https://github.com/apache/spark/commit/576600e2dcf9a58161a566e181f7fbc3ac94ae14).
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org
[GitHub] spark issue #22253: [SPARK-24411][SQL] Adding native Java tests for 'isInCol...
Posted by AmplabJenkins <gi...@git.apache.org>.
Github user AmplabJenkins commented on the issue:
https://github.com/apache/spark/pull/22253
Can one of the admins verify this patch?
---
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org