Skip to content

[SPARK-25031][SQL] Fix MapType schema print#22006

Closed
invkrh wants to merge 4 commits intoapache:masterfrom
invkrh:fix-map-schema-print
Closed

[SPARK-25031][SQL] Fix MapType schema print#22006
invkrh wants to merge 4 commits intoapache:masterfrom
invkrh:fix-map-schema-print

Conversation

@invkrh
Copy link
Copy Markdown
Contributor

@invkrh invkrh commented Aug 6, 2018

What changes were proposed in this pull request?

The PR fix the bug in buildFormattedString function in MapType, which makes the printed schema misleading.

How was this patch tested?

Added UT

mgaido91
mgaido91 reviewed Aug 6, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mgaido91 mgaido91 left a comment

Choose a reason for hiding this comment

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

the change itself LGTM, just left one comment.

cc @gatorsmile @HyukjinKwon for triggering the build

sql/catalyst/src/test/scala/org/apache/spark/sql/types/MapTypeSuite.scala Outdated
@@ -0,0 +1,47 @@
/*
Copy link
Copy Markdown
Contributor

@mgaido91 mgaido91 Aug 6, 2018

Choose a reason for hiding this comment

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

I think we can put this in DataTypeSuite

@HyukjinKwon
Copy link
Copy Markdown
Member

HyukjinKwon commented Aug 6, 2018

ok to test

HyukjinKwon
HyukjinKwon reviewed Aug 6, 2018
View reviewed changes
sql/catalyst/src/test/scala/org/apache/spark/sql/types/MapTypeSuite.scala Outdated

val keyType: DataType = StructType(
Seq(StructField("a", DataTypes.IntegerType),
StructField("b", DataTypes.IntegerType)))
Copy link
Copy Markdown
Member

@HyukjinKwon HyukjinKwon Aug 6, 2018

Choose a reason for hiding this comment

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

nit: 2 space indentation

Copy link
Copy Markdown
Contributor Author

@invkrh invkrh Aug 7, 2018

Choose a reason for hiding this comment

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

Done

HyukjinKwon
HyukjinKwon reviewed Aug 6, 2018
View reviewed changes
sql/catalyst/src/test/scala/org/apache/spark/sql/types/MapTypeSuite.scala Outdated
import org.apache.spark.SparkFunSuite

class MapTypeSuite extends SparkFunSuite {
test("SPARK-25031") {
Copy link
Copy Markdown
Member

@HyukjinKwon HyukjinKwon Aug 6, 2018

Choose a reason for hiding this comment

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

nit: SPARK-25031: MapType should produce current formatted string for complex types

Copy link
Copy Markdown
Contributor Author

@invkrh invkrh Aug 7, 2018

Choose a reason for hiding this comment

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

Done

@HyukjinKwon
Copy link
Copy Markdown
Member

HyukjinKwon commented Aug 6, 2018

LGTM too otherwise

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 6, 2018

Test build #94282 has finished for PR 22006 at commit f1a7e29.

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

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 7, 2018

Test build #94355 has finished for PR 22006 at commit 9bc0541.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Copy Markdown
Contributor

mgaido91 commented Aug 7, 2018

the test failure is flaky. Just created a PR for fixing it: #22023. I'd still prefer to merge the added UT to DataTypeSuite as all UTs regarding the types are there as of now. But if you @HyukjinKwon think we can use the new test suite, then we can retrigger the tests. Thanks.

@HyukjinKwon
Copy link
Copy Markdown
Member

HyukjinKwon commented Aug 7, 2018

Let's just put this in DataTypeSuite since we need another test that should be ran anyway.

@HyukjinKwon
Copy link
Copy Markdown
Member

HyukjinKwon commented Aug 7, 2018

retest this please

mgaido91
mgaido91 approved these changes Aug 7, 2018
View reviewed changes
Copy link
Copy Markdown
Contributor

@mgaido91 mgaido91 left a comment

Choose a reason for hiding this comment

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

LGTM

sql/catalyst/src/test/scala/org/apache/spark/sql/types/DataTypeSuite.scala Outdated
false)

test("SPARK-25031: MapType should produce current formatted string for complex types") {

Copy link
Copy Markdown
Contributor

@mgaido91 mgaido91 Aug 7, 2018

Choose a reason for hiding this comment

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

nit: unneeded blank line

Copy link
Copy Markdown
Contributor Author

@invkrh invkrh Aug 7, 2018

Choose a reason for hiding this comment

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

Done

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 7, 2018

Test build #94372 has finished for PR 22006 at commit 4328199.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@mgaido91
Copy link
Copy Markdown
Contributor

mgaido91 commented Aug 7, 2018

retest this please

@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 7, 2018

Test build #94368 has finished for PR 22006 at commit 06f656d.

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

@gatorsmile
Copy link
Copy Markdown
Member

gatorsmile commented Aug 7, 2018

Thanks! Merged to master.

@asfgit asfgit closed this in 8c13cb2 Aug 7, 2018
@SparkQA
Copy link
Copy Markdown

SparkQA commented Aug 7, 2018

Test build #94383 has finished for PR 22006 at commit 4328199.

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

otterc pushed a commit to linkedin/spark that referenced this pull request Mar 22, 2023
@invkrh @Victsm
[SPARK-25031][SQL] Fix MapType schema print
315b4a7 
The PR fix the bug in `buildFormattedString` function in `MapType`, which makes the printed schema misleading.

Added UT

Closes apache#22006 from invkrh/fix-map-schema-print.

Authored-by: invkrh <invkrh@gmail.com>
Signed-off-by: Xiao Li <gatorsmile@gmail.com>
(cherry picked from commit 8c13cb2)

Ref: LIHADOOP-40100

RB=1410523
BUG=LIHADOOP-40100
G=superfriends-reviewers
R=fli,mshen,yezhou,edlu
A=edlu
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@HyukjinKwon HyukjinKwon HyukjinKwon left review comments

+1 more reviewer

@mgaido91 mgaido91 mgaido91 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@invkrh @HyukjinKwon @SparkQA @mgaido91 @gatorsmile