Skip to content

Improved support for CQL Decimal - #376

Open
dehall wants to merge 19 commits into
masterfrom
bigdecimal
Open

Improved support for CQL Decimal#376
dehall wants to merge 19 commits into
masterfrom
bigdecimal

Conversation

@dehall

@dehall dehall commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

This PR migrates Decimals from being represented by plain JS number to being represented by a Decimal class. (CQL Integers remain represented by plain JS numbers.) Our new Decimal class is a wrapper around the decimal.js library. All interactions with the library are limited to one file so if we decide that's the wrong library, it should be straightforward to change.

Where possible, I've tried to make the changes developer-friendly, for instance, because Quantity.value is always a Decimal, the Quantity constructor accepts anything that can be converted to a Decimal, eg, a number, bigint, or string. This is primarily relevant to the unit tests where we have a lot of "result should equal(new Quantity(3, 'g')" style test expectations.

This change means that now all CQL types are represented 1:1 by their respective JS types, so passing a "type" argument around became unnecessary in several places.

Decimals need to be normalized to a max of 8 digits after the decimal point, and have a maximum and minimum value, and so my philosophy was to try to normalize and bounds check as few times as possible, and hence as late as possible: only in the ELM layer. There are some remaining instances of checking for overflow in the datatype layer that I could have removed, but that would require even more refactoring so I left them for now.

Per the spec, the string representation of a Decimal must always contain at least one digit on either side of the decimal point. (eg, 1.0 not 1 or 1., and 0.1 not .1, and not exponential notation like 1e8)

Changes here mostly fall into 3 categories:

  • Arithmetic operator support
  • Aggregate operator support
  • Interval expansion
    • Now instead of separate logic for number/bigint/Decimal, interval expansion is always performed with Decimals and converted back to the appropriate type when each "sub-interval" is constructed
    • While looking at interval expansion, I saw some skipped spec tests that could be fixed by adding just a few lines, so true support for the "single interval" expand overload is now present

Note this PR does not implement the CQL Precision operator, and the Decimal class doesn't keep track of significant figures. (JS numbers didn't either, so this isn't a regression) This means that trailing zeros after the decimal point will not be preserved. eg:

Decimal.from("1.00000000").toString()
-->
"1.0"

decimal.js doesn't support this natively, so a future effort will have to add a second internal state field to track scale.

The two unit test failures are expected at this point (I removed the part of the expand Interval logic that covers those 2 specific tests) but I'm waiting for more direction on #cql > Interval Expand example before doing anything more on that front.

Notable Boundaries

There are a couple instances where interactions with plain JS numbers are forced:

  • Quantity operations that interact with the UCUM unit conversion library
    • To try to reduce the chance of loss of precision, instead of calling the library with "n of unit A to unit B", I call the library with "1 of unit A to unit B" and then multiplying the input Decimal by the resulting factor.
  • DateTime operations that interact with the library luxon, specifically the timezoneOffset field
    • I think these always should be integers or rational numbers in a small range, so I'm pretty sure every possible value can be exactly represented with number anyway

Also note that the Decimal constructor accepts JS numbers that do not need to represent integers, but there is the risk of loss of precision if the literal used cannot be represented precisely as a js number. To be safe, consumers of this library constructing a Decimal instance should generally use the string constructor which guarantees round-trip safety. Eg:

Decimal.from(1.0000000000000000000000000001).toString()
--> "1"
 
Decimal.from("1.0000000000000000000000000001").toString()
--> "1.0000000000000000000000000001"

Decimal.from(900719925474099230945).toString()
--> "900719925474099200000" 

Decimal.from("900719925474099230945").toString()
--> "900719925474099230945"

Pull requests into cql-execution require the following.
Submitter and reviewer should ✔ when done.
For items that are not-applicable, mark "N/A" and ✔.

Submitter:

  • This pull request describes why these changes were made
  • Code diff has been done and been reviewed (it does not contain: additional white space, not applicable code changes, debug statements, etc.)
  • Tests are included and test edge cases
  • Tests have been run locally and pass
  • Code coverage has not gone down and all code touched or added is covered.
  • Code passes lint and prettier (hint: use npm run check to run tests, lint, and prettier)
  • All dependent libraries are appropriately updated or have a corresponding PR related to this change
    Reviewer:

Name:

  • Code is maintainable and reusable, reuses existing code and infrastructure where appropriate, and accomplishes the task’s purpose
  • The tests appropriately test the new code, including edge cases
  • You have tried to break the code

@codecov-commenter

codecov-commenter commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 91.01562% with 46 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.14%. Comparing base (81b47fc) to head (f913c70).

Files with missing lines Patch % Lines
src/elm/arithmetic.ts 88.17% 6 Missing and 5 partials ⚠️
src/elm/type.ts 68.57% 8 Missing and 3 partials ⚠️
src/elm/interval.ts 87.32% 3 Missing and 6 partials ⚠️
src/util/math.ts 93.54% 3 Missing and 3 partials ⚠️
src/util/immutableUtil.ts 50.00% 2 Missing and 1 partial ⚠️
src/datatypes/interval.ts 87.50% 0 Missing and 2 partials ⚠️
src/elm/aggregate.ts 97.01% 1 Missing and 1 partial ⚠️
src/datatypes/decimal.ts 98.43% 0 Missing and 1 partial ⚠️
src/datatypes/quantity.ts 95.23% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #376      +/-   ##
==========================================
+ Coverage   88.70%   89.14%   +0.43%     
==========================================
  Files          59       60       +1     
  Lines        4933     5104     +171     
  Branches     1429     1469      +40     
==========================================
+ Hits         4376     4550     +174     
+ Misses        322      318       -4     
- Partials      235      236       +1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@dehall dehall changed the title WIP: Improved support for CQL Decimal Improved support for CQL Decimal Aug 27, 2026
@dehall
dehall marked this pull request as ready for review August 27, 2026 15:47
@dehall
dehall requested a review from cmoesel August 27, 2026 15:47
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.

2 participants