Skip to content

Conversation

@mgaido91
Copy link
Contributor

What changes were proposed in this pull request?

When a ScalaUDF returns a value which overflows, currently it returns null regardless of the value of the config spark.sql.decimalOperations.nullOnOverflow.

The PR makes it respect the above-mentioned config and behave accordingly.

How was this patch tested?

added UT

@SparkQA
Copy link

SparkQA commented Jul 13, 2019

Test build #107630 has finished for PR 25144 at commit cc95f00.

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

throw new AnalysisException(s"Overflow when setting precision to $newPrecision")
}
res
toPrecision(newPrecision, 0, ROUND_CEILING, nullOnOverflow = false)
Copy link
Member

@viirya viirya Jul 15, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This throws exception on overflow, I think it preserves current behavior, but don't we want to respect decimalOperationsNullOnOverflow here too?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well, I don't think that is really an issue here. I mean, I see no way ceil and floor can produce an overflow, they rather reduce the needed precision. So I think this case cannot really happen and it is fine to just throw an exception

@mgaido91
Copy link
Contributor Author

cc @JoshRosen @cloud-fan

@cloud-fan
Copy link
Contributor

thanks, merging to master!

@cloud-fan cloud-fan closed this in a783690 Jul 22, 2019
@mgaido91
Copy link
Contributor Author

thanks @cloud-fan @dongjoon-hyun @viirya!

yiheng pushed a commit to yiheng/spark that referenced this pull request Jul 24, 2019
…n ScalaUDF result

## What changes were proposed in this pull request?

When a `ScalaUDF` returns a value which overflows, currently it returns null regardless of the value of the config `spark.sql.decimalOperations.nullOnOverflow`.

The PR makes it respect the above-mentioned config and behave accordingly.

## How was this patch tested?

added UT

Closes apache#25144 from mgaido91/SPARK-28369.

Authored-by: Marco Gaido <marcogaido91@gmail.com>
Signed-off-by: Wenchen Fan <wenchen@databricks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants