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

Add truncate(x,n) Presto function #2892

Closed
wants to merge 1 commit into from
Closed

Add truncate(x,n) Presto function #2892

wants to merge 1 commit into from

Conversation

gosharz
Copy link
Contributor

@gosharz gosharz commented Oct 19, 2022

Summary: Adding truncate(x,n) Presto function

Differential Revision: D40516665

@netlify
Copy link

netlify bot commented Oct 19, 2022 •
edited
Loading

✅ Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit dc8cfcf
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/63850f597de9b60008972593

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported labels Oct 19, 2022
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D40516665

@gosharz gosharz mentioned this pull request Oct 20, 2022
Closed
pedroerp
pedroerp reviewed Oct 21, 2022
View reviewed changes
velox/docs/functions/math.rst Outdated
@@ -134,6 +134,10 @@ Mathematical Functions

Returns x rounded to integer by dropping digits after decimal point.

.. function:: truncate(x,n) -> double
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: white-space between the arguments

velox/functions/prestosql/Arithmetic.h Outdated
@@ -487,8 +487,8 @@ struct EulerConstantFunction {
template <typename T>
struct TruncateFunction {
template <typename TInput>
FOLLY_ALWAYS_INLINE void call(TInput& result, TInput a) {
result = std::trunc(a);
FOLLY_ALWAYS_INLINE void call(TInput& result, TInput a, int32_t n = 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we should provide two overloads for these functions for performance, so we can remove the branch from the common case (which is probably a single parameter).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pedroerp Will that imply adding the overload to TruncateFunction class and adding a separate overload registration(unary) in the registrar?

velox/functions/prestosql/ArithmeticImpl.h Outdated
if (std::isnan(number) || std::isinf(number) || decimals == 0) {
return std::trunc(number);
}
const auto factor = std::pow(10, decimals);
Copy link
Contributor

Choose a reason for hiding this comment

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

is this how this is implemented in the java codebase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not exactly. The java implementation relies on BigDecimal(setScale, valueOf etc.) and not very simple logic incapsulated in it. To my research we don't possess similar functionality in c++. Unless we want to implemet/grab BigDecimal like utilities from somewhere this seems to be the quickest way of having the function available(though with the risk of mismatches in some cases).

Side question, do we have a A/B engine tests setup to test between presto and velox operators?

Copy link
Contributor

Choose a reason for hiding this comment

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

We support Decimal types in Velox (both Big and Small). The functionality should be similar but the API might not be exactly the same.

Side question, do we have a A/B engine tests setup to test between presto and velox operators?

There's a test suite in Prestissimo that validates that C++ and Java versions return the same results, but the queries need to be manually written there. We should make sure we add something covering truncate behavior as well.

Copy link
Contributor Author

@gosharz gosharz Oct 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

So we are interested in mimicking this line https://github.com/prestodb/presto/blob/ba9ec306d453f968f711e437ab2d60b91dc6b700/presto-main/src/main/java/com/facebook/presto/operator/scalar/MathFunctions.java#L380. Specifically:

  1. valueOf() - https://github.com/openjdk-mirror/jdk7u-jdk/blob/f4d80957e89a19a29bb9f9807d2a28351ed7f7df/src/share/classes/java/math/BigDecimal.java#L806
  2. setScale() - https://github.com/openjdk-mirror/jdk7u-jdk/blob/f4d80957e89a19a29bb9f9807d2a28351ed7f7df/src/share/classes/java/math/BigDecimal.java#L2378
  3. doubleValue() - https://github.com/openjdk-mirror/jdk7u-jdk/blob/f4d80957e89a19a29bb9f9807d2a28351ed7f7df/src/share/classes/java/math/BigDecimal.java#L3132
  4. And toString as an implementation detail

We have:

  1. UnscaledLongDecimal -

    velox/velox/type/UnscaledLongDecimal.h

    Line 55 in a19a652

    struct UnscaledLongDecimal {
  2. UnscaledShortDecimal -

    velox/velox/type/UnscaledShortDecimal.h

    Line 26 in a19a652

    struct UnscaledShortDecimal {
  3. DecimalUtils with rescale and toString -

    velox/velox/type/DecimalUtil.h

    Line 28 in a19a652

    class DecimalUtil {
  4. DECIMAL type -

    velox/velox/type/Type.h

    Line 1105 in a19a652

    TypePtr DECIMAL(uint8_t precision, uint8_t scale);

Looking into how to leverage those to mimick Java behavior.

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D40516665

gosharz added a commit to gosharz/velox that referenced this pull request Oct 24, 2022
@gosharz @facebook-github-bot
Add truncate(x,n) Presto function (facebookincubator#2892)
4a1fa08 
Summary:
Pull Request resolved: facebookincubator#2892

Adding truncate(x,n) Presto function

Differential Revision: D40516665

fbshipit-source-id: 75ac1ca44dfc354c9c809a082bfd3352d9c729dd
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D40516665

gosharz pushed a commit to gosharz/velox that referenced this pull request Nov 3, 2022
@facebook-github-bot
Add truncate(x,n) Presto function (facebookincubator#2892)
831af7b 
Summary:
Pull Request resolved: facebookincubator#2892

Adding truncate(x,n) Presto function

Differential Revision: D40516665

fbshipit-source-id: e076863863fb8b54d6960eb479d7f1447c708486
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D40516665

gosharz pushed a commit to gosharz/velox that referenced this pull request Nov 28, 2022
@facebook-github-bot
Add truncate(x,n) Presto function (facebookincubator#2892)
b0f16b1 
Summary:
Pull Request resolved: facebookincubator#2892

Adding truncate(x,n) Presto function

Differential Revision: D40516665

fbshipit-source-id: 1d3eeb2587f32a437cd093351d444911a8e5fcd3
gosharz pushed a commit to gosharz/velox that referenced this pull request Nov 28, 2022
@facebook-github-bot
Add truncate(x,n) Presto function (facebookincubator#2892)
c1e9493 
Summary:
Pull Request resolved: facebookincubator#2892

Adding truncate(x,n) Presto function

Differential Revision: D40516665

fbshipit-source-id: 295451a2a6de62bb6f4c126a326db564f8947066
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D40516665

gosharz pushed a commit to gosharz/velox that referenced this pull request Nov 28, 2022
@facebook-github-bot
Add truncate(x,n) Presto function (facebookincubator#2892)
7869c54 
Summary:
Pull Request resolved: facebookincubator#2892

Adding truncate(x,n) Presto function

Differential Revision: D40516665

fbshipit-source-id: 50fecc52801024b6b4c001c818b492aaa776b39a
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D40516665

@facebook-github-bot
Add truncate(x,n) Presto function (facebookincubator#2892)
dc8cfcf 
Summary:
Pull Request resolved: facebookincubator#2892

Adding truncate(x,n) Presto function

Differential Revision: D40516665

fbshipit-source-id: 49cd2c9dad87c63a940530c41aa192a2d3440f37
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D40516665

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pedroerp pedroerp pedroerp left review comments

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. fb-exported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@gosharz @facebook-github-bot @pedroerp