Skip to content

[SPARK-22668][SQL] Ensure no global variables in arguments of method split by CodegenContext.splitExpressions() #20021

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
cloud-fan wants to merge 5 commits into from
Closed

[SPARK-22668][SQL] Ensure no global variables in arguments of method split by CodegenContext.splitExpressions() #20021

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
18ec598
Ensure no global variables in arguments of method split by CodegenCon…
cloud-fan Dec 19, 2017
f1afb92
fix stack
cloud-fan Dec 19, 2017
3d44195
address comments
cloud-fan Dec 20, 2017
95d0c1b
Merge remote-tracking branch 'origin/master' into codegen
cloud-fan Dec 21, 2017
900f246
only check in test
cloud-fan Dec 21, 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
Prev Previous commit
only check in test
  • Loading branch information
@cloud-fan
cloud-fan committed Dec 21, 2017
commit 900f246579f86f187b61454df6b0f76c8d5052c8
12 changes: 7 additions & 5 deletions ...lyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,11 +943,13 @@ class CodegenContext {
// inline execution if only one block
blocks.head
} else {
// Passing global variables to the split method is dangerous, as any mutating to it is
// ignored and may lead to unexpected behavior.
arguments.foreach { case (_, name) =>
assert(!mutableStateNames.contains(name),
s"[BUG] split function argument $name cannot be a global variable.")
if (Utils.isTesting) {
Copy link
Member

@viirya viirya Dec 20, 2017

Choose a reason for hiding this comment

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

Do we only do the assert in testing? Because passing global variables won't raise compile error, if we have any global variables passed in when not in testing, the codegen still work and may lead to wrong result.

Copy link
Contributor

@mgaido91 mgaido91 Dec 20, 2017

Choose a reason for hiding this comment

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

as you said, it may lead, but likely it doesn't. Then I do think that the best option is to assert it only in testing, where this might help finding potential bugs. In production it is an overkill to throw an exception for a situation which most likely is not a problem IMHO.

Copy link
Member

@kiszk kiszk Dec 20, 2017

Choose a reason for hiding this comment

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

Here is a discussion about this testing.

// Passing global variables to the split method is dangerous, as any mutating to it is
// ignored and may lead to unexpected behavior.
arguments.foreach { case (_, name) =>
assert(!mutableStateNames.contains(name),
s"split function argument $name cannot be a global variable.")
}
}

val func = freshName(funcName)
Expand Down