fix: correct Long hyperparameter sampling, Float seed forwarding, form validation, and Jaccard similarity

[Rana Singh (ranadeepsingh)]

Summary

Four defects in core utilities, found while reviewing test coverage in #2498. Each fix ships with a test that fails against current master and passes after the fix — verified by reverting the production changes and re-running.

Split out of #2498 deliberately so that PR stays test-only.

#	Defect	Symptom	Blast radius
1	LongRangeHyperParam overflows its own range	returns values far outside [min, max)	any LongParam tuned via HyperparamBuilder searched garbage
2	FloatRangeHyperParam drops its seed	different seeds → identical sequences	reproducibility silently broken, no symptom
3	UnicodeNormalize.setForm validates the wrong value	invalid form accepted, fails later on executors	opaque, misattributed runtime failure
4	ModelEquality.jaccardSimilarity isn't Jaccard	only ever returns 1.0 or 0.0	currently uncalled — a trap for the next caller

The defects

1. LongRangeHyperParam escapes its own range

val range = max - min
(random.nextLong() * range) + min        // nextLong() spans all 64 bits — this overflows

With min=0, max=100 it produces values like -4494195807357089668. The Int and Double siblings reduce into the range first (nextInt(range), nextDouble() * range); only Long does not.

Fixed by reducing into the range before offsetting, handling three cases:

range == 0   →  min
range  < 0   →  span exceeds Long.MaxValue and can't be represented;
                draw from the full 64-bit range and reject out-of-bounds
                (>half of draws land in bounds, so this terminates fast)
range  > 0   →  boundedNextLong(range) + min

boundedNextLong rejects the partial final block so the result carries no modulo bias — the same technique java.util.Random.nextInt(bound) already uses for Int. A plain % would have skewed toward low values.

Also added require(max >= min). Previously an inverted range returned nonsense instead of failing.

2. FloatRangeHyperParam silently drops its seed

- val doubleRange = new DoubleRangeHyperParam(min.toDouble, max.toDouble)
+ val doubleRange = new DoubleRangeHyperParam(min.toDouble, max.toDouble, seed)

The seed constructor argument was never forwarded, so every instance drew from seed 0. Two Float params configured with different seeds produced identical sequences.

3. UnicodeNormalize.setForm validates the wrong value

def setForm(value: String): this.type = {
  Normalizer.Form.valueOf(getForm)   // ← the value already set, not the incoming one
  set("form", value)
}

getForm falls back to "NFKD", so validation always passed and any string was accepted. setForm("BOGUS") succeeded, then failed inside the UDF on the executors, where the cause is far harder to attribute.

4. ModelEquality.jaccardSimilarity is not a Jaccard index

val a = Set(s1)   // singleton sets of the whole strings
val b = Set(s2)   // ⇒ result is only ever 1.0 or 0.0

A strict equality check wearing a similarity function's name. Now uses character bigrams — matching the working implementation already present in SpeechToTextSDKSuite/SpeechToTextSuite — and defines the both-empty case as 1.0 rather than NaN.

Compatibility

No signature changes; no removed or renamed public members.

LongRangeHyperParam and FloatRangeHyperParam now return different values for a given seed — that is the fix; neither previously returned anything usable. require(max >= min) newly throws on inverted ranges, which no production caller constructs (DefaultHyperparams uses only Int/Double; the only other entry point is user-supplied via py4j).

UnicodeNormalizeSuite supplies a valid form through the existing getterSetterParamExamples hook, because the generic GetterSetterFuzzing fuzzer probes String setters with "foo" and setForm legitimately rejects that now. That hook is the designed extension point, so production validation is not weakened to satisfy the fuzzer.

Validation

• HyperparamRangeSuite (new) + UnicodeNormalizeSuite + PipelineTestCoverageSuite: 18/18 pass
• ModelEqualitySuite: 5/5 pass
• Reverting only the production files: 4 tests fail — confirming they are real regression guards
• Sampling verified empirically against an exact java.util.Random replication: uniform over [min, max) (χ²=7.4, df=6), min attainable, max correctly excluded, overflow branch stays in bounds, no Long.MinValue/math.abs trap
• core/scalastyle, core/Test/scalastyle, cognitive/Test/scalastyle: 0 errors

New suite lands in the automl package, which the pipeline covers by glob, so no pipeline.yaml change is needed.

Follow-up not done here

jaccardSimilarity is now duplicated three ways (here and in two speech suites). Those suites are credential-gated, so de-duplicating them belongs in a separate change.

Checklist

• Tests added and confirmed to fail before / pass after
• No dependency changes
• Not a new feature — no website samples needed

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Rana Singh (ranadeepsingh)]

/azp run

[github-actions]

Hey Rana Singh (@ranadeepsingh) 👋!
Thank you so much for contributing to our repository 🙌.
Someone from SynapseML Team will be reviewing this pull request soon.

We use semantic commit messages to streamline the release process.
Before your pull request can be merged, you should make sure your first commit and PR title start with a semantic prefix.
This helps us to create release messages and credit you for your hard work!

Examples of commit messages with semantic prefixes:

• fix: Fix LightGBM crashes with empty partitions
• feat: Make HTTP on Spark back-offs configurable
• docs: Update Spark Serving usage
• build: Add codecov support
• perf: improve LightGBM memory usage
• refactor: make python code generation rely on classes
• style: Remove nulls from CNTKModel
• test: Add test coverage for CNTKModel

To test your commit locally, please follow our guild on building from source.
Check out the developer guide for additional guidance on testing your change.

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Pull request overview

This PR fixes several correctness issues in SynapseML core utilities and adds targeted regression tests to ensure the corrected behavior is exercised (notably around AutoML hyperparameter sampling, Unicode normalization parameter validation, and string similarity logic in ModelEquality).

Changes:

• Fixes LongRangeHyperParam sampling to avoid overflow-driven out-of-range values; ensures FloatRangeHyperParam forwards its seed correctly.
• Corrects UnicodeNormalize.setForm to validate the incoming form value at set-time (failing fast on invalid forms).
• Re-implements ModelEquality.jaccardSimilarity as a Jaccard index over character bigrams and adds tests for expected similarity behavior.

Show a summary per file

File	Description
core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala	Fixes LongRangeHyperParam overflow behavior and forwards seed in FloatRangeHyperParam.
core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala	Adds regression tests for range bounds and seed reproducibility across numeric hyperparams.
core/src/main/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalize.scala	Fixes setForm to validate the passed-in value, preventing invalid forms from being stored.
core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalizeSuite.scala	Adds tests for setForm validation behavior and supplies valid fuzzer examples for form.
core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala	Replaces the incorrect “singleton-set” similarity with bigram Jaccard similarity and adds an empty-input convention.
cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala	Adds tests covering similarity semantics (identity/orthogonality/partial overlap/case-insensitivity/symmetry).

Review details

Tip

Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

• Files reviewed: 6/6 changed files
• Comments generated: 3
• Review effort level: Lite

[Codecov Comments Bot (codecov-commenter)]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.11%. Comparing base (38b078a) to head (6f62f71).

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2625      +/-   ##
==========================================
+ Coverage   87.05%   87.11%   +0.06%     
==========================================
  Files         338      338              
  Lines       18843    18858      +15     
  Branches     1805     1764      -41     
==========================================
+ Hits        16403    16429      +26     
+ Misses       2440     2429      -11     

☔ 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.

[Rana Singh (ranadeepsingh)]

Removed the modulo bias from LongRangeHyperParam.

Math.floorMod(nextLong(), range) is not uniform: 2^64 is not divisible by an arbitrary range, so the low residues get drawn one extra time. The bias is tiny (~range/2^64) but there is no reason to accept it when Random.nextInt(bound) already does the exactly-uniform thing for Int -- this makes Long behave the same way, via the standard rejection-sampling loop (mask fast path for power-of-two bounds, reject the partial final block otherwise).

Written tail-recursively because scalastyle bans while. Both branches are covered by tests -- power-of-two and non-power-of-two -- which assert full bucket coverage and roughly even counts, and which also prove the recursion terminates.

Local: scalastyle clean, 19/19 pass.

/azp run

[Copilot]

Review details

Suppressed comments (2)

core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala:23

• For strings shorter than 2 characters, sliding(2).toSet is empty, so this returns 1.0 for any pair of 0/1-length strings (e.g., "a" vs "b"), which is incorrect for a similarity measure.

    val a = s1.toLowerCase.sliding(2).toSet
    val b = s2.toLowerCase.sliding(2).toSet
    if (a.isEmpty && b.isEmpty) 1.0 else a.intersect(b).size.toDouble / (a | b).size.toDouble

core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala:37

• range = max - min can overflow for very wide intervals (e.g., min = Long.MinValue, max = Long.MaxValue). In that case range becomes non-positive and getNext() returns min for every draw, which breaks the distribution even though max > min.

  def getNext(): Long = {
    val range = max - min
    // nextLong() spans the full 64-bit range, so multiplying it by the range overflows and
    // escapes [min, max) entirely. Reduce into the range first, as nextInt(range) does.
    if (range <= 0) min else boundedNextLong(range) + min
  }

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Rana Singh (ranadeepsingh)]

Addressed all three review threads (details in each). Summary: LongRangeHyperParam.getNext now requires max >= min and handles the too-wide-to-represent span on its own rejection branch instead of misreading the wrapped subtraction as an empty range; added explicit 0/1-character jaccard assertions. The sliding(2) concern turned out not to hold -- Scala keeps the short final window, so 1-character strings yield a singleton set, not an empty one, and that is now pinned by a test.

Local: scalastyle clean across core and cognitive, 12/12 automl and 6/6 ModelEqualitySuite.

