Skip to content

Conversation

@kevincmchen
Copy link
Contributor

@kevincmchen kevincmchen commented Feb 23, 2021

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

1. we don't need to implement SessionConfigSupport in simple writable table data source tests. remove it
2. change the schema of `SimpleWritableDataSource`, to match `TestingV2Source`
@github-actions github-actions bot added the SQL label Feb 23, 2021
@srowen
Copy link
Member

srowen commented Feb 24, 2021

Jenkins test this please

@SparkQA
Copy link

SparkQA commented Feb 24, 2021

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

@SparkQA
Copy link

SparkQA commented Feb 24, 2021

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

@SparkQA
Copy link

SparkQA commented Feb 24, 2021

Test build #135430 has finished for PR 31621 at commit e252d75.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • public class JavaSimpleWritableDataSource implements TestingV2Source
  • class SimpleWritableDataSource extends SimpleTableProvider

* Each job moves files from `target/_temporary/uniqueId/` to `target`.
*/
class SimpleWritableDataSource extends SimpleTableProvider with SessionConfigSupport {
class SimpleWritableDataSource extends SimpleTableProvider {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we extend TestingV2Source instead?

Copy link
Contributor Author

@kevincmchen kevincmchen Mar 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, i‘ll fix it

Copy link
Member

@srowen srowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks OK


override def write(record: InternalRow): Unit = {
out.writeBytes(s"${record.getLong(0)},${record.getLong(1)}\n")
out.writeBytes(s"${record.getInt(0)},${record.getInt(1)}\n")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change just matches the TestingV2Source.schema?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea

}

override def readSchema(): StructType = tableSchema
override def readSchema(): StructType = TestingV2Source.schema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SimpleScanBuilder.readSchema is already TestingV2Source.schema, we don't need to override it here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it‘s unnecessary to override the function.

private val conf = SparkContext.getActive.get.hadoopConfiguration

override def schema(): StructType = tableSchema
override def schema(): StructType = TestingV2Source.schema
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto, SimpleBatchTable.schema is already TestingV2Source.schema

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it‘s unnecessary.

@srowen
Copy link
Member

srowen commented Mar 2, 2021

Jenkins test this please

@cloud-fan
Copy link
Contributor

GA passed, merging to master, thanks!

@cloud-fan cloud-fan closed this in 9e8547c Mar 2, 2021
@SparkQA
Copy link

SparkQA commented Mar 2, 2021

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

@SparkQA
Copy link

SparkQA commented Mar 2, 2021

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants