Skip to content

[SPARK-22693][SQL] CreateNamedStruct and InSet should not use global variables #19896

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

Closed
mgaido91 wants to merge 5 commits into from
Closed

[SPARK-22693][SQL] CreateNamedStruct and InSet should not use global variables #19896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ac7131f
[SPARK-22693][SQL] InSet and CreateNamedStruct should not use global …
mgaido91 Dec 5, 2017
91565d1
Merge branch 'master' of github.com:apache/spark into SPARK-22693
mgaido91 Dec 6, 2017
b5bd951
revert changes for CreateNamedStruct
mgaido91 Dec 6, 2017
35f22ae
Revert "revert changes for CreateNamedStruct"
mgaido91 Dec 6, 2017
36537d8
address code style comments
mgaido91 Dec 6, 2017
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
27 changes: 15 additions & 12 deletions ...atalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/complexTypeCreator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -356,22 +356,25 @@ case class CreateNamedStruct(children: Seq[Expression]) extends CreateNamedStruc
override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
val rowClass = classOf[GenericInternalRow].getName
val values = ctx.freshName("values")
ctx.addMutableState("Object[]", values, s"$values = null;")
val valCodes = valExprs.zipWithIndex.map { case (e, i) =>
val eval = e.genCode(ctx)
s"""
|${eval.code}
|if (${eval.isNull}) {
| $values[$i] = null;
|} else {
| $values[$i] = ${eval.value};
|}
""".stripMargin
}
val valuesCode = ctx.splitExpressionsWithCurrentInputs(
valExprs.zipWithIndex.map { case (e, i) =>
val eval = e.genCode(ctx)
s"""
${eval.code}
if (${eval.isNull}) {
$values[$i] = null;
} else {
$values[$i] = ${eval.value};
}"""
})
expressions = valCodes,
funcName = "createNamedStruct",
extraArguments = "Object[]" -> values :: Nil)

ev.copy(code =
s"""
|$values = new Object[${valExprs.size}];
|Object[] $values = new Object[${valExprs.size}];
Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Dec 6, 2017

Choose a reason for hiding this comment

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

actually I'm not very sure about this. The previous code can avoid creating this array every time.

Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Dec 6, 2017

Choose a reason for hiding this comment

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

oh it didn't, but it can, you know my point...

Copy link
Copy Markdown
Contributor Author

@mgaido91 mgaido91 Dec 6, 2017

Choose a reason for hiding this comment

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

why? It is creating it every time (new Object[${valExprs.size}]), the only thing which is reused is the pointer to the array.

Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Dec 6, 2017

Choose a reason for hiding this comment

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

I mean it can reuse the array...

Copy link
Copy Markdown
Contributor Author

@mgaido91 mgaido91 Dec 6, 2017

Choose a reason for hiding this comment

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

I see what you mean now. Since currently this is not done, what should we do? Should I create a new PR to reuse the array and remove this from here?

Copy link
Copy Markdown
Contributor

@cloud-fan cloud-fan Dec 6, 2017

Choose a reason for hiding this comment

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

yea sounds good

|$valuesCode
|final InternalRow ${ev.value} = new $rowClass($values);
|$values = null;
Expand Down
22 changes: 11 additions & 11 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/predicates.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,17 +344,17 @@ case class InSet(child: Expression, hset: Set[Any]) extends UnaryExpression with
} else {
""
}
ctx.addMutableState(setName, setTerm,
s"$setTerm = (($InSetName)references[${ctx.references.size - 1}]).getSet();")
ev.copy(code = s"""
${childGen.code}
boolean ${ev.isNull} = ${childGen.isNull};
boolean ${ev.value} = false;
if (!${ev.isNull}) {
${ev.value} = $setTerm.contains(${childGen.value});
$setNull
}
""")
ev.copy(code =
s"""
|${childGen.code}
|${ctx.JAVA_BOOLEAN} ${ev.isNull} = ${childGen.isNull};
|${ctx.JAVA_BOOLEAN} ${ev.value} = false;
|if (!${ev.isNull}) {
| $setName $setTerm = (($InSetName)references[${ctx.references.size - 1}]).getSet();
| ${ev.value} = $setTerm.contains(${childGen.value});
| $setNull
|}
""".stripMargin)
}

override def sql: String = {
Expand Down
7 changes: 7 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/ComplexTypeSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ package org.apache.spark.sql.catalyst.expressions
import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.catalyst.analysis.UnresolvedExtractValue
import org.apache.spark.sql.catalyst.dsl.expressions._
import org.apache.spark.sql.catalyst.expressions.codegen.CodegenContext
import org.apache.spark.sql.types._
import org.apache.spark.unsafe.types.UTF8String

Expand Down Expand Up @@ -299,4 +300,10 @@ class ComplexTypeSuite extends SparkFunSuite with ExpressionEvalHelper {
new StringToMap(Literal("a=1_b=2_c=3"), Literal("_"), NonFoldableLiteral("="))
.checkInputDataTypes().isFailure)
}

test("SPARK-22693: CreateNamedStruct should not use global variables") {
val ctx = new CodegenContext
CreateNamedStruct(Seq("a", "x", "b", 2.0)).genCode(ctx)
assert(ctx.mutableStates.isEmpty)
}
}
7 changes: 7 additions & 0 deletions sql/catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/PredicateSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import org.apache.spark.SparkFunSuite
import org.apache.spark.sql.RandomDataGenerator
import org.apache.spark.sql.catalyst.InternalRow
import org.apache.spark.sql.catalyst.encoders.ExamplePointUDT
import org.apache.spark.sql.catalyst.expressions.codegen.CodegenContext
import org.apache.spark.sql.catalyst.util.{ArrayData, GenericArrayData}
import org.apache.spark.sql.types._

Expand Down Expand Up @@ -429,4 +430,10 @@ class PredicateSuite extends SparkFunSuite with ExpressionEvalHelper {
val infinity = Literal(Double.PositiveInfinity)
checkEvaluation(EqualTo(infinity, infinity), true)
}

test("SPARK-22693: InSet should not use global variables") {
val ctx = new CodegenContext
InSet(Literal(1), Set(1, 2, 3, 4)).genCode(ctx)
assert(ctx.mutableStates.isEmpty)
}
}