From 9c8b4a6fa965d12484c0dcf0baf11b0044a30a66 Mon Sep 17 00:00:00 2001 From: Ranadeep Singh Date: Wed, 12 Aug 2026 02:16:52 +0000 Subject: [PATCH 1/3] fix: correct hyperparameter range overflow, seed forwarding, form validation Three defects surfaced while reviewing test coverage for these utilities. Each fix ships with a test that fails against the current code and passes after. LongRangeHyperParam.getNext() computed `(random.nextLong() * range) + min`. nextLong() spans the full 64-bit range, so multiplying it by the range overflows and escapes [min, max) entirely: with min=0, max=100 it produced values such as -4494195807357089668. The Int and Double variants already reduce into the range first (nextInt(range), nextDouble() * range). Reduce the Long draw with floorMod, and return min for an empty range rather than dividing by zero. FloatRangeHyperParam built its inner DoubleRangeHyperParam without passing `seed`, so every instance drew from seed 0 and two Float params configured with different seeds produced identical sequences. Reproducibility and seed independence were both silently broken. UnicodeNormalize.setForm validated `getForm` -- the value already set -- rather than the incoming `value`. Since getForm falls back to "NFKD", validation always passed and an invalid form was accepted, only failing later inside the UDF on the executors where the cause is hard to attribute. ModelEquality.jaccardSimilarity built Set(s1)/Set(s2), singleton sets of the whole strings, so it could only ever return 1.0 or 0.0 -- a strict equality check, not a similarity measure. Use character bigrams, matching the working implementation already used in the speech test suites, and define the both-empty case as 1.0 instead of NaN. UnicodeNormalizeSuite now supplies a valid `form` example through the GetterSetterFuzzing hook, since the generic fuzzer probes String setters with "foo" and setForm legitimately rejects it now. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../core/utils/utils/ModelEqualitySuite.scala | 16 +++++ .../synapse/ml/automl/HyperparamBuilder.scala | 6 +- .../synapse/ml/core/utils/ModelEquality.scala | 9 ++- .../synapse/ml/stages/UnicodeNormalize.scala | 5 +- .../ml/automl/HyperparamRangeSuite.scala | 70 +++++++++++++++++++ .../ml/stages/UnicodeNormalizeSuite.scala | 17 +++++ 6 files changed, 116 insertions(+), 7 deletions(-) create mode 100644 core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala diff --git a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala index b2bc5ed750f..198d87aba92 100644 --- a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala +++ b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala @@ -12,6 +12,22 @@ import com.microsoft.azure.synapse.ml.stages.DropColumns class ModelEqualitySuite extends TestBase { spark + test("jaccardSimilarity grades partial overlap") { + // Sets of whole strings would collapse this to a 1.0/0.0 equality check. + assert(ModelEquality.jaccardSimilarity("abcd", "abcd") === 1.0) + assert(ModelEquality.jaccardSimilarity("abcd", "wxyz") === 0.0) + val partial = ModelEquality.jaccardSimilarity("the quick brown fox", "the quick brown cat") + assert(partial > 0.0 && partial < 1.0) + assert(partial > ModelEquality.jaccardSimilarity("the quick brown fox", "entirely unlike")) + } + + test("jaccardSimilarity is case insensitive and symmetric") { + assert(ModelEquality.jaccardSimilarity("Hello World", "hello world") === 1.0) + assert(ModelEquality.jaccardSimilarity("kitten", "sitting") + === ModelEquality.jaccardSimilarity("sitting", "kitten")) + assert(ModelEquality.jaccardSimilarity("", "") === 1.0) + } + test("Complex param equality") { val m1 = new TextSentiment().setLocation("eastus") val m2 = new TextSentiment().setLocation("eastus") diff --git a/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala b/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala index 475466bf658..7e22f2f3ce7 100644 --- a/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala +++ b/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala @@ -30,7 +30,9 @@ class LongRangeHyperParam(min: Long, max: Long, seed: Long = 0) def getNext(): Long = { val range = max - min - (random.nextLong() * range) + 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, mirroring nextInt(range). + if (range <= 0) min else Math.floorMod(random.nextLong(), range) + min } } @@ -38,7 +40,7 @@ class LongRangeHyperParam(min: Long, max: Long, seed: Long = 0) class FloatRangeHyperParam(min: Float, max: Float, seed: Long = 0) extends RangeHyperParam[Float](min, max, seed) { - val doubleRange = new DoubleRangeHyperParam(min.toDouble, max.toDouble) + val doubleRange = new DoubleRangeHyperParam(min.toDouble, max.toDouble, seed) def getNext(): Float = { doubleRange.getNext().toFloat } diff --git a/core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala b/core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala index 797e82d122d..66ad00917ec 100644 --- a/core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala +++ b/core/src/main/scala/com/microsoft/azure/synapse/ml/core/utils/ModelEquality.scala @@ -14,10 +14,13 @@ trait ParamEquality[T] extends Param[T] { object ModelEquality { + /** Similarity of two strings as the Jaccard index over their character bigrams. */ def jaccardSimilarity(s1: String, s2: String): Double = { - val a = Set(s1) - val b = Set(s2) - a.intersect(b).size.toDouble / (a | b).size.toDouble + // Sets of the whole strings would only ever yield 1.0 or 0.0, making this a strict + // equality check rather than 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 } def assertEqual(m1: Params, m2: Params): Unit = { diff --git a/core/src/main/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalize.scala b/core/src/main/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalize.scala index 5ef14b37642..6ef6df50c4d 100644 --- a/core/src/main/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalize.scala +++ b/core/src/main/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalize.scala @@ -32,8 +32,9 @@ class UnicodeNormalize(val uid: String) extends Transformer /** @group setParam */ def setForm(value: String): this.type = { - // check input value - Normalizer.Form.valueOf(getForm) + // Validate the incoming value, not the value already set. Validating getForm let an invalid + // form be stored and only surface later, inside the UDF on the executors. + Normalizer.Form.valueOf(value) set("form", value) } diff --git a/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala b/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala new file mode 100644 index 00000000000..04447e049f3 --- /dev/null +++ b/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala @@ -0,0 +1,70 @@ +// Copyright (C) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. See LICENSE in project root for information. + +package com.microsoft.azure.synapse.ml.automl + +import com.microsoft.azure.synapse.ml.core.test.base.TestBase + +// scalastyle:off magic.number +/** Covers the bounds and seeding contract shared by every RangeHyperParam. */ +class HyperparamRangeSuite extends TestBase { + + private val draws = 500 + + test("LongRangeHyperParam stays within its range") { + val hp = new LongRangeHyperParam(0L, 100L, seed = 42) + val values = (1 to draws).map(_ => hp.getNext()) + assert(values.forall(v => v >= 0L && v < 100L), s"out of range: ${values.filter(v => v < 0L || v >= 100L)}") + assert(values.toSet.size > 1) + } + + test("LongRangeHyperParam stays within a range too large to fit in an Int") { + val min = -4000000000L + val max = 4000000000L + val hp = new LongRangeHyperParam(min, max, seed = 7) + val values = (1 to draws).map(_ => hp.getNext()) + assert(values.forall(v => v >= min && v < max)) + assert(values.exists(_ < 0L) && values.exists(_ > 0L)) + } + + test("LongRangeHyperParam with an empty range returns min") { + val hp = new LongRangeHyperParam(5L, 5L, seed = 42) + assert((1 to 10).forall(_ => hp.getNext() === 5L)) + } + + test("IntRangeHyperParam stays within its range") { + val hp = new IntRangeHyperParam(5, 15, seed = 42) + val values = (1 to draws).map(_ => hp.getNext()) + assert(values.forall(v => v >= 5 && v < 15)) + } + + test("DoubleRangeHyperParam stays within its range") { + val hp = new DoubleRangeHyperParam(0.0, 1.0, seed = 42) + assert((1 to draws).map(_ => hp.getNext()).forall(v => v >= 0.0 && v < 1.0)) + } + + test("FloatRangeHyperParam stays within its range") { + val hp = new FloatRangeHyperParam(0.0f, 1.0f, seed = 42) + assert((1 to draws).map(_ => hp.getNext()).forall(v => v >= 0.0f && v < 1.0f)) + } + + test("equal seeds reproduce equal sequences and differing seeds diverge") { + def draw(hp: Dist[_]): Seq[Any] = (1 to 20).map(_ => hp.getNext) + + assert(draw(new IntRangeHyperParam(0, 1000, 11)) === draw(new IntRangeHyperParam(0, 1000, 11))) + assert(draw(new LongRangeHyperParam(0L, 1000L, 11)) === draw(new LongRangeHyperParam(0L, 1000L, 11))) + assert(draw(new DoubleRangeHyperParam(0.0, 1.0, 11)) === draw(new DoubleRangeHyperParam(0.0, 1.0, 11))) + // FloatRangeHyperParam delegates to an inner DoubleRangeHyperParam; it must forward its seed. + assert(draw(new FloatRangeHyperParam(0.0f, 1.0f, 11)) === draw(new FloatRangeHyperParam(0.0f, 1.0f, 11))) + assert(draw(new FloatRangeHyperParam(0.0f, 1.0f, 11)) !== draw(new FloatRangeHyperParam(0.0f, 1.0f, 12))) + } + + test("HyperParamUtils.getRangeHyperParam honors the seed for every numeric type") { + Seq[(Any, Any)]((0, 1000), (0L, 1000L), (0.0, 1.0), (0.0f, 1.0f)).foreach { case (min, max) => + val a = HyperParamUtils.getRangeHyperParam(min, max, 99) + val b = HyperParamUtils.getRangeHyperParam(min, max, 99) + assert((1 to 20).map(_ => a.getNext) === (1 to 20).map(_ => b.getNext), s"seed ignored for $min/$max") + } + } +} +// scalastyle:on magic.number diff --git a/core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalizeSuite.scala b/core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalizeSuite.scala index e7d098ca6fb..664140c5c6b 100644 --- a/core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalizeSuite.scala +++ b/core/src/test/scala/com/microsoft/azure/synapse/ml/stages/UnicodeNormalizeSuite.scala @@ -5,6 +5,7 @@ package com.microsoft.azure.synapse.ml.stages import com.microsoft.azure.synapse.ml.core.test.base.TestBase import com.microsoft.azure.synapse.ml.core.test.fuzzing.{TestObject, TransformerFuzzing} +import org.apache.spark.ml.param.Param import org.apache.spark.ml.util.MLReadable import org.apache.spark.sql.DataFrame @@ -49,9 +50,25 @@ class UnicodeNormalizeSuite extends TestBase with TransformerFuzzing[UnicodeNorm test("Check for NFKD forms") { testForm("NFKD", expectedResultDecomposed) } + test("setForm rejects an invalid form at set time") { + // An unvalidated form used to be accepted here and only fail later inside the UDF, + // on the executors, where the cause is far harder to attribute. + assertThrows[IllegalArgumentException](new UnicodeNormalize().setForm("NOT_A_FORM")) + } + + test("setForm accepts every supported form") { + java.text.Normalizer.Form.values().foreach { f => + assert(new UnicodeNormalize().setForm(f.name).getForm === f.name) + } + } + def testObjects(): Seq[TestObject[UnicodeNormalize]] = List(new TestObject( new UnicodeNormalize().setInputCol("words").setOutputCol("out"), makeBasicDF())) + // The generic fuzzer probes String setters with "foo"; form only accepts Normalizer.Form names. + override def getterSetterParamExamples(pipelineStage: UnicodeNormalize): Map[Param[_], Any] = + Map[Param[_], Any]((pipelineStage.form, "NFC")) + override def reader: MLReadable[_] = UnicodeNormalize } From 9be8713b6714c008d20b84b8be3103a55ea58033 Mon Sep 17 00:00:00 2001 From: SynapseML CI Date: Wed, 12 Aug 2026 03:27:17 +0000 Subject: [PATCH 2/3] fix: remove modulo bias from LongRangeHyperParam sampling Math.floorMod(nextLong(), range) is non-uniform: the residues below 2^64 mod range are drawn one extra time. Replace it with the standard rejection-sampling loop so Long ranges are exactly as uniform as Random.nextInt(bound) already is for Int. Adds coverage tests for both branches (power-of-two mask and non-power-of-two rejection) which also prove the recursion terminates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../synapse/ml/automl/HyperparamBuilder.scala | 22 +++++++++++++++++-- .../ml/automl/HyperparamRangeSuite.scala | 16 ++++++++++++++ 2 files changed, 36 insertions(+), 2 deletions(-) diff --git a/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala b/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala index 7e22f2f3ce7..7c29c988c99 100644 --- a/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala +++ b/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala @@ -5,6 +5,7 @@ package com.microsoft.azure.synapse.ml.automl import org.apache.spark.ml.param._ +import scala.annotation.tailrec import scala.collection.JavaConverters._ import scala.collection.mutable import scala.util.Random @@ -31,8 +32,25 @@ class LongRangeHyperParam(min: Long, max: Long, seed: Long = 0) 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, mirroring nextInt(range). - if (range <= 0) min else Math.floorMod(random.nextLong(), range) + min + // escapes [min, max) entirely. Reduce into the range first, as nextInt(range) does. + if (range <= 0) min else boundedNextLong(range) + min + } + + /** Uniform draw from [0, bound), rejecting the partial final block so the result carries no + * modulo bias. Mirrors what java.util.Random.nextInt(bound) already does for Int. + */ + private def boundedNextLong(bound: Long): Long = { + val m = bound - 1 + if ((bound & m) == 0L) { + random.nextLong() & m // bound is a power of two, so the low bits are already uniform + } else { + @tailrec def draw(): Long = { + val u = random.nextLong() >>> 1 + val r = u % bound + if (u + m - r < 0L) draw() else r + } + draw() + } } } diff --git a/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala b/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala index 04447e049f3..11682ce688f 100644 --- a/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala +++ b/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala @@ -32,6 +32,22 @@ class HyperparamRangeSuite extends TestBase { assert((1 to 10).forall(_ => hp.getNext() === 5L)) } + test("LongRangeHyperParam covers a power-of-two range without bias") { + // Power-of-two bounds take the mask branch; every bucket must still be reachable. + val hp = new LongRangeHyperParam(0L, 8L, seed = 3) + val counts = (1 to 4000).map(_ => hp.getNext()).groupBy(identity).map { case (k, v) => k -> v.size } + assert(counts.keySet === (0L until 8L).toSet) + assert(counts.values.forall(c => c > 300 && c < 700), s"uneven distribution: $counts") + } + + test("LongRangeHyperParam covers a non-power-of-two range without bias") { + // Non-power-of-two bounds take the rejection branch, which must terminate and stay uniform. + val hp = new LongRangeHyperParam(0L, 7L, seed = 3) + val counts = (1 to 4000).map(_ => hp.getNext()).groupBy(identity).map { case (k, v) => k -> v.size } + assert(counts.keySet === (0L until 7L).toSet) + assert(counts.values.forall(c => c > 350 && c < 800), s"uneven distribution: $counts") + } + test("IntRangeHyperParam stays within its range") { val hp = new IntRangeHyperParam(5, 15, seed = 42) val values = (1 to draws).map(_ => hp.getNext()) From 6f62f71bd87e1f104833046167d2674129765c3a Mon Sep 17 00:00:00 2001 From: SynapseML CI Date: Wed, 12 Aug 2026 03:43:46 +0000 Subject: [PATCH 3/3] fix: make LongRangeHyperParam total and cover short-string jaccard cases require(max >= min) instead of silently returning min for an inverted range, and draw with rejection when the span is too wide to represent as a Long rather than treating the wrapped negative as an empty range. Adds explicit assertions for 0- and 1-character jaccard inputs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../ml/core/utils/utils/ModelEqualitySuite.scala | 11 +++++++++++ .../synapse/ml/automl/HyperparamBuilder.scala | 15 ++++++++++++++- .../synapse/ml/automl/HyperparamRangeSuite.scala | 13 +++++++++++++ 3 files changed, 38 insertions(+), 1 deletion(-) diff --git a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala index 198d87aba92..4b021ed2707 100644 --- a/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala +++ b/cognitive/src/test/scala/com/microsoft/azure/synapse/ml/core/utils/utils/ModelEqualitySuite.scala @@ -28,6 +28,17 @@ class ModelEqualitySuite extends TestBase { assert(ModelEquality.jaccardSimilarity("", "") === 1.0) } + test("jaccardSimilarity handles strings shorter than one bigram") { + // sliding(2) keeps the short final window, so a 1-char string yields Set(char) + // rather than the empty set. Distinct 1-char strings must therefore score 0.0, + // not fall into the both-empty shortcut. + assert(ModelEquality.jaccardSimilarity("a", "b") === 0.0) + assert(ModelEquality.jaccardSimilarity("a", "a") === 1.0) + assert(ModelEquality.jaccardSimilarity("", "a") === 0.0) + assert(ModelEquality.jaccardSimilarity("a", "") === 0.0) + assert(ModelEquality.jaccardSimilarity("", "") === 1.0) + } + test("Complex param equality") { val m1 = new TextSentiment().setLocation("eastus") val m2 = new TextSentiment().setLocation("eastus") diff --git a/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala b/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala index 7c29c988c99..31e9f20b13d 100644 --- a/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala +++ b/core/src/main/scala/com/microsoft/azure/synapse/ml/automl/HyperparamBuilder.scala @@ -30,10 +30,23 @@ class LongRangeHyperParam(min: Long, max: Long, seed: Long = 0) extends RangeHyperParam[Long](min, max, seed) { def getNext(): Long = { + require(max >= min, s"LongRangeHyperParam requires max >= min, got min=$min, max=$max") 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 + if (range == 0) { + min + } else if (range < 0) { + // The span is wider than Long.MaxValue so it cannot be represented as a Long. Draw from + // the whole 64-bit range instead; more than half of it lands in bounds, so this is cheap. + @tailrec def drawInBounds(): Long = { + val v = random.nextLong() + if (v >= min && v < max) v else drawInBounds() + } + drawInBounds() + } else { + boundedNextLong(range) + min + } } /** Uniform draw from [0, bound), rejecting the partial final block so the result carries no diff --git a/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala b/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala index 11682ce688f..3e150dfaf74 100644 --- a/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala +++ b/core/src/test/scala/com/microsoft/azure/synapse/ml/automl/HyperparamRangeSuite.scala @@ -27,6 +27,19 @@ class HyperparamRangeSuite extends TestBase { assert(values.exists(_ < 0L) && values.exists(_ > 0L)) } + test("LongRangeHyperParam rejects an inverted range instead of silently returning min") { + val hp = new LongRangeHyperParam(10L, 5L, seed = 1) + assertThrows[IllegalArgumentException](hp.getNext()) + } + + test("LongRangeHyperParam stays in bounds when the span overflows Long") { + // max - min wraps negative here, so the ordinary reduction path cannot be used. + 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) + } + test("LongRangeHyperParam with an empty range returns min") { val hp = new LongRangeHyperParam(5L, 5L, seed = 42) assert((1 to 10).forall(_ => hp.getNext() === 5L))