Skip to content

Fix long overflow in CosineSimilarity dot product - #779

Draft
PHJ2000 wants to merge 1 commit into
apache:masterfrom
PHJ2000:fix-cosine-similarity-overflow
Draft

PHJ2000 wants to merge 1 commit into
apache:masterfrom
PHJ2000:fix-cosine-similarity-overflow

Conversation

@PHJ2000

@PHJ2000 PHJ2000 commented Oct 4, 2026 •

Copy link
Copy Markdown

Comparing identical vectors containing three Integer.MAX_VALUE components returns -0.33333333457509673 instead of 1.0. Each individual product fits in a long, but their sum overflows the long accumulator in CosineSimilarity.dot().

Accumulate the dot product in double, as the squared norms already do. Keep the long multiplication so each integer product is computed without integer overflow before accumulation. Add a parameterized regression test for identical vectors with components 1, Integer.MAX_VALUE, and Integer.MIN_VALUE.

Validation:

  • On unmodified production code, mvn -Dtest=CosineSimilarityTest test ran 7 tests and failed the two large-value cases.
  • The published 1.15.0 JAR also reproduces the positive-vector failure.
  • After the change, the default mvn goal passes, including the full test suite, Apache RAT, japicmp, Checkstyle, PMD, SpotBugs, and Javadoc generation (OpenJDK 21.0.12.1, Maven 3.9.11).

Jira: pending an Apache Jira account. This PR remains a draft until the report can be filed and its issue key linked here and in the commit message.

Checklist from the repository template:

  • Read the contribution guidelines.
  • Read the ASF Generative Tooling Guidance.
  • Used AI: OpenAI Codex investigated the bug, generated the implementation change and regression test, ran validation, and drafted the issue/PR text.
  • Run a successful build using the default Maven goal (mvn).
  • Added regression tests that fail without the production change.
  • Explained what changes, how, and why.
  • Used a meaningful commit subject and body.

Accumulate products in double so identical vectors with large integer
components retain a cosine similarity of one. Individual products still
use long arithmetic before being added to the accumulator.

Add regression coverage for identical vectors with ordinary, maximum,
and minimum integer components. The two large-component cases fail
without the accumulator change.

Generated-by: OpenAI Codex
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.

1 participant