This is an automated email from the ASF dual-hosted git repository.
chaokunyang pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/fory.git
The following commit(s) were added to refs/heads/main by this push:
new 792ab5053 fix(js): avoid precision loss in negative varint64 fast path
(#3992)
792ab5053 is described below
commit 792ab50530cb35c60668efbe99f14e11a7f5d568
Author: Ayush Kumar <[email protected]>
AuthorDate: Sat Aug 29 22:34:05 2026 +0530
fix(js): avoid precision loss in negative varint64 fast path (#3992)
## Why?
zigzag is computed in double precision, so any negative value with
magnitude above 2^52 rounds to an even zigzag and decodes as positive:
-4503599627370497n round-trips as +4503599627370496n. Also reachable
through writeSliInt64.
## What does this PR do?
the writeVarInt64 fast path now requires v >= -(2^52) in both the number
and safe-range bigint branches; anything more negative falls through to
the exact bigint zigzag (v << 1n) ^ (v >> 63n). writeSliInt64 is fixed
too since its big-long path delegates here. A short comment on the new
constant records the invariant.
## Related issues
## AI Contribution Checklist
- [ ] Substantial AI assistance was used in this PR: `yes` / `no`
- [ ] If `yes`, I included a completed [AI Contribution
Checklist](https://github.com/apache/fory/blob/main/AI_POLICY.md#9-contributor-checklist-for-ai-assisted-prs)
in this PR description and the required `AI Usage Disclosure`.
- [ ] If `yes`, my PR description includes the required `ai_review`
summary and screenshot evidence or equivalent persisted links of the
final clean AI review results from both fresh reviewers described in
`AI_POLICY.md`, the Fory-guided reviewer and the independent general
reviewer, on the current PR diff or current HEAD after the latest code
changes.
## Does this PR introduce any user-facing change?
- [ ] Does this PR introduce any public API change?
- [ ] Does this PR introduce any binary protocol compatibility change?
## Benchmark
---
javascript/packages/core/lib/writer/index.ts | 10 ++++++---
javascript/test/writer.test.ts | 31 ++++++++++++++++++++++++++++
2 files changed, 38 insertions(+), 3 deletions(-)
diff --git a/javascript/packages/core/lib/writer/index.ts
b/javascript/packages/core/lib/writer/index.ts
index f82cad30f..f08f6bb5e 100644
--- a/javascript/packages/core/lib/writer/index.ts
+++ b/javascript/packages/core/lib/writer/index.ts
@@ -25,7 +25,11 @@ import { toBFloat16Bits } from "../types/bfloat16";
const MAX_POOL_SIZE = 1024 * 1024 * 3; // 3MB
const MAX_SAFE_BIGINT = BigInt(Number.MAX_SAFE_INTEGER);
-const MIN_SAFE_BIGINT = BigInt(Number.MIN_SAFE_INTEGER);
+// Zigzag in doubles needs -v * 2 - 1 to stay exact. Below -(2^52) that value
is
+// an odd integer above 2^53, which rounds to an even neighbor and flips the
+// decoded sign, so those values must take the bigint zigzag path.
+const MIN_VAR_INT64_NUMBER = -4503599627370496; // -(2^52)
+const MIN_VAR_INT64_BIGINT = BigInt(MIN_VAR_INT64_NUMBER);
function getInternalStringDetector() {
try {
@@ -455,12 +459,12 @@ export class BinaryWriter {
writeVarInt64(v: bigint | number) {
if (typeof v !== "bigint") {
- if (Number.isSafeInteger(v)) {
+ if (Number.isSafeInteger(v) && v >= MIN_VAR_INT64_NUMBER) {
this.writeVarUInt64Number(v >= 0 ? v * 2 : -v * 2 - 1);
return;
}
v = BigInt(v);
- } else if (v >= MIN_SAFE_BIGINT && v <= MAX_SAFE_BIGINT) {
+ } else if (v >= MIN_VAR_INT64_BIGINT && v <= MAX_SAFE_BIGINT) {
const value = Number(v);
this.writeVarUInt64Number(value >= 0 ? value * 2 : -value * 2 - 1);
return;
diff --git a/javascript/test/writer.test.ts b/javascript/test/writer.test.ts
index 6c079535a..164860bbf 100644
--- a/javascript/test/writer.test.ts
+++ b/javascript/test/writer.test.ts
@@ -18,6 +18,7 @@
*/
import { OwnershipError } from "../packages/core/lib/error";
+import { BinaryReader } from "../packages/core/lib/reader";
import { BinaryWriter } from "../packages/core/lib/writer";
import { describe, expect, test } from "@jest/globals";
@@ -63,4 +64,34 @@ describe("writer", () => {
}
throw new Error("unreachable code");
});
+
+ test("varint64 round-trips negative values beyond 2^52", () => {
+ // The number fast path computes zigzag as -v * 2 - 1 in doubles; below
+ // -(2^52) that intermediate rounds to an even neighbor and flips the
+ // decoded sign, so such values must use the exact bigint zigzag path.
+ const values: (bigint | number)[] = [
+ -(2n ** 52n),
+ -(2n ** 52n + 1n),
+ -(2n ** 53n - 1n),
+ -4503599627370497,
+ 2n ** 53n - 1n,
+ -1n,
+ 0,
+ ];
+ for (const value of values) {
+ const writer = new BinaryWriter({});
+ writer.writeVarInt64(value);
+ const reader = new BinaryReader({});
+ reader.reset(writer.dump());
+ expect(reader.readVarInt64()).toBe(BigInt(value));
+ }
+ });
+
+ test("sliInt64 round-trips negative values beyond 2^52", () => {
+ const writer = new BinaryWriter({});
+ writer.writeSliInt64(-(2n ** 52n + 1n));
+ const reader = new BinaryReader({});
+ reader.reset(writer.dump());
+ expect(reader.readSliInt64()).toBe(-(2n ** 52n + 1n));
+ });
});
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
