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-20876][SQL]If the input parameter is float type for ceil or floor,the result is not we expected #18103

Closed
wants to merge 1 commit into from
Closed

[SPARK-20876][SQL]If the input parameter is float type for ceil or floor,the result is not we expected #18103

wants to merge 1 commit into from

Conversation

10110346
Copy link
Contributor

@10110346 10110346 commented May 25, 2017 •
edited
Loading

What changes were proposed in this pull request?

spark-sql>SELECT ceil(cast(12345.1233 as float));
spark-sql>12345
For this case, the result we expected is 12346
spark-sql>SELECT floor(cast(-12345.1233 as float));
spark-sql>-12345
For this case, the result we expected is -12346

Because in Ceil or Floor, inputTypes has no FloatType, so it is converted to LongType.

How was this patch tested?

After the modification:
spark-sql>SELECT ceil(cast(12345.1233 as float));
spark-sql>12346
spark-sql>SELECT floor(cast(-12345.1233 as float));
spark-sql>-12346

@10110346 10110346 force-pushed the wip-lx-0525-1 branch 4 times, most recently from 953d207 to c9331e3 Compare May 25, 2017 03:38
@10110346 10110346 changed the title [SPARK-20876][SQL]if the input parameter is float type for ceil ,the result is not we expected [SPARK-20876][SQL]if the input parameter is float type for ceil or floor,the result is not we expected May 25, 2017
@sameeragarwal
Copy link
Member

ok to test

1 similar comment
@gatorsmile
Copy link
Member

ok to test

@10110346 10110346 changed the title [SPARK-20876][SQL]if the input parameter is float type for ceil or floor,the result is not we expected [SPARK-20876][SQL]If the input parameter is float type for ceil or floor,the result is not we expected May 25, 2017
@heary-cao heary-cao mentioned this pull request May 25, 2017
Closed
@SparkQA
Copy link

SparkQA commented May 25, 2017

Test build #77345 has finished for PR 18103 at commit 37f0ff3.

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

gatorsmile
gatorsmile reviewed May 27, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala
case DoubleType => f(input.asInstanceOf[Double]).toLong
case DecimalType.Fixed(precision, scale) => input.asInstanceOf[Decimal].floor
case DecimalType.Fixed(_, _) => input.asInstanceOf[Decimal].floor
}

override def doGenCode(ctx: CodegenContext, ev: ExprCode): ExprCode = {
Copy link
Member

Choose a reason for hiding this comment

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

You also need to fix this code path.

gatorsmile
gatorsmile reviewed May 27, 2017
View reviewed changes
...catalyst/src/test/scala/org/apache/spark/sql/catalyst/expressions/MathExpressionsSuite.scala Outdated

val doublePi: Double = 3.1415
val floatPi: Float = 3.1415f
val longPi: Long = 31415926535897912L
Copy link
Member

Choose a reason for hiding this comment

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

Moving the related test cases from operators.sql here. You can hit the bug I mentioned above.

Copy link
Contributor Author

@10110346 10110346 May 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

yes, actually, i have done it as #18082

Copy link
Contributor Author

@10110346 10110346 May 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@gatorsmile I closed the PR #18082 temporarily

gatorsmile
gatorsmile reviewed May 27, 2017
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/mathExpressions.scala Outdated
@@ -232,12 +232,13 @@ case class Ceil(child: Expression) extends UnaryMathExpression(math.ceil, "CEIL"
}

override def inputTypes: Seq[AbstractDataType] =
Seq(TypeCollection(LongType, DoubleType, DecimalType))
Seq(TypeCollection(DoubleType, DecimalType, LongType, FloatType))
Copy link
Member

@gatorsmile gatorsmile May 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

No need to add FloatType, I think. Type coersion will promote it to DoubleType.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it will promote it to DoubleType, but must move LongType to end

@SparkQA
Copy link

SparkQA commented May 27, 2017

Test build #77444 has finished for PR 18103 at commit 4cdb44e.

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

@gatorsmile
Copy link
Member

Could you remove the sql statements from opterators.sql after you moving them to mathExpressionSuite?

@gatorsmile
Copy link
Member

LGTM except one comment.

@heary-cao heary-cao mentioned this pull request May 27, 2017
Closed
@SparkQA
Copy link

SparkQA commented May 27, 2017

Test build #77450 has started for PR 18103 at commit 660c038.

@SparkQA
Copy link

SparkQA commented May 27, 2017

Test build #77458 has finished for PR 18103 at commit 93baa57.

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

@gatorsmile
Copy link
Member

Thanks! Merging to master.

@gatorsmile
Copy link
Member

Could you open another PR to backport the fix to 2.2? Thanks!

@asfgit asfgit closed this in 3969a80 May 27, 2017
@10110346 10110346 mentioned this pull request May 31, 2017
Closed
asfgit pushed a commit that referenced this pull request May 31, 2017
@10110346 @gatorsmile
[SPARK-20876][SQL][BACKPORT-2.2] If the input parameter is float type…
f59f9a3 
… for ceil or floor,the result is not we expected

## What changes were proposed in this pull request?

This PR is to backport #18103 to Spark 2.2

## How was this patch tested?
unit test

Author: liuxian <liu.xian3@zte.com.cn>

Closes #18155 from 10110346/wip-lx-0531.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gatorsmile gatorsmile gatorsmile left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@10110346 @sameeragarwal @gatorsmile @SparkQA