Skip to content

[SPARK-32576][SQL][TEST][FOLLOWUP] Add tests for all the character array types in PostgresIntegrationSuite #29397

Closed
maropu wants to merge 2 commits into
apache:masterfrom
maropu:SPARK-32576-FOLLOWUP
Closed

[SPARK-32576][SQL][TEST][FOLLOWUP] Add tests for all the character array types in PostgresIntegrationSuite #29397
maropu wants to merge 2 commits into
apache:masterfrom
maropu:SPARK-32576-FOLLOWUP

Conversation

@maropu

@maropu maropu commented Aug 10, 2020

Copy link
Copy Markdown
Member

What changes were proposed in this pull request?

This is a follow-up PR of #29192 that adds integration tests for character arrays in PostgresIntegrationSuite.

Why are the changes needed?

For better test coverage.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Add tests.

@maropu

maropu commented Aug 10, 2020

Copy link
Copy Markdown
Member Author

@dongjoon-hyun Sorry to bother you, but could you check it again?

@kujon

kujon commented Aug 10, 2020

Copy link
Copy Markdown
Contributor

It's probably worth adding a SELECT query as well. That's how I discovered the issue.

@maropu

maropu commented Aug 10, 2020

Copy link
Copy Markdown
Member Author

Thanks for the comment, @kujon. A SELECT query has the almost same code path, so I think the curreent tests look fine.

@kujon kujon mentioned this pull request Aug 10, 2020
Closed
@maropu maropu closed this in 7990ea1 Aug 10, 2020
maropu added a commit that referenced this pull request Aug 10, 2020
@maropu
[SPARK-32576][SQL][TEST][FOLLOWUP] Add tests for all the character ar…
843ff03 
…ray types in PostgresIntegrationSuite

### What changes were proposed in this pull request?

This is a follow-up PR of #29192 that adds integration tests for character arrays in `PostgresIntegrationSuite`.

### Why are the changes needed?

For better test coverage.

### Does this PR introduce _any_ user-facing change?

No.

### How was this patch tested?

Add tests.

Closes #29397 from maropu/SPARK-32576-FOLLOWUP.

Authored-by: Takeshi Yamamuro <yamamuro@apache.org>
Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>
(cherry picked from commit 7990ea1)
Signed-off-by: Takeshi Yamamuro <yamamuro@apache.org>
@maropu

maropu commented Aug 10, 2020

Copy link
Copy Markdown
Member Author

Thanks, all! Merged to master/branch-3.0.

yaooqinn
yaooqinn reviewed Aug 10, 2020
View reviewed changes
Comment thread ...er-integration-tests/src/test/scala/org/apache/spark/sql/jdbc/PostgresIntegrationSuite.scala
test("SPARK-32576: character array type tests") {
val df = sqlContext.read.jdbc(jdbcUrl, "char_array_types", new Properties)
val row = df.collect()
assert(row.length == 1)

@yaooqinn yaooqinn Aug 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

super nit: ===

@maropu maropu Aug 10, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh, I missed that. But, its trivial, so I'll fix it next time I update this file. Anyway, thanks for the check, @yaooqinn

@HyukjinKwon HyukjinKwon Aug 10, 2020

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

That's alright. We don't currently have a style rule for that. == is already being used here and there too though I prefer === as well because it can compare, for example, arrays properly with a good error message.

@SparkQA

SparkQA commented Aug 10, 2020

Copy link
Copy Markdown

Test build #127275 has finished for PR 29397 at commit 37e7c32.

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

@SparkQA

SparkQA commented Aug 10, 2020

Copy link
Copy Markdown

Test build #127277 has finished for PR 29397 at commit 9907bf5.

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

@dongjoon-hyun

dongjoon-hyun commented Aug 10, 2020

Copy link
Copy Markdown
Member

Thank you for adding the test coverage, @maropu ! :)

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

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon approved these changes
@yaooqinn yaooqinn yaooqinn left review comments

Assignees

No one assigned

Labels

SQL

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@maropu @kujon @SparkQA @dongjoon-hyun @HyukjinKwon @yaooqinn