Skip to content

Conversation

@yaooqinn
Copy link
Member

@yaooqinn yaooqinn commented Sep 9, 2020

What changes were proposed in this pull request?

In Spark 3.0.0, the SparkGetColumnsOperation can not recognize NULL columns but now we can because the side effect of https://issues.apache.org/jira/browse/SPARK-32696 / f14f374, but the test coverage for this change was not added.

In Spark, the column size for null fields should be 1, in this PR, we set the right column size for the null type.

Why are the changes needed?

test coverage and fix the client-side information about the null type through jdbc

Does this PR introduce any user-facing change?

NO

How was this patch tested?

added ut both for this pr and SPARK-32696

@yaooqinn
Copy link
Member Author

yaooqinn commented Sep 9, 2020

cc @cloud-fan @maropu @dongjoon-hyun, and thanks very much~

@yaooqinn yaooqinn changed the title [SPARK-32826][SQL] Add test case for get null columns using SparkGetColumnsOperation [SPARK-32826][SQL][TESTS] Add test case for get null columns using SparkGetColumnsOperation Sep 9, 2020
@SparkQA
Copy link

SparkQA commented Sep 9, 2020

Test build #128433 has finished for PR 29687 at commit 2e6914d.

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

@maropu maropu changed the title [SPARK-32826][SQL][TESTS] Add test case for get null columns using SparkGetColumnsOperation [SPARK-32826][SQL][TEST] Add test case for get null columns using SparkGetColumnsOperation Sep 9, 2020
private def getColumnSize(typ: DataType): Option[Int] = typ match {
case dt @ (BooleanType | _: NumericType | DateType | TimestampType | CalendarIntervalType) =>
case dt @ (BooleanType | _: NumericType | DateType | TimestampType |
CalendarIntervalType | NullType) =>
Copy link
Contributor

Choose a reason for hiding this comment

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

so it's not a test only PR. Can we remove [TEST] from PR title?

Copy link
Member Author

Choose a reason for hiding this comment

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

OK~

@yaooqinn yaooqinn changed the title [SPARK-32826][SQL][TEST] Add test case for get null columns using SparkGetColumnsOperation [SPARK-32826][SQL] Add test case for get null columns using SparkGetColumnsOperation Sep 10, 2020
@cloud-fan
Copy link
Contributor

please also update the PR description to mention the non-test change.

@yaooqinn yaooqinn changed the title [SPARK-32826][SQL] Add test case for get null columns using SparkGetColumnsOperation [SPARK-32826][SQL] Set the right column size for the null type in SparkGetColumnsOperation Sep 10, 2020
@yaooqinn
Copy link
Member Author

updated, thanks for suggestions

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in 9ab8a2c Sep 10, 2020
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