Skip to content

Conversation

@maropu
Copy link
Member

@maropu maropu commented Oct 26, 2019

What changes were proposed in this pull request?

This pr is to fix wrong code to check parameter lengths of split methods in subexpressionEliminationForWholeStageCodegen.

Why are the changes needed?

Bug fix.

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Existing tests.

val inputVarsForAllFuncs = commonExprs.map { expr =>
getLocalInputVariableValues(this, expr.head).toSeq
}
if (inputVarsForAllFuncs.map(calculateParamLengthFromExprValues).forall(isValidParamLength)) {
Copy link
Member Author

Choose a reason for hiding this comment

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

@viirya @cloud-fan sorry, but I found my mistake made in SPARK-29008... Could you check?

Copy link
Member

Choose a reason for hiding this comment

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

yea, good catch! We should check param length for input parameters to common expressions.

@SparkQA
Copy link

SparkQA commented Oct 26, 2019

Test build #112714 has finished for PR 26267 at commit 6b81cef.

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

@maropu
Copy link
Member Author

maropu commented Oct 27, 2019

@viirya many thanks!

Copy link
Member

@dongjoon-hyun dongjoon-hyun left a comment

Choose a reason for hiding this comment

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

+1, LGTM, too. Thank you, @maropu and @viirya .
Merged to master.

@maropu
Copy link
Member Author

maropu commented Oct 28, 2019

Thanks for the merging, @dongjoon-hyun !

@cloud-fan
Copy link
Contributor

late LGTM, good catch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants