Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SPARK-26218][SQL][Follow up] Fix the corner case when casting float to Integer. #27151

Closed
wants to merge 9 commits into from
Closed

[SPARK-26218][SQL][Follow up] Fix the corner case when casting float to Integer. #27151

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
5647990
SPARK-26218: [Follow up] throw exception on overflow for integers
turboFei Jan 9, 2020
43866bc
address comments
turboFei Jan 9, 2020
e3f9191
fix style
turboFei Jan 9, 2020
4a78aba
fix ut
turboFei Feb 4, 2020
0534eb6
fix ut
turboFei Feb 4, 2020
ccad7e6
ut
turboFei Feb 5, 2020
0ac6e47
fix ut
turboFei Feb 5, 2020
356284d
trigger jenkins
turboFei Feb 5, 2020
4d47a49
trigger
turboFei Feb 5, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions sql/catalyst/src/main/scala/org/apache/spark/sql/types/numerics.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,10 +121,10 @@ object FloatExactNumeric extends FloatIsFractional {
private def overflowException(x: Float, dataType: String) =
throw new ArithmeticException(s"Casting $x to $dataType causes overflow")

private val intUpperBound = Int.MaxValue.toFloat
private val intLowerBound = Int.MinValue.toFloat
private val longUpperBound = Long.MaxValue.toFloat
private val longLowerBound = Long.MinValue.toFloat
private val intUpperBound = Int.MaxValue
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I'm also not clear how this helps - won't it just promote to a float in the comparison below anyway?
Do we want floorDiv, etc, instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Math.floor returns double, so it's promoted to double

Copy link
Member Author

@turboFei turboFei Jan 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As mentioned by cloud-fan, it seems that cast int to float, then cast to double is not same with casting it to double directly.
image

Copy link
Contributor

Choose a reason for hiding this comment

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

it's true

scala> Int.MaxValue.toDouble
res2: Double = 2.147483647E9

scala> Int.MaxValue.toFloat.toDouble
res3: Double = 2.147483648E9

private val intLowerBound = Int.MinValue
private val longUpperBound = Long.MaxValue
private val longLowerBound = Long.MinValue

override def toInt(x: Float): Int = {
// When casting floating values to integral types, Spark uses the method `Numeric.toInt`
Expand Down Expand Up @@ -155,10 +155,10 @@ object DoubleExactNumeric extends DoubleIsFractional {
private def overflowException(x: Double, dataType: String) =
throw new ArithmeticException(s"Casting $x to $dataType causes overflow")

private val intUpperBound = Int.MaxValue.toDouble
private val intLowerBound = Int.MinValue.toDouble
private val longUpperBound = Long.MaxValue.toDouble
private val longLowerBound = Long.MinValue.toDouble
private val intUpperBound = Int.MaxValue
private val intLowerBound = Int.MinValue
private val longUpperBound = Long.MaxValue
private val longLowerBound = Long.MinValue

override def toInt(x: Double): Int = {
if (Math.floor(x) <= intUpperBound && Math.ceil(x) >= intLowerBound) {
Expand Down
2 changes: 1 addition & 1 deletion sql/core/src/test/resources/sql-tests/inputs/postgreSQL/float4.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ SELECT smallint(float('32767.6'));
SELECT smallint(float('-32768.4'));
SELECT smallint(float('-32768.6'));
SELECT int(float('2147483520'));
SELECT int(float('2147483647'));
Copy link
Contributor

Choose a reason for hiding this comment

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

These tests are copied from pgsql and we shouldn't change it. We just need to re-generate the answer file and keep the actual result as it is.

SELECT int(float('2147483392'));
Copy link
Contributor

Choose a reason for hiding this comment

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

let's NOT change the pgsql tests. They are used to verify the difference between Spark and pgsql. We should respect the test result, whatever it is.

SELECT int(float('-2147483648.5'));
SELECT int(float('-2147483900'));
SELECT bigint(float('9223369837831520256'));
Expand Down
6 changes: 3 additions & 3 deletions sql/core/src/test/resources/sql-tests/results/postgreSQL/float4.sql.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -320,11 +320,11 @@ struct<CAST(CAST(2147483520 AS FLOAT) AS INT):int>


-- !query
SELECT int(float('2147483647'))
SELECT int(float('2147483392'))
-- !query schema
struct<CAST(CAST(2147483647 AS FLOAT) AS INT):int>
struct<CAST(CAST(2147483392 AS FLOAT) AS INT):int>
-- !query output
2147483647
2147483392


-- !query
Expand Down
11 changes: 11 additions & 0 deletions sql/core/src/test/scala/org/apache/spark/sql/SQLQuerySuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3383,6 +3383,17 @@ class SQLQuerySuite extends QueryTest with SharedSparkSession with AdaptiveSpark
checkAnswer(df, Row(1))
}
}

test("SPARK-26218: Fix the corner case when casting float to Integer") {
withSQLConf(SQLConf.ANSI_ENABLED.key -> "true") {
intercept[ArithmeticException](
sql("SELECT CAST(CAST(2147483648 as FLOAT) as Integer)").collect()
)
intercept[ArithmeticException](
sql("SELECT CAST(CAST(2147483648 as DOUBLE) as Integer)").collect()
)
}
}
}

case class Foo(bar: Option[String])