Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SPARK-13540][SQL] Supports using nested classes within Scala objects as Dataset element type #11421

Closed

Conversation

liancheng
Copy link
Contributor

What changes were proposed in this pull request?

Nested classes defined within Scala objects are translated into Java static nested classes. Unlike inner classes, they don't need outer scopes. But the analyzer still thinks that an outer scope is required.

This PR fixes this issue simply by checking whether a nested class is static before looking up its outer scope.

How was this patch tested?

A test case is added to DatasetSuite. It checks contents of a Dataset whose element type is a nested class declared in a Scala object.

@liancheng liancheng changed the title Supports Scala object as outer scope [SPARK-13540][SQL] Supports Scala object as outer scope Feb 28, 2016
@liancheng
Copy link
Contributor Author

cc @cloud-fan

@SparkQA
Copy link

SparkQA commented Feb 28, 2016

Test build #52147 has finished for PR 11421 at commit dc225af.

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

@liancheng liancheng changed the title [SPARK-13540][SQL] Supports Scala object as outer scope [SPARK-13540][SQL] Supports using nested classes within Scala objects as Dataset element type Feb 28, 2016
@SparkQA
Copy link

SparkQA commented Feb 28, 2016

Test build #52148 has finished for PR 11421 at commit d8ca6d7.

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

@@ -559,7 +561,10 @@ class Analyzer(
}

resolveExpression(unbound, LocalRelation(attributes), throws = true) transform {
case n: NewInstance if n.outerPointer.isEmpty && n.cls.isMemberClass =>
case n: NewInstance
Copy link
Contributor

Choose a reason for hiding this comment

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

would be great to add some inline documentation what about these conditions are checking

@liancheng
Copy link
Contributor Author

Thanks for the review! Comments addressed.

case n: NewInstance if n.outerPointer.isEmpty && n.cls.isMemberClass =>
case n: NewInstance
// If this is an inner class of another class, register the outer object in `OuterScopes`.
// Note that static inner classes (e.g., inner classes within Scala objects. don't need
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: (e.g. inner classes within Scala objects)

@cloud-fan
Copy link
Contributor

LGTM except some minor comments.

@liancheng liancheng force-pushed the spark-13540-object-as-outer-scope branch from 28e1139 to 4ce71ce Compare February 29, 2016 06:02
@SparkQA
Copy link

SparkQA commented Feb 29, 2016

Test build #52164 has finished for PR 11421 at commit 28e1139.

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

@liancheng
Copy link
Contributor Author

retest this please

@SparkQA
Copy link

SparkQA commented Feb 29, 2016

Test build #52171 has finished for PR 11421 at commit 4ce71ce.

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

@liancheng liancheng force-pushed the spark-13540-object-as-outer-scope branch from 4ce71ce to e707881 Compare February 29, 2016 08:10
@SparkQA
Copy link

SparkQA commented Feb 29, 2016

Test build #52173 has finished for PR 11421 at commit 4ce71ce.

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

@SparkQA
Copy link

SparkQA commented Feb 29, 2016

Test build #52178 has finished for PR 11421 at commit e707881.

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

@cloud-fan
Copy link
Contributor

retest this please.

@SparkQA
Copy link

SparkQA commented Feb 29, 2016

Test build #52185 has finished for PR 11421 at commit e707881.

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

@liancheng
Copy link
Contributor Author

I'm merging this to master as it blocks #11431.

@asfgit asfgit closed this in 916fc34 Feb 29, 2016
@liancheng liancheng deleted the spark-13540-object-as-outer-scope branch February 29, 2016 17:12
roygao94 pushed a commit to roygao94/spark that referenced this pull request Mar 22, 2016
… as Dataset element type

## What changes were proposed in this pull request?

Nested classes defined within Scala objects are translated into Java static nested classes. Unlike inner classes, they don't need outer scopes. But the analyzer still thinks that an outer scope is required.

This PR fixes this issue simply by checking whether a nested class is static before looking up its outer scope.

## How was this patch tested?

A test case is added to `DatasetSuite`. It checks contents of a Dataset whose element type is a nested class declared in a Scala object.

Author: Cheng Lian <[email protected]>

Closes apache#11421 from liancheng/spark-13540-object-as-outer-scope.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants