Skip to content

[SPARK-25817][SQL] Dataset encoder should support combination of map and product type - #22812

Closed
cloud-fan wants to merge 3 commits into
apache:masterfrom
cloud-fan:map
Closed

[SPARK-25817][SQL] Dataset encoder should support combination of map and product type#22812
cloud-fan wants to merge 3 commits into
apache:masterfrom
cloud-fan:map

Conversation

@cloud-fan

@cloud-fancloud-fan commented Oct 24, 2018

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

After #22745 , Dataset encoder supports the combination of java bean and map type. This PR is to fix the Scala side.

The reason why it didn't work before is, CatalystToExternalMap tries to get the data type of the input map expression, while it can be unresolved and its data type is known. To fix it, we can follow UnresolvedMapObjects, to create a UnresolvedCatalystToExternalMap, and only create CatalystToExternalMap when the input map expression is resolved and the data type is known.

How was this patch tested?

enable a old test case

cloud-fan
cloud-fan commented Oct 24, 2018
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala Outdated

@cloud-fancloud-fanOct 24, 2018•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

these are unrelated, but is a followup of #16986 to address the remaining code style comments.

cloud-fan
cloud-fan commented Oct 24, 2018
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/objects/objects.scala Outdated

@cloud-fancloud-fanOct 24, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

this is to address #22745 (comment)

@cloud-fan

cloud-fan commented Oct 24, 2018

Copy link
Copy Markdown
ContributorAuthor

cc @michalsenkyr@vofque@viirya

@SparkQA

SparkQA commented Oct 24, 2018

Copy link
Copy Markdown

Test build #97959 has finished for PR 22812 at commit da31d26.

  • This patch fails due to an unknown error code, -9.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class UnresolvedCatalystToExternalMap(

@cloud-fan

cloud-fan commented Oct 24, 2018

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

SparkQA commented Oct 24, 2018

Copy link
Copy Markdown

Test build #97965 has finished for PR 22812 at commit da31d26.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
  • case class UnresolvedCatalystToExternalMap(

cloud-fan
cloud-fan commented Oct 25, 2018
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala Outdated

@cloud-fancloud-fanOct 25, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

unrelated but to fix minor code style issues in #22749

@SparkQA

SparkQA commented Oct 25, 2018

Copy link
Copy Markdown

Test build #98019 has finished for PR 22812 at commit bc39824.

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

@cloud-fan

cloud-fan commented Oct 25, 2018

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

SparkQA commented Oct 25, 2018

Copy link
Copy Markdown

Test build #98020 has finished for PR 22812 at commit cf5a01e.

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

@SparkQA

SparkQA commented Oct 25, 2018

Copy link
Copy Markdown

Test build #98026 has finished for PR 22812 at commit cf5a01e.

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

viirya
viirya reviewed Oct 26, 2018
View reviewed changes
Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/encoders/ExpressionEncoder.scala Outdated

@viiryaviiryaOct 26, 2018

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.

Thanks!

Comment threadsql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/analysis/Analyzer.scala Outdated

@viiryaviiryaOct 26, 2018

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.

When u.child is resolved, is there still UnresolvedExtractValue?

@cloud-fancloud-fanOct 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Yea I think so. The UnresolvedExtractValue might appear in CatalystToExternalMap.keyLambdaFunction and valueLambdaFunction

@viiryaviiryaOct 26, 2018

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.

ResolveReferences might also process that, but it is also good to have them here.

@cloud-fancloud-fanOct 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

TBH I don't quite remember why I did this for MapObjects, so I just follow it here. Maybe we can remove it in a followup PR.

@viirya

viirya commented Oct 26, 2018

Copy link
Copy Markdown
Member

LGTM

@SparkQA

SparkQA commented Oct 26, 2018

Copy link
Copy Markdown

Test build #98062 has finished for PR 22812 at commit 517bebf.

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

@cloud-fan

cloud-fan commented Oct 28, 2018

Copy link
Copy Markdown
ContributorAuthor

retest this please

@SparkQA

SparkQA commented Oct 28, 2018

Copy link
Copy Markdown

Test build #98147 has finished for PR 22812 at commit 517bebf.

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

@cloud-fan

cloud-fan commented Oct 28, 2018

Copy link
Copy Markdown
ContributorAuthor

thanks, merging to master!

jackylee-ch pushed a commit to jackylee-ch/spark that referenced this pull request Feb 18, 2019
@cloud-fanJackey Lee
[SPARK-25817][SQL] Dataset encoder should support combination of map …
637d4f4
…and product type
## What changes were proposed in this pull request?
After apache#22745 , Dataset encoder supports the combination of java bean and map type. This PR is to fix the Scala side.
The reason why it didn't work before is, `CatalystToExternalMap` tries to get the data type of the input map expression, while it can be unresolved and its data type is known. To fix it, we can follow `UnresolvedMapObjects`, to create a `UnresolvedCatalystToExternalMap`, and only create `CatalystToExternalMap` when the input map expression is resolved and the data type is known.
## How was this patch tested?
enable a old test case
Closesapache#22812 from cloud-fan/map.
Authored-by: Wenchen Fan <wenchen@databricks.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@viiryaviiryaviirya left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cloud-fan@SparkQA@viirya