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-11750][SQL] revert SPARK-11727 and code clean up #9726

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
3 participants
@cloud-fan
Copy link
Contributor

commented Nov 16, 2015

After some experiment, I found it's not convenient to have separate encoder builders: FlatEncoder and ProductEncoder. For example, when create encoders for ScalaUDF, we have no idea if the type T is flat or not. So I revert the splitting change in #9693, while still keeping the bug fixes and tests.

@cloud-fan

View changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala Outdated
@@ -718,39 +630,4 @@ trait ScalaReflection {
throw new UnsupportedOperationException(s"Schema for type $other is not supported")
}
}

def typeOfObject: PartialFunction[Any, DataType] = {

This comment has been minimized.

Copy link
@cloud-fan

cloud-fan Nov 16, 2015

Author Contributor

This method is only used in tests, so I think we should remove it and its related tests.

val inputObject = child.gen(ctx)

s"""
${inputObject.code}

boolean ${ev.isNull} = false;
scala.Option<$javaType> ${ev.value} =
scala.Option ${ev.value} =

This comment has been minimized.

Copy link
@cloud-fan

cloud-fan Nov 16, 2015

Author Contributor

Janino doesn't support generic type, so we don't need to pass in the optionType.

@cloud-fan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 16, 2015

@SparkQA

This comment has been minimized.

Copy link

commented Nov 16, 2015

Test build #45980 has finished for PR 9726 at commit 6dbc124.

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

View changes

sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/ScalaReflection.scala Outdated
val universe: scala.reflect.api.Universe

/** The mirror used to access types in the universe */
def mirror: universe.Mirror

This comment has been minimized.

Copy link
@marmbrus

marmbrus Nov 16, 2015

Contributor

This level of indirection is on purpose. It lets us use the same reflection code both at runtime and in macros. Even if we aren't using that right now, we shouldn't undo the work that was done to make this possible.

@cloud-fan cloud-fan force-pushed the cloud-fan:follow branch Nov 17, 2015

@SparkQA

This comment has been minimized.

Copy link

commented Nov 17, 2015

Test build #46070 has finished for PR 9726 at commit 37fa83f.

  • This patch fails from timeout after a configured wait of 250m.
  • This patch merges cleanly.
  • This patch adds no public classes.
@cloud-fan

This comment has been minimized.

Copy link
Contributor Author

commented Nov 17, 2015

retest this please.

@SparkQA

This comment has been minimized.

Copy link

commented Nov 17, 2015

Test build #46083 has finished for PR 9726 at commit 37fa83f.

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

@cloud-fan cloud-fan force-pushed the cloud-fan:follow branch to d0a294e Nov 19, 2015

@@ -736,39 +645,4 @@ trait ScalaReflection {
assert(methods.length == 1)
methods.head.getParameterTypes
}

def typeOfObject: PartialFunction[Any, DataType] = {

This comment has been minimized.

Copy link
@cloud-fan

cloud-fan Nov 19, 2015

Author Contributor

This method is only used in tests, so I think we should remove it and its related tests.

This comment has been minimized.

Copy link
@marmbrus

marmbrus Nov 19, 2015

Contributor

Probably fine in this case, but this isn't related to the undoing of the refactoring and mixing concerns makes the PR much harder to review.

@SparkQA

This comment has been minimized.

Copy link

commented Nov 19, 2015

Test build #46314 has finished for PR 9726 at commit d0a294e.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.
// We convert the not-serializable TypeTag into StructType and ClassTag.
val mirror = typeTag[T].mirror
val cls = mirror.runtimeClass(typeTag[T].tpe)
val flat = !classOf[Product].isAssignableFrom(cls)

This comment has been minimized.

Copy link
@marmbrus

marmbrus Nov 19, 2015

Contributor

This isn't going to work for java. We should just leave this as it was.

This comment has been minimized.

Copy link
@cloud-fan

cloud-fan Nov 20, 2015

Author Contributor

But we need a way to detect flatness automatically, sometimes we only have the type T and nothing else, like when we wanna use encoders for ScalaUDF. How about flat = !ScalaReflection.schemaFor[T].isInstanceOf[StructType])?

asfgit pushed a commit that referenced this pull request Nov 19, 2015

[SPARK-11750][SQL] revert SPARK-11727 and code clean up
After some experiment, I found it's not convenient to have separate encoder builders: `FlatEncoder` and `ProductEncoder`. For example, when create encoders for `ScalaUDF`, we have no idea if the type `T` is flat or not. So I revert the splitting change in #9693, while still keeping the bug fixes and tests.

Author: Wenchen Fan <wenchen@databricks.com>

Closes #9726 from cloud-fan/follow.

(cherry picked from commit 47d1c23)
Signed-off-by: Michael Armbrust <michael@databricks.com>

@asfgit asfgit closed this in 47d1c23 Nov 19, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.