[SPARK-34432][SQL][TESTS] Add JavaSimpleWritableDataSource #31560

kevincmchen · 2021-02-14T02:56:28Z

What changes were proposed in this pull request?

This is a followup of #19269

In #19269 , there is only a scala implementation of simple writable data source in DataSourceV2Suite.

This PR adds a java implementation of it.

Why are the changes needed?

To improve test coverage.

Does this PR introduce any user-facing change?

No

How was this patch tested?

existing testsuites

…e writable data source in `DataSourceV2Suite` ### What changes were proposed in this pull request? This is a followup of #19269 In #19269 , there is only a scala implementation of simple writable data source in `DataSourceV2Suite`. This PR adds a java implementation of it. ### Why are the changes needed? To improve test coverage. ### Does this PR introduce _any_ user-facing change? No ### How was this patch tested? existing testsuites

HyukjinKwon · 2021-02-16T02:58:30Z

cc @rdblue @cloud-fan

sql/core/src/test/java/test/org/apache/spark/sql/connector/JavaSimpleWritableDataSource.java

dongjoon-hyun

Thank you for making your first contribution. I left a few general comments.

sql/core/src/test/java/test/org/apache/spark/sql/connector/JavaSimpleWritableDataSource.java

cloud-fan · 2021-02-17T14:17:08Z

ok to test

SparkQA · 2021-02-17T15:39:39Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39780/

SparkQA · 2021-02-17T16:08:45Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39780/

SparkQA · 2021-02-17T19:52:59Z

Test build #135199 has finished for PR 31560 at commit d5dc307.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
public class JavaSimpleWritableDataSource implements TestingV2Source, SessionConfigSupport
static class JavaCSVInputPartitionReader implements InputPartition
static class JavaCSVReaderFactory implements PartitionReaderFactory
static class JavaSimpleCounter
static class JavaCSVDataWriterFactory implements DataWriterFactory
static class JavaCSVDataWriter implements DataWriter<InternalRow>
class MyScanBuilder extends JavaSimpleScanBuilder
static class MyWriteBuilder implements WriteBuilder, SupportsTruncate
static class MyWrite implements Write
static class MyBatchWrite implements BatchWrite
class MyTable extends JavaSimpleBatchTable implements SupportsWrite

1. add a class description for JavaSimpleWritableDataSource OPTIMIZE: 1. re-order the import 2. match the class layout with the existing SimpleWritableDataSource UPDATE: 1. catch the specific exception(IOException) instead of Exception. 2. use SimpleCounter

delete duplicated blank Lines in `JavaSimpleWritableDataSource`

SparkQA · 2021-02-18T14:58:58Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39811/

SparkQA · 2021-02-18T15:17:20Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39811/

SparkQA · 2021-02-18T16:02:39Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39812/

SparkQA · 2021-02-18T17:19:17Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39812/

SparkQA · 2021-02-18T18:54:34Z

Test build #135230 has finished for PR 31560 at commit 88c6edf.

This patch passes all tests.
This patch merges cleanly.
This patch adds the following public classes (experimental):
static class JavaCSVInputPartitionReader implements InputPartition
static class JavaCSVReaderFactory implements PartitionReaderFactory
static class JavaCSVDataWriterFactory implements DataWriterFactory
static class JavaCSVDataWriter implements DataWriter<InternalRow>

SparkQA · 2021-02-18T19:17:40Z

Test build #135231 has finished for PR 31560 at commit 8c080ba.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-02-19T14:02:55Z

sql/core/src/test/java/test/org/apache/spark/sql/connector/JavaSimpleWritableDataSource.java

