fix GpuCreateNamedStruct not serializable issue #2442

wbo4958 · 2021-05-18T23:30:00Z

The return from columnarEval of GpuLiteral has been changed to GpuScalar which
will be as the type of names(a private field in GpuCreateNamedStruct). And the
names will be serialized before spark scheduling, but the GpuScalar is not serializable,
it will cause "org.apache.spark.SparkException: Task not serializable".

On the other hand, the calling columnarEval(null) in the driver side is really dangerous,
This PR also pull it apart manually

Signed-off-by: Bobby Wang wbo4958@gmail.com

Closes #2427
Closes #2443

wbo4958 · 2021-05-18T23:30:12Z

build

wbo4958 · 2021-05-19T00:14:24Z

build

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala

wbo4958 · 2021-05-19T02:08:19Z

build

wbo4958 · 2021-05-19T12:30:27Z

@revans2 Could you help to review this PR

revans2 · 2021-05-19T13:02:35Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala

-    case g: GpuExpression => g.columnarEval(null)
+    case gl: GpuLiteral => gl.value
+    case ge: GpuExpression =>
+      throw new IllegalStateException(s"Unexpected GPU expression $ge")


I have tried to make it so the Literal is wrapped in an Alias, which should totally be valid, but I have not been able to make it happen with the public spark APIs. If we are going to throw an exception on something we don't expect, then we should be absolutely positive that we got what we expected, and all of the default literal checks being use allow for an alias. So either still do the eval and then pull the name out of the GpuScalar that is returned. Or add in a second rule for an Alias of a literal, just to be on the safe side.

still do the eval and then pull the name out of the GpuScalar that is returned.

We can't do this because this is happening on the driver side, correct? That's one of the drawbacks of the recent GpuScalar refactor -- it's practically impossible to call columnarEval without requiring a GPU to do it, so it's effectively off-limits to the driver. That would be a simpler way to handle any Alias or similar shenanigans, but since we can't call columnarEval or eval to solve it, we'll have to pull it apart manually instead IIUC.

I agree with Jason, and the new commit has added GpuAlias case. BTW, I see the name check has been limited to literal, Do we really add the GpuAlias case?

