[SPARK-18398][SQL] Fix nullabilities of MapObjects and ExternalMapToCatalyst.

[ueshin]

What changes were proposed in this pull request?

The nullabilities of MapObject can be made more strict by relying on inputObject.nullable and lambdaFunction.nullable.

Also ExternalMapToCatalyst.dataType can be made more strict by relying on valueConverter.nullable.

How was this patch tested?

Existing tests.

[SparkQA]

Test build #68462 has finished for PR 15840 at commit b287ded.

• This patch fails PySpark unit tests.
• This patch merges cleanly.
• This patch adds no public classes.

[ueshin]

Jenkins, retest this please.

[hvanhovell]

On a whole this PR looks good. On a more general level, I would like to suggest that we should have a more generic way of preventing unneeded null-checks.

[SparkQA]

Test build #68466 has finished for PR 15840 at commit b287ded.

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

[kiszk]

Do we prepare a utility method like this in CodeGenerator?

def genNullableAssignment(genIsNull: boolean, isNull: boolean, lhs: String, rhs: String): String = {
  if (genIsNull) {
    s"""
      if ($isNull) {
        $lhs = null;
      } else {
        $lhs = rhs;
      }
    """
  } else {
    s"$lhs = $rhs;"
  }
}

[hvanhovell]

@kiszk that could be a start. However I would like to take a step back, and see which nullability checking patterns are common, and provide generic utilities for them. I also think that the proper use of NullIntolerant could also lead to significant improvement.

[kiszk]

@hvanhovell Thank for pointing out NullIntolerant. I did not know it. I also provide it as a starting point based on two instances that I saw today.
I agree that it would be good to add NullIntolerant to other classes and finding common patterns.

[ueshin]

I found that we could use CodegenContext.nullSafeExec() for most case, which is not so generic but we can easily replace nullability checking for now.

[ueshin]

@hvanhovell @kiszk I tried to use CodegenContext.nullSafeExec() in MapObjects as an example.
If you can bear with this for now, I'll apply it to other places to generate nullability checking codes.

[SparkQA]

Test build #68509 has finished for PR 15840 at commit deffccd.

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

[kiszk]

Does this approach avoid to generate code convertedArray[loopIndex] = null; if lambdaFunction.nullable is true? Is it a design? I know that it works correctly in this case since an array allocation ensures that all of the elements have null.

I am curious that whether the current restriction (generate only if-then) in nullSafeExec() works in other places. What do you think?

[ueshin]

@kiszk I have not checked all the case yet but I think the case that we need to generate else-clause doesn't match the case we discuss here.
Of course we can add the method you suggested when we find there is the case and let me know if you find it.

[kiszk]

I agree that this case is fine.

[ueshin]

I replaced nullability checking where we can use CodegenContext.nullSafeExec().

[ueshin]

@hvanhovell The pattern basically I used to replace nullability checking here is like:

val eval = child.genCode(ctx)
ev.copy(code = s"""
  ${eval.code}
  boolean ${ev.isNull} = ${eval.isNull};
  ${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
  if (!${ev.isNull}) {
      ${ev.value} = something;
  }
""")

into:

val eval = child.genCode(ctx)
ev.copy(code = s"""
  ${eval.code}
  ${ctx.javaType(dataType)} ${ev.value} = ${ctx.defaultValue(dataType)};
""" +
  ctx.nullSafeExec(child.nullable, eval.isNull)(s"""
    ${ev.value} = something;
  """), isNull = child.isNull)

Some of them are modified to fit each case.

Does this help you think about generic utilities?

[SparkQA]

Test build #68607 has finished for PR 15840 at commit 0fc36a8.

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

[SparkQA]

Test build #68627 has finished for PR 15840 at commit 658c5e3.

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

[SparkQA]

Test build #68629 has finished for PR 15840 at commit ec0c55c.

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

[ueshin]

@hvanhovell I'd like to revert some of commits in this pr which replace null checking with ctx.nullSafeExec() to focus on the original purpose of this pr, and send another pr to discuss null checking way, use ctx.nullSafeExec() or provide generic utilities for them as you suggested.
What do you think?

[ueshin]

@hvanhovell Could you review this pr again, please?

I reverted some commits which replace null checking with ctx.nullSafeExec() and then modified some null checkings to ${isNull} ? null : ${value} pattern in objects.scala because Janino can optimize codes like false ? a : b to b, which we found at #15901.
We expect isNull == "false" for expr.nullable = false (not always, though), so we can expect Janino will optimize them.

[hvanhovell]

LGTM - pending jenkins.

Small question: is it really easier for Janino to optimize ternary operators than an if/else block?

[ueshin]

@hvanhovell Ah, good question.

I checked the Janino code around IfStatement and I found Janino seems to optimize if/else block, too:
https://github.com/janino-compiler/janino/blob/22a7787e62037049e337cb1ce4064c29a4856022/janino/src/main/java/org/codehaus/janino/UnitCompiler.java#L2275
So we didn't need to replace them.

I'll revert them.

[ueshin]

Finally the purpose of this pr is to fix nullabilities of MapObjects and ExternalMapToCatalyst, I'll update pr title and description.

[hvanhovell]

retest this please

[SparkQA]

Test build #68931 has finished for PR 15840 at commit 78d9e80.

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

[SparkQA]

Test build #68933 has finished for PR 15840 at commit e777fbc.

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

[SparkQA]

Test build #68934 has finished for PR 15840 at commit e777fbc.

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

[hvanhovell]

Merging to master/2.1. Thanks!