Skip to content

fix: Improve ergonomics for the Decimal type #341

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
Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

fix: Improve ergonomics for the Decimal type #341

wants to merge 4 commits into from

Conversation

@Divyanshu-s13
Copy link
Contributor

@Divyanshu-s13 Divyanshu-s13 commented Nov 18, 2025 •
edited by kou
Loading

What's Changed

Added a new decimal.ts module that provides high-level utilities for working with Decimal128 and Decimal256 values in JavaScript. These utilities make decimal handling more ergonomic by removing the need for low-level bit manipulation and aligning behavior with Arrow C++ semantics. The module is exported through the util namespace and integrates cleanly without introducing breaking changes.

Closes #81.

@Divyanshu-s13
Copy link
Contributor Author

Divyanshu-s13 commented Nov 18, 2025 •
edited
Loading

@kou Fix #81: Improve ergonomics for the Decimal type in JavaScript bindings.

domoritz
domoritz requested changes Nov 20, 2025
View reviewed changes
Copy link
Member

@domoritz domoritz left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request. This definitely needs tests.

Also, does this address #100 and #77 in some way?

test: add decimal utilities test suite
f95f882 
- Comprehensive tests for isNegativeDecimal, negateDecimal
- Tests for toDecimalString and toDecimalNumber conversions
- Tests for fromDecimalString parsing with roundtrips
- Integration tests for negation and edge cases
@Divyanshu-s13
Copy link
Contributor Author

Divyanshu-s13 commented Nov 20, 2025 •
edited
Loading

Thanks for the pull request. This definitely needs tests.

Also, does this address #100 and #77 in some way?

thanks for review!
I have added a test case for this.

This partially addresses Issues #77 and #100 by providing comprehensive utility functions for working with Decimal types in Apache Arrow JavaScript.

@kou kou changed the title Fix #81: Improve ergonomics for the Decimal type in JavaScript bindings fix: Improve ergonomics for the Decimal type Nov 22, 2025
@domoritz
Copy link
Member

domoritz commented Nov 24, 2025

Thanks for these utilities. I think we should try to use them in the default converters so that #77 gets fixed. Either you could do this here or we can do it in a follow up when you fixed the CI errors and we merged.

@Divyanshu-s13
Copy link
Contributor Author

Divyanshu-s13 commented Nov 24, 2025

Thanks for these utilities. I think we should try to use them in the default converters so that #77 gets fixed. Either you could do this here or we can do it in a follow up when you fixed the CI errors and we merged.

I have fixed CI error can you merge it.

@kou
Copy link
Member

kou commented Nov 25, 2025

Could you check lint failures?

https://github.com/apache/arrow-js/actions/runs/19644239168/job/56300291328?pr=341#step:5:8

/home/runner/work/arrow-js/arrow-js/src/util/decimal.ts
Error:    84:9  error  'n' is never reassigned. Use 'const' instead                 prefer-const
Error:    89:5  error  This `if` statement can be replaced by a ternary expression  unicorn/prefer-ternary
Error:   128:5  error  This `if` statement can be replaced by a ternary expression  unicorn/prefer-ternary

FYI: You can run CI on your fork by enabling GitHub Actions on your fork.

fix: resolve ESLint errors in decimal.ts
a13dcda 
- Change 'let n' to 'const n' (prefer-const)
- Use ternary for magnitude assignment (unicorn/prefer-ternary)
- Use ternary for result composition (unicorn/prefer-ternary)
@Divyanshu-s13
Copy link
Contributor Author

Divyanshu-s13 commented Nov 25, 2025

Could you check lint failures?

https://github.com/apache/arrow-js/actions/runs/19644239168/job/56300291328?pr=341#step:5:8

/home/runner/work/arrow-js/arrow-js/src/util/decimal.ts
Error:    84:9  error  'n' is never reassigned. Use 'const' instead                 prefer-const
Error:    89:5  error  This `if` statement can be replaced by a ternary expression  unicorn/prefer-ternary
Error:   128:5  error  This `if` statement can be replaced by a ternary expression  unicorn/prefer-ternary

FYI: You can run CI on your fork by enabling GitHub Actions on your fork.

I have fix this can you merge it.

@Divyanshu-s13
Copy link
Contributor Author

Divyanshu-s13 commented Nov 25, 2025

@kou @domoritz I have fix all CI failure and all checks are passed please merge it.

domoritz
domoritz reviewed Nov 25, 2025
View reviewed changes
src/util/decimal.ts
}

// Calculate divisor as 10^scale
// Using a loop instead of BigInt exponentiation (**) for ES2015 compatibility
Copy link
Member

@domoritz domoritz Nov 25, 2025

Choose a reason for hiding this comment

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

Should we bump to something beyond es2015 for better code?

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

Reviewers

@domoritz domoritz domoritz requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[JS] Ergonomics for the Decimal type via the JavaScript bindings

3 participants

@Divyanshu-s13 @domoritz @kou