[SPARK-16100] [SQL] avoid crash in TreeNode.withNewChildren

[inouehrs]

What changes were proposed in this pull request?

This patch fixes a reported bug, which causes IndexOutOfBoundsException during the code generation.
This issue is cased for a MapObjects instance which has the same object for loopVar and lambdaFunction.

case class MapObjects private(
    loopVar: LambdaVariable,
    lambdaFunction: Expression,
    inputData: Expression) extends Expression with NonSQLExpression {

  override def children: Seq[Expression] = lambdaFunction :: inputData :: Nil

In such case, loopVar is handled as a child in TreeNode.withNewChildren since the object is included in children as lambdaFunction.
I add checks for the number of unprocessed children to avoid this issue.
Is there any idea on cleaner way to avoid such a pathological case?

I also add assert to confirm that the mapping Children and productElement is correct.

How was this patch tested?

Using existing unit tests

[AmplabJenkins]

Can one of the admins verify this patch?

[hvanhovell]

Could you add a test case to reproduce this?

[cloud-fan]

hi @inouehrs , sorry I haven't noticed that you already have a PR for this JIRA ticket.
Yea the root cause of this bug is exactly what you described in your PR descption, thanks for finding this out!
However, I don't think adding a check to stop processing the children in TreeNode.withNewChildren is a good fix, we are replacing the wrong children there.
Instead, I think we should avoid mistakenly treating MapOjects.loopVar as a child. Do you mind reviewing my PR at #13835? thanks!

[inouehrs]

Hi @cloud-fan, your fix looks cleaner than mine.
As a TODO comment at withNewChildren says, adding validation of the order of children somewhere will help avoiding future bugs caused by classes other than MapObjects.
Thank you.

[JoshRosen]

@inouehrs, it looks like this issue has been resolved by #13835, so do you mind closing this PR? Thanks!