Skip to content

fix: cves - log4j, parquet-avro etc #1053

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 12, 2025
Merged

fix: cves - log4j, parquet-avro etc #1053

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
16b23c6
fix: cve
nikhil-zlai Aug 11, 2025
02c1b00
should matchers
nikhil-zlai Aug 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 10 additions & 7 deletions aggregator/src/test/scala/ai/chronon/aggregator/test/FrequentItemsTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import java.util
import ai.chronon.api.ScalaJavaConversions._
import org.apache.datasketches.frequencies.ErrorType
import org.scalatest.matchers.should.Matchers._
import org.scalatest.matchers.should.Matchers

import scala.util.Random

Expand Down Expand Up @@ -182,16 +183,18 @@ class FrequentItemsTest extends AnyFlatSpec {
}

"MostFrequentK" should "always produce nearly k elements when cardinality is > k" in {
val k = 10
val topFrequentItems = new FrequentItems[java.lang.Long](k)
val frequentItemsIr = topFrequentItems.prepare(0)
for (i <- 0 until 900) {
val k = 10
val topFrequentItems = new FrequentItems[java.lang.Long](k)
val frequentItemsIr = topFrequentItems.prepare(0)

createSkewedData().foreach(i => topFrequentItems.update(frequentItemsIr, i))
createSkewedData().foreach(i => topFrequentItems.update(frequentItemsIr, i))

val topHistogram = topFrequentItems.finalize(frequentItemsIr)
val topHistogram = topFrequentItems.finalize(frequentItemsIr)

math.abs(topHistogram.size() - k) <= 2 shouldBe true
heavyHitterElems.foreach(elem => topHistogram.containsKey(elem.toString))
(topHistogram.size() - k) should be < 3
heavyHitterElems.foreach(elem => topHistogram.containsKey(elem.toString))
}
Comment on lines +186 to +197
Copy link
Contributor

@coderabbitai coderabbitai bot Aug 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

⚠️ Potential issue

Fix assertion logic and enforce heavy-hitter checks; tame flakiness/perf.

  • Only upper-bound is checked; use abs(delta) <= 2.
  • containsKey() results aren’t asserted; test may pass even when keys are missing.
  • 900 iterations with unseeded RNG can be slow/flaky.

Proposed patch:

-for (i <- 0 until 900) {
+for (i <- 0 until 100) {
   val k = 10
   val topFrequentItems = new FrequentItems[java.lang.Long](k)
   val frequentItemsIr = topFrequentItems.prepare(0)

   createSkewedData().foreach(i => topFrequentItems.update(frequentItemsIr, i))

   val topHistogram = topFrequentItems.finalize(frequentItemsIr)

-  (topHistogram.size() - k) should be < 3
-  heavyHitterElems.foreach(elem => topHistogram.containsKey(elem.toString))
+  math.abs(topHistogram.size() - k) should be <= 2
+  heavyHitterElems.foreach(elem => topHistogram.containsKey(elem.toString) shouldBe true)
 }

Optionally, make the randomness deterministic:

// Outside this block:
private val rnd = new Random(42L)
// In createSkewedData:
Random.shuffle(longTail ++ heavyHitters)(rnd)
🤖 Prompt for AI Agents 
In aggregator/src/test/scala/ai/chronon/aggregator/test/FrequentItemsTest.scala
around lines 186 to 197, the test only checks an upper bound on size, doesn't
assert containsKey results, and runs 900 unseeded iterations causing flakiness
and slowness; change the size assertion to assert math.abs(topHistogram.size() -
k) <= 2 (or equivalent) so both upper and lower bounds are enforced, add
explicit assertions for each heavyHitterElems element (e.g.,
heavyHitterElems.foreach(elem => topHistogram.containsKey(elem.toString)
shouldBe true) ) to fail when keys are missing, and reduce iterations (e.g.,
from 900 to a much smaller number) and make randomness deterministic by seeding
Random (create a private val rnd = new Random(42L) and use it in
createSkewedData with Random.shuffle(...)(rnd)) to tame flakiness and improve
test performance.

}

"HeavyHittersK" should "always produce only heavy hitter elements regardless of cardinality" in {
Expand Down
Loading