/azp run

[Rana Singh (ranadeepsingh)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Review details

Suppressed comments (2)

core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala:17

• The scaladoc says the Jaccard index is computed over character bigrams, but the implementation intentionally keeps partial windows so 0/1-length strings become empty-set/unigram sets. Updating the doc helps prevent confusion for future callers.

  /** Similarity of two strings as the Jaccard index over their character bigrams. */

core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala:22

• jaccardSimilarity uses String.toLowerCase without an explicit locale, making results dependent on the JVM default locale (e.g., Turkish locale casing can change character mappings). Use Locale.ROOT to keep similarity deterministic across environments.

    val a = s1.toLowerCase.sliding(2).toSet
    val b = s2.toLowerCase.sliding(2).toSet

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Rana Singh (ranadeepsingh)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Review details

Suppressed comments (2)

core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala:22

• toLowerCase without an explicit locale is locale-dependent (e.g., Turkish locale casing) and can make similarity results non-deterministic across environments/executors. The codebase already uses Locale.ROOT for case-folding in similar string-normalization logic (e.g., SpeechToTextSDK.scala:55).

    val a = s1.toLowerCase.sliding(2).toSet
    val b = s2.toLowerCase.sliding(2).toSet

core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala:40

• Local val draws shadows the suite-level private val draws, which makes the test harder to read and can be confusing when adjusting the draw counts. Rename the local collection to avoid shadowing.

    val hp = new LongRangeHyperParam(Long.MinValue + 1, Long.MaxValue, seed = 7)
    val draws = (1 to 200).map(_ => hp.getNext())
    assert(draws.forall(v => v >= Long.MinValue + 1 && v < Long.MaxValue))
    assert(draws.distinct.length > 1)

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Rana Singh (ranadeepsingh)]

/azp run

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[Copilot]

Review details

Suppressed comments (1)

core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala:22

• toLowerCase without an explicit locale is locale-dependent (e.g., Turkish locale casing rules) and can make jaccardSimilarity produce different results depending on the JVM’s default locale. Use Locale.ROOT for deterministic, locale-invariant case folding.

    val a = s1.toLowerCase.sliding(2).toSet
    val b = s2.toLowerCase.sliding(2).toSet

• Files reviewed: 6/6 changed files
• Comments generated: 0 new
• Review effort level: Lite

[Brendan Walsh (BrendanWalsh)]

Approved — automated multi-model review cycle.

Reviewed by Claude Opus 5 and GPT-5.6 Sol (both at maximum reasoning effort) against a blocking-issues-only rubric, followed by an independent adjudication pass. Final result: 0 blocking findings on commit 6f62f71b. All 30 checks are green.

Two findings were raised and both were overturned on verification:

1. "Float sampling can emit the exclusive maximum" (HyperparamBuilder.scala:74) — not blocking.
The doubleRange.getNext().toFloat narrowing is byte-identical on origin/master; the same 1.0f result reproduces on the base branch with seed 0 over the range [nextDown(1.0f), 1.0f). Seed forwarding therefore does not introduce the behaviour — it is pre-existing, and no hyperparameter range in the repo comes within a float ULP of its maximum. (Python cannot construct a FloatRangeHyperParam at all, since py4j marshals to double.)

2. "Form validation is bypassed by the Python API" (UnicodeNormalize.scala:37) — not blocking.
The codegen claim is correct: Wrappable.scala:189 does emit self._set(form=value), so the generated Python setter bypasses the JVM setter. However, the pre-PR Scala setter validated getForm — reading the field before assigning it — which made it a no-op that rejected nothing on any path. Python was never protected, so this PR is a strict improvement rather than a regression, and the residual failure mode is a loud IllegalArgumentException naming the offending value, not a silent wrong result.

Additional verification performed on the core sampling change:

• boundedNextLong's power-of-two branch was checked against the JDK's own nextLong(origin, bound) and is a line-for-line match (if ((n & m) == 0L) r = (r & m) + origin;). Measured uniform: at bound 8, 464–536 per bucket against an expected 500, with ≤1.73% deviation over 100k draws at bounds 2/4/16. The rejection loop u + m - r < 0 correctly mirrors java.util.Random.nextInt.
• The exact java.util.Random LCG plus this PR's algorithm were simulated end to end; all 11 new deterministic assertions pass, including the [Long.MinValue+1, Long.MaxValue) overflow branch and seed reproducibility across all four numeric types.
• ModelEquality.jaccardSimilarity has no in-repo callers (the two speech suites hold private copies), and TuneHyperparameters.fit draws sequentially on the driver, so there is no shared-Random thread exposure.

Non-blocking observations, recorded but not gating:

• range == 0 returns min whereas IntRangeHyperParam throws — pre-existing, and now pinned by a new test.
• jaccardSimilarity is public with unspecified null/empty-set semantics; worth documenting if it gains callers.