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.

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

fix: remove leading zeros when casting decimal to string #5270

Merged
merged 3 commits into from
May 14, 2020

Conversation

spena
Copy link
Member

@spena spena commented May 5, 2020

Description

What behavior do you want to change, why, how does your patch achieve the changes?

Fixes #4862

Testing done

Describe the testing strategy. Unit and integration tests are expected for any behavior changes.
Added unit tests
Verified manually

ksql> CREATE STREAM rewards(user VARCHAR, reward DECIMAL(100, 0)) WITH(KAFKA_TOPIC='events', VALUE_FORMAT='JSON', PARTITIONS=1, REPLICAS=1);

ksql> insert into rewards(user, reward) values ('u1', 281474976710656);

ksql> select user, CAST(SUM(reward) AS STRING) AS VALUE from rewards GROUP BY user emit changes;
+---------------------------------------------------------------------------------+---------------------------------------------------------------------------------+
|USER                                                                             |VALUE                                                                            |
+---------------------------------------------------------------------------------+---------------------------------------------------------------------------------+
|u1                                                                               |281,474,976,710,656                                                              |

Reviewer checklist

  • Ensure docs are updated if necessary. (eg. if a user visible feature is being added or changed).
  • Ensure relevant issues are linked (description should include text like "Fixes #")

@spena spena added this to the 0.10.0 milestone May 5, 2020
@spena spena requested review from agavra and a team May 5, 2020 16:31
Copy link
Contributor

@agavra agavra left a comment

Choose a reason for hiding this comment

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

LGTM!

Comment on lines 126 to 128
if (precision < value.precision()) {
format.setMinimumIntegerDigits(precision - scale);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

why do we need this at all? does decimal format not handle integer digits by default?

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure why this was needed before, but I know now this is used to add the zeros at the left. I removed it now.

@spena spena force-pushed the remove_decimal_leading_zeros branch 5 times, most recently from ce0a8eb to accec1e Compare May 13, 2020 23:13
@spena spena force-pushed the remove_decimal_leading_zeros branch from accec1e to b6cb685 Compare May 14, 2020 02:13
@spena
Copy link
Member Author

spena commented May 14, 2020

The test failing is not related to this patch. The previous test passed, and now this one failed. There might a be a problem in master with other code.

@spena spena merged commit e1cc8ad into confluentinc:master May 14, 2020
@spena spena deleted the remove_decimal_leading_zeros branch May 14, 2020 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Casting a Decimal to string looks weird.
2 participants