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.

Sign up for GitHub

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

fix GpuCreateNamedStruct not serializable issue #2442

Merged
merged 6 commits into from
May 20, 2021
Merged

fix GpuCreateNamedStruct not serializable issue #2442

Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
c3e07f0
fix GpuCreateNamedStruct not serializable issue
wbo4958 May 18, 2021
b7d2e26
resolve comments
wbo4958 May 19, 2021
bfe48e4
resolve comments
wbo4958 May 19, 2021
4034015
resolve comments
wbo4958 May 20, 2021
0ffc9a8
remove unused import
wbo4958 May 20, 2021
d194afe
resolve comments
wbo4958 May 20, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions sql-plugin/src/main/scala/org/apache/spark/sql/rapids/complexTypeCreator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
package org.apache.spark.sql.rapids

import ai.rapids.cudf.{ColumnVector, DType}
import com.nvidia.spark.rapids.{GpuColumnVector, GpuExpression, GpuExpressionsUtils}
import com.nvidia.spark.rapids.{GpuAlias, GpuColumnVector, GpuExpression, GpuExpressionsUtils, GpuLiteral}
import com.nvidia.spark.rapids.RapidsPluginImplicits.ReallyAGpuExpression

import org.apache.spark.sql.catalyst.analysis.{TypeCheckResult, TypeCoercion}
Expand Down Expand Up @@ -87,8 +87,17 @@ case class GpuCreateNamedStruct(children: Seq[Expression]) extends GpuExpression
case Seq(name, value) => (name, value)
}.toList.unzip

// Names will be serialized before Spark scheduling, and the returned type GpuScalar
// from GpuLiteral.columnarEval(null) can't be serializable, which causes
// `org.apache.spark.SparkException: Task not serializable` issue.
//
// And on the other hand, the calling for columnarEval(null) in the driver side is
// dangerous for GpuExpressions, we'll have to pull it apart manually.
private lazy val names = nameExprs.map {
case g: GpuExpression => g.columnarEval(null)
case gl: GpuLiteral => gl.value
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

case GpuAlias(gl: GpuLiteral, _) => gl.value
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

case ge: GpuExpression =>
throw new IllegalStateException(s"Unexpected GPU expression $ge")
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

@wbo4958 wbo4958 May 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator

@firestarman firestarman May 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

case e => e.eval(EmptyRow)
}

Expand Down