object CreateNamedStructCheck extends ExprChecks { val nameSig: TypeSig = TypeSig.lit(TypeEnum.STRING)

it's practically impossible to call columnarEval without requiring a GPU to do it, so it's effectively off-limits to the driver.

If a GpuScalar is created from a Scala value (GpuLiteral will create such a GpuScalar in its columnarEval) and only the getValue will be called to get the internal Scala value, then it is safe to use on the driver side without GPU. Becasue the GpuScalar for this case is totally a wrapper of a Scala value, nothing to do with GPU.

But I am a little confused. Could we call the columnarEval on driver side with an invalid input for a generic GPU expression before the GpuScalar refactor? I think it is still not always safe because the processing inside the columnarEval may require a GPU. It may be safe if we know the instance of the GPU expression. And if we know the instance of a GPU expression, I think we can use its direct methods or members instead.

revans2 · 2021-05-19T13:03:24Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala

@@ -82,7 +82,9 @@ case class GpuCreateNamedStruct(children: Seq[Expression]) extends GpuExpression
  }.toList.unzip

  private lazy val names = nameExprs.map {
-    case g: GpuExpression => g.columnarEval(null)
+    case gl: GpuLiteral => gl.value


The reason for this change is not clear. Could we get a comment to explain why this is happening so no one accidentally changes it back.

revans2

I left a comment before, but marking as needs changes because ti was already approved.

wbo4958 · 2021-05-19T23:08:54Z

build

The return from columnarEval of GpuLiteral has been changed to GpuScalar which will be as the type of names(a private field in GpuCreateNamedStruct). And the names will be serialized before scheduling, but the GpuScalar is not serializable, it will cause "org.apache.spark.SparkException: Task not serializable" Signed-off-by: Bobby Wang <wbo4958@gmail.com>

wbo4958 · 2021-05-19T23:14:28Z

since there are some conflicts, I rebased the PR

wbo4958 · 2021-05-19T23:14:34Z

build

jlowe · 2021-05-19T23:57:12Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala

  private lazy val names = nameExprs.map {
    case gl: GpuLiteral => gl.value
+    case GpuAlias(gl: GpuLiteral, _) => gl.value


Technically Alias can nest arbitrarily, so I think we'd want to match on the result of calling something like what GpuOverrides.extractLit does but for GpuLiteral or what Spark's AliasHelper.trimAliases does, probably handling GpuAlias since it's already been translated at that point.

Yeah, Good suggestion, from the safe side, I added the extractGpulit for this case.
Thx

wbo4958 · 2021-05-20T00:28:05Z

build

firestarman · 2021-05-20T02:17:09Z

LGTM

gerashegalov · 2021-05-20T02:49:00Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala

  private lazy val names = nameExprs.map {
-    case g: GpuExpression => g.columnarEval(null)
+    case ge: GpuExpression =>
+      GpuExpressionsUtils.extractGpuLit(ge) match {


nit:

GpuExpressionsUtils.extractGpuLit(ge).map(_.value) .getOrElse(throw new IllegalStateException(s"Unexpected GPU expression $ge"))

+1, but I would take it a step further because extractGpuLit takes an Expression as input not a GpuExpression and we should always get GpuExpressions in nameExprs or we have other problems.

private lazy val names = nameExprs.map { exp => GpuExpressionsUtils.extractGpuLit(exp) .getOrElse(throw new IllegalStateException(s"Unexpected Expression $exp")) }

Yeah, But if extractGpuLit takes a GpuExpression, then it's hard for GpuAlias whose child is Expression to do the recursive calling.

And Seems GpuAlias can wrap like AttributeReference which can't be cast to GpuExpression and it will make the extractGpuLit really complicated if it takes GpuExpressiion @revans2

I don't think you understood. I don't want extractGpuLit to take a GpuExpression. It is good like it is. I just don't think we need to check for the non-GpuExpression case when computing names. It should never happen so it is OK to throw an exception if we see it., Which is why I said we could simplify the processing to just be.

private lazy val names = nameExprs.map { exp => GpuExpressionsUtils.extractGpuLit(exp) .getOrElse(throw new IllegalStateException(s"Unexpected Expression $exp")) }

Oh, Yeah, You're right. Sorry for the misunderstanding.

revans2

The change looks fine, and I would be OK with checking it in as is, but I would like to see it cleaned up a bit.

sql-plugin/src/main/scala/com/nvidia/spark/rapids/GpuExpressions.scala

revans2 · 2021-05-20T10:45:03Z

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala

  private lazy val names = nameExprs.map {
-    case g: GpuExpression => g.columnarEval(null)
+    case ge: GpuExpression =>
+      GpuExpressionsUtils.extractGpuLit(ge) match {


+1, but I would take it a step further because extractGpuLit takes an Expression as input not a GpuExpression and we should always get GpuExpressions in nameExprs or we have other problems.

private lazy val names = nameExprs.map { exp => GpuExpressionsUtils.extractGpuLit(exp) .getOrElse(throw new IllegalStateException(s"Unexpected Expression $exp")) }

wbo4958 · 2021-05-20T12:47:49Z

build

revans2

not exactly what I wanted, but good enough.

* fix GpuCreateNamedStruct not serializable issue The return from columnarEval of GpuLiteral has been changed to GpuScalar which will be as the type of names(a private field in GpuCreateNamedStruct). And the names will be serialized before scheduling, but the GpuScalar is not serializable, it will cause "org.apache.spark.SparkException: Task not serializable" Signed-off-by: Bobby Wang <wbo4958@gmail.com> * resolve comments * resolve comments * resolve comments * remove unused import * resolve comments

wbo4958 requested a review from firestarman May 18, 2021 23:36

sameerz added the bug Something isn't working label May 19, 2021

sameerz added this to the May 10 - May 21 milestone May 19, 2021

pxLi changed the base branch from branch-0.6 to branch-21.06 May 19, 2021 01:07

firestarman reviewed May 19, 2021

View reviewed changes

sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala Outdated Show resolved Hide resolved

wbo4958 self-assigned this May 19, 2021

wbo4958 requested a review from firestarman May 19, 2021 02:06

firestarman previously approved these changes May 19, 2021

View reviewed changes

wbo4958 requested review from revans2 and jlowe May 19, 2021 03:08

revans2 reviewed May 19, 2021

View reviewed changes

revans2 requested changes May 19, 2021

View reviewed changes

wbo4958 dismissed firestarman’s stale review via 2e37d0d May 19, 2021 23:02

wbo4958 added 3 commits May 20, 2021 07:10

resolve comments

b7d2e26

resolve comments

bfe48e4

wbo4958 force-pushed the fix_serialize_bug branch from 2e37d0d to bfe48e4 Compare May 19, 2021 23:13

wbo4958 requested a review from revans2 May 19, 2021 23:15

jlowe reviewed May 20, 2021

View reviewed changes

resolve comments

4034015

remove unused import

0ffc9a8

gerashegalov reviewed May 20, 2021

View reviewed changes

revans2 previously approved these changes May 20, 2021

View reviewed changes

resolve comments

d194afe

wbo4958 dismissed revans2’s stale review via d194afe May 20, 2021 12:47

revans2 approved these changes May 20, 2021

View reviewed changes

razajafri merged commit 7b4d5af into NVIDIA:branch-21.06 May 20, 2021

wbo4958 deleted the fix_serialize_bug branch June 15, 2021 02:24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix GpuCreateNamedStruct not serializable issue #2442

fix GpuCreateNamedStruct not serializable issue #2442

wbo4958 commented May 18, 2021 •

edited

Loading

wbo4958 commented May 18, 2021

wbo4958 commented May 19, 2021

wbo4958 commented May 19, 2021

wbo4958 commented May 19, 2021 •

edited

Loading

revans2 May 19, 2021

jlowe May 19, 2021

wbo4958 May 19, 2021 •

edited

Loading

firestarman May 20, 2021 •

edited

Loading

revans2 May 19, 2021

wbo4958 May 19, 2021

revans2 left a comment

wbo4958 commented May 19, 2021

wbo4958 commented May 19, 2021 •

edited

Loading

wbo4958 commented May 19, 2021

jlowe May 19, 2021

wbo4958 May 20, 2021

wbo4958 May 20, 2021

wbo4958 commented May 20, 2021

firestarman commented May 20, 2021

gerashegalov May 20, 2021

wbo4958 May 20, 2021

revans2 May 20, 2021

wbo4958 May 20, 2021

wbo4958 May 20, 2021 •

edited

Loading

revans2 May 20, 2021

wbo4958 May 20, 2021

revans2 left a comment

revans2 May 20, 2021

wbo4958 commented May 20, 2021

revans2 left a comment

fix GpuCreateNamedStruct not serializable issue #2442

fix GpuCreateNamedStruct not serializable issue #2442

Conversation

wbo4958 commented May 18, 2021 • edited Loading

wbo4958 commented May 18, 2021

wbo4958 commented May 19, 2021

wbo4958 commented May 19, 2021

wbo4958 commented May 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbo4958 May 19, 2021 • edited Loading

Choose a reason for hiding this comment

firestarman May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revans2 left a comment

Choose a reason for hiding this comment

wbo4958 commented May 19, 2021

wbo4958 commented May 19, 2021 • edited Loading

wbo4958 commented May 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbo4958 commented May 20, 2021

firestarman commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbo4958 May 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

revans2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbo4958 commented May 20, 2021

revans2 left a comment

Choose a reason for hiding this comment

wbo4958 commented May 18, 2021 •

edited

Loading

wbo4958 commented May 19, 2021 •

edited

Loading

wbo4958 May 19, 2021 •

edited

Loading

firestarman May 20, 2021 •

edited

Loading

wbo4958 commented May 19, 2021 •

edited

Loading

wbo4958 May 20, 2021 •

edited

Loading