Skip to content

[SPARK-8206][SQL][WIP]Add function round #6836

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
Closed
wants to merge 4 commits into from
Closed

[SPARK-8206][SQL][WIP]Add function round #6836

wants to merge 4 commits into from

Conversation

@zhichao-li
Copy link
Contributor

@zhichao-li zhichao-li commented Jun 16, 2015

No description provided.

@zhichao-li
Copy link
Contributor Author

zhichao-li commented Jun 16, 2015

cc @chenghao-intel

@SparkQA
Copy link

SparkQA commented Jun 16, 2015

Test build #34972 has finished for PR 6836 at commit 4069bba.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Round(left: Expression, right: Expression)

chenghao-intel
chenghao-intel reviewed Jun 16, 2015
View reviewed changes
sql/core/src/main/scala/org/apache/spark/sql/functions.scala Outdated
Copy link
Contributor

@chenghao-intel chenghao-intel Jun 16, 2015

Choose a reason for hiding this comment

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

This probably not necessary, as both of the parameters are literals(constant value).

@markhamstra
Copy link
Contributor

markhamstra commented Jun 16, 2015

I don't believe this is quite right, since setScale in BigDecimal will handle negative values differently than SQL ROUND(column_name, decimals) will handle a negative value of decimals -- every SQL engine that I'm familiar with will treat negative decimals as if it were 0.

@chenghao-intel
Copy link
Contributor

chenghao-intel commented Jun 16, 2015

Still some other issues need to be addressed:

  • From the GenericUDFRound of Hive, seems the scale is a constant value, we need to it in the same way.
  • The output dataType can be either decimal or double.

@markhamstra
Copy link
Contributor

markhamstra commented Jun 16, 2015

Actually, it's worth considering whether in the common case of rounding to zero decimal places we should be producing integral values.

@zhichao-li zhichao-li changed the title [SPARK-8206][SQL]Add function round [WIP] [SPARK-8206][SQL][WIP]Add function round Jun 17, 2015
@zhichao-li
Copy link
Contributor Author

zhichao-li commented Jun 17, 2015

@markhamstra @chenghao-intel For hive it would produce double for such case I guess we should keep the same ?

@chenghao-intel
Copy link
Contributor

chenghao-intel commented Jun 17, 2015

We'd better to keep the same behavior as Hive does, otherwise it will causes inconsistencies for existed code.

@SparkQA
Copy link

SparkQA commented Jun 19, 2015

Test build #35254 has finished for PR 6836 at commit 2d5d402.

  • This patch fails Scala style tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • case class Round(left: Expression, right: Expression)

@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35500 has finished for PR 6836 at commit cd30012.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • case class Round(left: Expression, right: Expression)

@yjshen
Copy link
Member

yjshen commented Jun 23, 2015

Ah.... I didn't notice there are two JIRA for round, #6938 is my working version, behave as what Hive's GenericUDFRound does.

@chenghao-intel chenghao-intel mentioned this pull request Jun 23, 2015
Closed
@yjshen
Copy link
Member

yjshen commented Jun 23, 2015

How about just act as Hive's GenericUDFRound (#6938 takes the same logic to behave like Hive's counterparts) ? If so, I think we can merge these two.

@chenghao-intel
Copy link
Contributor

chenghao-intel commented Jun 23, 2015

can you rebase this please?

@SparkQA
Copy link

SparkQA commented Jun 23, 2015

Test build #35547 has finished for PR 6836 at commit 87164a1.

  • This patch passes all tests.
  • This patch does not merge cleanly.
  • This patch adds the following public classes (experimental):
    • case class Round(left: Expression, right: Expression)

yjshen
yjshen reviewed Jun 23, 2015
View reviewed changes
sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/math.scala Outdated
Copy link
Member

@yjshen yjshen Jun 23, 2015

Choose a reason for hiding this comment

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

GenericUDFRound would return null if Float.isNaN(f) or Float.isInfinite(f)

Copy link
Contributor Author

@zhichao-li zhichao-li Jun 24, 2015

Choose a reason for hiding this comment

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

Good catch, would add that same as hive.

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35613 has finished for PR 6836 at commit 4e66e8d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Round(left: Expression, right: Expression)

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35626 has finished for PR 6836 at commit 918376d.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Round(left: Expression, right: Expression)

@SparkQA
Copy link

SparkQA commented Jun 24, 2015

Test build #35649 has finished for PR 6836 at commit 2a6dc13.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds the following public classes (experimental):
    • case class Round(valueExpr: Expression, scaleExpr: Expression)

@asfgit asfgit closed this in fc3bd96 Jul 28, 2015
feynmanliang pushed a commit to feynmanliang/spark that referenced this pull request Jul 28, 2015
Merge remote-tracking branch 'apache/master' into SPARK-8856-SparkPla…
8b9a36c 
…n-instrumentation

* apache/master: (113 commits)
  [SPARK-8196][SQL] Fix null handling & documentation for next_day.
  [SPARK-9373][SQL] follow up for StructType support in Tungsten projection.
  [SPARK-9402][SQL] Remove CodegenFallback from Abs / FormatNumber.
  [SPARK-8919] [DOCUMENTATION, MLLIB] Added @SInCE tags to mllib.recommendation
  [EC2] Cosmetic fix for usage of spark-ec2 --ebs-vol-num option
  [SPARK-9394][SQL] Handle parentheses in CodeFormatter.
  Closes apache#6836 since Round has already been implemented.
  [SPARK-9335] [STREAMING] [TESTS] Make sure the test stream is deleted in KinesisBackedBlockRDDSuite
  [MINOR] [SQL] Support mutable expression unit test with codegen projection
  [SPARK-9373][SQL] Support StructType in Tungsten projection
  [SPARK-8828] [SQL] Revert SPARK-5680
  Fixed a test failure.
  [SPARK-9395][SQL] Create a SpecializedGetters interface to track all the specialized getters.
  [SPARK-8195] [SPARK-8196] [SQL] udf next_day last_day
  [SPARK-8882] [STREAMING] Add a new Receiver scheduling mechanism
  [SPARK-9386] [SQL] Feature flag for metastore partition pruning
  [SPARK-9230] [ML] Support StringType features in RFormula
  [SPARK-9385] [PYSPARK] Enable PEP8 but disable installing pylint.
  [SPARK-4352] [YARN] [WIP] Incorporate locality preferences in dynamic allocation requests
  [SPARK-9385] [HOT-FIX] [PYSPARK] Comment out Python style check
  ...
@zhichao-li zhichao-li deleted the round branch September 9, 2015 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@zhichao-li @SparkQA @markhamstra @chenghao-intel @yjshen