Support round and bround SQL functions

[nartal1]

This fixes #37

[nartal1]

There is a bug in this which I am still figuring out. All test cases do not pass.
If the scale is 0, the value is incremented by 1(which shouldn't happen).
If I have something like below:

df1 = spark.createDataFrame(data=[(Decimal('3977631')), (Decimal('8291028')), (Decimal('8291025'))], schema=DecimalType(7,0))
ret = df2.selectExpr('round(value, 0)')

CPU result: (Decimal('3977631')), (Decimal('8291028')), (Decimal('8291025'))
GPU result: (Decimal('3977632')), (Decimal('8291029')), (Decimal('8291026'))

[andygrove]

rhs could be null at this point so needs a null check here. Perhaps it would be better to introduce something similar to the withResource method that can work with Any's that are possibly also AutoCloseable?

[revans2]

I added on

spark-rapids/sql-plugin/src/main/scala/com/nvidia/spark/rapids/Arm.scala

Lines 52 to 62 in cce1ec1

	/** Executes the provided code block and then closes the value if it is AutoCloseable */
	def withResourceIfAllowed[T, V](r: T)(block: T => V): V = {
	try {
	block(r)
	} finally {
	r match {
	case c: AutoCloseable => c.close()
	case _ => //NOOP
	}
	}
	}

[andygrove]

It turns out we already have a withResource that can work here.

withResourceIfAllowed(left.columnarEval(batch)) { lhs =>
  withResourceIfAllowed(right.columnarEval(batch)) { rhs =>
    (lhs, rhs) match {
      case (l: GpuColumnVector, r) =>
        withResource(GpuScalar.from(r, right.dataType)) { scalar =>
          GpuColumnVector.from(doColumnar(l, scalar), dataType)
        }
      case _ => null
    }
  }
}

[revans2]

But further to the point why is this not a GpuBinaryExpression? BRound and Round are both BinaryExpressions which would clean up this code.

[nartal1]

Thanks @andygrove and @revans2 for taking a look. Sorry missed this conversation. The reason I was not making it as GpuBinaryExpression is because there isn't an BinaryOp for round or BRound in JNI or in cudf binaryops. And I was not sure what would be the overidden enum be for these operators. But this also isn't working for all cases. So I will try how to go about it as a GpuBinaryExpression

[revans2]

GpuBinaryExpression does not assume an enum but CudfBinaryExpression does.

[nartal1]

Thanks! changed it to use GpuBinaryExpression. Will be in next patch.

[revans2]

round and bround support all numeric types, not just decimal (float, int, byte, double, etc). Do we have tests for these too?

[nartal1]

Yes, will add tests for other numeric types.

[nartal1]

Added tests for other numeric types.

[nartal1]

Cleaned up the code to use GpuBinaryExpression.
Need to add numerical tests and also verify decimal type for negative scale

[nartal1]

There is a bug in this which I am still figuring out. All test cases do not pass.
If the scale is 0, the value is incremented by 1(which shouldn't happen).
If I have something like below:

df1 = spark.createDataFrame(data=[(Decimal('3977631')), (Decimal('8291028')), (Decimal('8291025'))], schema=DecimalType(7,0))
ret = df2.selectExpr('round(value, 0)')

CPU result: (Decimal('3977631')), (Decimal('8291028')), (Decimal('8291025'))
GPU result: (Decimal('3977632')), (Decimal('8291029')), (Decimal('8291026'))

I couldn't figure out the issue here as the code seems straight forward. So, switched to see if i can reproduce it in Java code. This is reproducible in Java side as well. So have to debug it in cudf Java

[nartal1]

@revans2 Could you please suggest how we handle overflow when for each types.
For example(considering short type), pyspark results in 0 for the below round operation:

>>> df2= spark.createDataFrame(data=[32562], schema=ShortType())
>>> ret = df2.selectExpr('round(value, -5)')
>>> ret.show()
+----------------+
|round(value, -5)|
+----------------+
|               0|
+----------------+

But we see different GPU result(-31072) as overflow results in undefined behavior in libcudf. 
Should we throw an exception whenever we sense an overflow for each type at this point ? 

[revans2]

The problem is completely in how we implement round/bround vs how spark does it, and I am not 100% sure how to make them sync up without a lot of work on the cudf side for these corner cases.

cudf tries to do the round on the native type, which can result in an overflow. Spark will convert the native value to a decimal value (128-bits if needed), set the scale to do the rounding, and then convert the value back (with some special cases for NaN and Infinite in floating point).

https://github.com/apache/spark/blob/0626901bcbeebceb6937001e1f32934c71876210/sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala#L1220-L1249

There can be no overflow in those cases because all of the processing is happening on 128-bits. For integer smaller than a long we could cast it to a long first, do the round/bround, and then cast it back. But we would still end up with issues in long because of overflow.

Similar with float/double.

[revans2]

Do we have tests for overflow to check if it is working correctly or are we going to mark the operators and incompatible until we can figure out a way to make it work properly?

[nartal1]

@revans2 I am waiting on another fix in cudf to get merged before starting the CI here. It would be great if you could please take another look and suggest if looks okay for this iteration. If further changes are required, I can make the changes and verify it locally.

[nartal1]

build

[nartal1]

build

[nartal1]

build

[revans2]

Do we have tests for overflow to check if it is working correctly or are we going to mark the operators and incompatible until we can figure out a way to make it work properly?

[revans2]

build

[revans2]

@andygrove you reviewed this before are you okay with merging it?

[andygrove]

LGTM