Use IfExpr to check when input to log2 is <=0 and return null

[PedroMDuarte]

Which issue does this PR close?

Closes #485

Rationale for this change

Compatibility with how Spark handles logarithms of values <=0.

What changes are included in this PR?

Use IfExpr to check when input to log2 is <=0 and return null. This is done to match Spark's behavior, which in turn is implemented to match Hive's behavior.

How are these changes tested?

The existing test for ln, log2 and log10 was modified so that it includes negative numbers as part of the inputs being tested.

[andygrove]

This is looking good so far. Thanks @PedroMDuarte

[viirya]

Hmm, please don't put this special handling in create_scalar_function_expr. Could you move it to create_comet_physical_fun where special handling of scalar functions should be located?

[PedroMDuarte]

Thanks for bringing that up!

Just to clarify, if we were to do this as part of create_comet_physical_fun then it would be all reimplemented at a lower level and not able to reuse the Datafusion Log2 implementation via an IfExpr. Is that right?

Happy to go either way. @andygrove's original hint mentioned using IfExpr, but I couldn't quite get that working inside of create_comet_physical_fun because it doesn't have access to the arg exprs.

[andygrove]

Apologies for the delay in responding here. I will start reviewing this on Monday.

[kazuyukitanimura]

I would rather do this in QueryPlanSerde, E.g.

        case Log2(child) =>
          val childExpr = exprToProtoInternal(nullIfNegative(child), inputs)

def nullIfNegative(expression: Expression): {
    val zero = Literal.default(expression.dataType)
    If(LessThanOrEqual(expression, zero), Literal.create(null, expression.dataType), expression)
}

See nullIfWhenPrimitive for a similar case.

[PedroMDuarte]

I would rather do this in QueryPlanSerde, E.g.

        case Log2(child) =>
          val childExpr = exprToProtoInternal(nullIfNegative(child), inputs)

def nullIfNegative(expression: Expression): {
    val zero = Literal.default(expression.dataType)
    If(LessThanOrEqual(expression, zero), Literal.create(null, expression.dataType), expression)
}

See nullIfWhenPrimitive for a similar case.

thanks for this suggestion!

[andygrove]

LGTM pending CI. I confirmed that the change to the unit test does demonstrate the incorrect behavior. Thanks @PedroMDuarte

[andygrove]

Could you also update this section in expressions.md

| Log        | log(0) will produce `-Infinity` unlike Spark which returns `null`   |
| Log2       | log2(0) will produce `-Infinity` unlike Spark which returns `null`  |
| Log10      | log10(0) will produce `-Infinity` unlike Spark which returns `null` |

[kazuyukitanimura]

hmm, ci is failing... looks CI passed

[PedroMDuarte]

thanks for the guidance here, really appreciate you all taking the time to show me how to contribute this fix.

[kazuyukitanimura]

Merged, thanks @PedroMDuarte @andygrove

[kazuyukitanimura]

And thanks @viirya