+          return new InputPartition[0];
+        }
+      } catch (IOException e) {
+        throw new RuntimeException(e);


This seems not java friendly, I think we should update Batch#planInputPartitions to add throws IOException, the same as PartitionReader#next.

The throws clause is a compile-time check and won't break binary compatibility. Also cc @rdblue

I feel the same.

I have some different opinions recently.

for Batch#planInputPartitions, If this method fails (by throwing an exception), the underlying data source scan may require manual cleanup .

for BatchWrite#commit, If this method fails (by throwing an exception), this writing job is considered to have been failed, and BatchWrite#abort would be called，but BatchWrite#abort may not be able to deal with it. more details, pls see the comments of BatchWrite#abort

so its better to deal with these exceptions in Batch#planInputPartitions and BatchWrite#commit;

@cloud-fan

sql/core/src/test/java/test/org/apache/spark/sql/connector/JavaSimpleWritableDataSource.java

…gV2Source`

SparkQA · 2021-02-20T11:15:11Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39890/

…rom JavaSimpleBatchTable and override the function capabilities in MyTable

SparkQA · 2021-02-20T12:50:45Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39890/

SparkQA · 2021-02-20T12:57:54Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39893/

SparkQA · 2021-02-20T13:27:16Z

Kubernetes integration test status success
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39893/

SparkQA · 2021-02-20T14:05:23Z

Test build #135307 has finished for PR 31560 at commit 9037d5f.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-20T14:31:06Z

Test build #135310 has finished for PR 31560 at commit 9178240.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-20T17:01:20Z

Test build #135313 has finished for PR 31560 at commit 3fc7288.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-21T16:18:43Z

Kubernetes integration test starting
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39902/

SparkQA · 2021-02-21T16:37:05Z

Kubernetes integration test status failure
URL: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder-K8s/39902/

SparkQA · 2021-02-21T18:56:08Z

Test build #135320 has finished for PR 31560 at commit c665f68.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

SparkQA · 2021-02-21T19:17:53Z

Test build #135322 has finished for PR 31560 at commit 0d85f08.

This patch passes all tests.
This patch merges cleanly.
This patch adds no public classes.

cloud-fan · 2021-02-22T09:32:13Z

sql/core/src/test/java/test/org/apache/spark/sql/connector/JavaSimpleWritableDataSource.java

+
+  @Override
+  public String keyPrefix() {
+    return "javaSimpleWritableDataSource";


Seems like we don't need to implement SessionConfigSupport. Can you open a follow-up PR to remove it from both the Scala and Java versions?

cloud-fan · 2021-02-22T09:34:28Z

sql/core/src/test/java/test/org/apache/spark/sql/connector/JavaSimpleWritableDataSource.java

+            Object[] objects =
+                Arrays.stream(currentLine.split(","))
+                    .map(String::trim)
+                    .map(Integer::parseInt)


can we change the Scala version as well in a follow-up PR?

ok，i'll change the scala version

cloud-fan · 2021-02-22T09:38:09Z

thanks, merging to master!

cloud-fan · 2021-02-22T09:39:36Z

@kevincmchen can you open a new PR to add throws clause to planInputPartitions, commit, etc., and trigger the discussion there?

kevincmchen · 2021-02-22T11:50:52Z

@kevincmchen can you open a new PR to add throws clause to planInputPartitions, commit, etc., and trigger the discussion there?

ok, all right.

### What changes were proposed in this pull request? This is a followup of #31560, In #31560, we added `JavaSimpleWritableDataSource ` and left some little problems like unused interface `SessionConfigSupport` 、 inconsistent schema between `JavaSimpleWritableDataSource ` and `SimpleWritableDataSource`. This PR fixes the remaining problems in #31560. ### Why are the changes needed? 1. `SessionConfigSupport` in `JavaSimpleWritableDataSource ` and `SimpleWritableDataSource` is never used, so we don't need to implement it. 2. change the schema of `SimpleWritableDataSource`, to match `TestingV2Source` ### Does this PR introduce _any_ user-facing change? NO ### How was this patch tested? existing testsuites Closes #31621 from kevincmchen/SPARK-34498. Authored-by: kevincmchen <[email protected]> Signed-off-by: Wenchen Fan <[email protected]>

github-actions bot added the SQL label Feb 14, 2021

HyukjinKwon changed the title ~~[SPARK-34432][SQL][TEST][FOLLOWUP] Add a java implementation of simpl…~~ [SPARK-34432][SQL][TESTS] Add a java implementation of simple writable data source in DataSourceV2Suite Feb 16, 2021