Fix divide-by-zero in GpuAverage with ansi mode

[abellina]

Fixes: #2078

[andygrove]

LGTM

[andygrove]

The change looks good but I was curious why we are using a different pattern to Spark which just has a plain boolean argument with a default value, rather than using an option. Was this necessary because of the way we're using the shim layer?

[abellina]

Yes, I tried other ways but couldn't think of something cleaner.

[gerashegalov]

I don't think we need a 3-value logic of Option[Boolean].

I think we can do it almost like Spark.

let us undo the change to DivModLike just make failOnError non-lazy and define

case class GpuDivide(
  left: Expression, right: Expression,
  override val failOnError: Boolean = ShimLoader.getSparkShims.shouldFailDivByZero()
) extends GpuDivModLike {

[abellina]

build

[gerashegalov]

nit: best practice Option(false) but I think we can get away with a simple Boolean

[gerashegalov]

I don't think we need a 3-value logic of Option[Boolean].

I think we can do it almost like Spark.

let us undo the change to DivModLike just make failOnError non-lazy and define

case class GpuDivide(
  left: Expression, right: Expression,
  override val failOnError: Boolean = ShimLoader.getSparkShims.shouldFailDivByZero()
) extends GpuDivModLike {

[gerashegalov]

let us try without failOnErrorOverride, just make failOnError non-lazy, so we can override it

[revans2]

You can override a lazy val

override lazy val failOnError: Boolean = failOnErrorOverride.getOrElse(GpuDivModeLike.failOnError)

But I am fine with keeping this as is.

[gerashegalov]

yes, I just meant you can't use lazy as a parameter.

[abellina]

@gerashegalov we don't have ANSI semantics in several of the operations. In the DivModLike ops alone, the failOnError flag is actually given (in Spark) to: Divide, IntegralDivide and Remainder. Each of these has as default SQLConf.get.ansiEnabled. In our case, Pmod falls under the DivModLike category also.

Doing a quick search in Spark, I am finding that:

• Divide: failOnError is passed only in the Average as per this PR.
• IntegralDivide: failOnError in case class signature but not overwritten.
• Reminder: failOnError in case class signature but not overwritten.
• Pmod: failOnError in case class signature but not overwritten.

So the only class that really needs it is GpuDivide, and it seems better (and less error prone) to me to have the call to the shim in a single place GpuDivModLike, with the override possibility, as only GpuDivide requires it.

That said, if we want to match Spark more, I can look into adding the failOnError to all the GpuDivModLikes. Interested to know if we need an issue to track ANSI work for the plugin in general, as it seems that many places have forks for this.

[gerashegalov]

@abellina this is the change I am suggesting in the nut shell. https://github.com/abellina/spark-rapids/compare/agg/fix_ansi_avg...gerashegalov:agg/fix_ansi_avg?expand=1

@revans2 if we made failOnError non-lazy then we don't need an override inside the GpuDivide case class body but could do it just as a param

[abellina]

@gerashegalov updated PR to incorporate your suggestion.

[gerashegalov]

LGTM

[abellina]

build