Skip to content

Add fingerprint_id and run_id to value table for indexed detection qu… - #306

Open
stalep wants to merge 1 commit into
Hyperfoil:mainfrom
stalep:fingerprint-index
Open

Add fingerprint_id and run_id to value table for indexed detection qu…#306
stalep wants to merge 1 commit into
Hyperfoil:mainfrom
stalep:fingerprint-index

Conversation

@stalep

@stalep stalep commented Sep 2, 2026

Copy link
Copy Markdown
Member

Introduce FingerprintEntity table for deduplicated fingerprint identities and add fingerprint_id + root_id FK columns on the value table with compound indexes. This enables direct indexed lookups for detection queries instead of recursive CTE DAG traversals.

Performance: findByFingerprint() replaces recursive CTEs in the RelativeDifference fast path.
Per-upload detection time drops from57-344s to 0.1-0.9s with no linear growth as history accumulates.

Semantics: the fast path preserves window-scanning behavior for out-of-order uploads. When a domain value is inserted in the middle of existing history, following domain values in the window are re-evaluated. Delete-then-recompute cleanup prevents duplicate detections, matching Horreum's invalidation pattern.

Fingerprint stamping runs in ProcessingService.completeIngestion() after all work items complete, avoiding timing issues and SQLite lock contention. Values without fingerprint_id fall back to the legacy CTE-based detection path.

FingerprintEntity cleanup on node deletion: deleting a FingerprintNode nullifies fingerprint_id on affected values and removes orphaned fingerprint_entry rows.

Single-fingerprint-per-group assumption: stampFingerprintOnSiblings uses first-writer-wins; multi-fingerprint groups fall back to the legacy CTE path (correct but slower).

Closes #303

@willr3

willr3 commented Sep 2, 2026

Copy link
Copy Markdown
Collaborator

run_id

please keep in mind that there is not a run in h5m and we made a collective choice to not use that terminology. Perhaps root_id to reference the root node's value?

@stalep

stalep commented Sep 2, 2026

Copy link
Copy Markdown
Member Author

run_id

please keep in mind that there is not a run in h5m and we made a collective choice to not use that terminology. Perhaps root_id to reference the root node's value?

Ah, yeah, root_id can work.

@stalep
stalep marked this pull request as ready for review September 2, 2026 16:36
@willr3
willr3 self-requested a review September 2, 2026 17:15

@willr3 willr3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If this PR aims to address issue #303 then I think we should discuss the issue before proposing code changes. Proposing code changes before we fully understand the issue risks wasting time for both the person writing the PR and the person reviewing.

How does this PR handle when there is more than one fingerprint node in the group? The legacy Horreum design restricted users to a single fingerprint definition per Test but h5m does not have that restriction.

The FingerprintEntity is basically a shared cache, how are entries invalidated and updated when the corresponding FingerprintNode changes? We should have test coverage for this behavior to ensure correctness.

I think it best we not merge this PR until we agree on when a change should be detected and have concrete examples of those conditions.

@stalep
stalep force-pushed the fingerprint-index branch 9 times, most recently from 1340090 to c84361f Compare September 3, 2026 14:11
…ueries

Introduce FingerprintEntity table for deduplicated fingerprint identities
and add fingerprint_id + root_id FK columns on the value table with
compound indexes. This enables direct indexed lookups for detection
queries instead of recursive CTE DAG traversals.

Performance: findByFingerprint() replaces recursive CTEs in the
RelativeDifference fast path. Per-upload detection time drops from
57-344s to 0.1-0.9s with no linear growth as history accumulates.

Semantics: the fast path preserves window-scanning behavior for
out-of-order uploads. When a domain value is inserted in the middle
of existing history, following domain values in the window are
re-evaluated. Delete-then-recompute cleanup prevents duplicate
detections, matching Horreum's invalidation pattern.

Fingerprint stamping runs in ProcessingService.completeIngestion()
after all work items complete, avoiding timing issues and SQLite
lock contention. Values without fingerprint_id fall back to the
legacy CTE-based detection path.

FingerprintEntity cleanup on node deletion: deleting a FingerprintNode
nullifies fingerprint_id on affected values and removes orphaned
fingerprint_entry rows.

Single-fingerprint-per-group assumption: stampFingerprintOnSiblings
uses first-writer-wins; multi-fingerprint groups fall back to the
legacy CTE path (correct but slower).

Closes Hyperfoil#303
@stalep

stalep commented Sep 4, 2026

Copy link
Copy Markdown
Member Author

I looked into splitting this into a correctness fix and a performance fix. They're the same problem.
The duplicate detections (12,682 for 15 runs of testId=113) have two causes:

  1. Stale detection cleanup only runs when !rtrn.isEmpty() and keeps old detections that match a new one at the same domain value. Both survive.
  2. allDomainValues accumulates across sdIdx iterations without resetting. Later uploads re-evaluate at domain values from earlier uploads' windows, creating new detections each time.

I fixed the first issue on the CTE path: removed the !rtrn.isEmpty() guard, moved cleanup before evaluation, deleted unconditionally. That dropped detections from 12,682 to 322. The accumulation in 2. is baked into how the CTE path iterates.
The fast path in this PR replaces both. It batch-fetches all range/domain values upfront, builds a position index, and evaluates at the domain values within the upload's window. No cross-upload accumulation. Delete-then-recompute runs before evaluation and deletes unconditionally.
Result: 29 detections. The 2x ratio vs Horreum's 15 comes from h5m having 2 RD configs per variable with different thresholds, I created #315 to track that.
The CTE path is unchanged. Unit tests that create values manually (no pipeline) still use it via the fingerprintValue.fingerprint == null fallback.

@willr3 willr3 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The code change in this PR introduces a separate if conditional into calculateRelativeDifferenceValues that avoids the existing calculation logic for the conditions observed in the associated issue #303. Adding a separate conditional execution for an observed issue but leaving the existing logic unchanged is only a valid fix if the existing logic is correct. We already see where the existing logic produces excessive change detection values in #303 so it is quite unlikely that the logic is correct.
The code in this PR bifurcates the calculation of relative difference values to over fit the conditions created when importing data from Horreum. Merging this would increase the complexity of calculating relative difference, increase the complexity of the entity mode, and leave the problematic logic unfixed in relative difference calculation for any condition not captured by the fingerprint caching.
I believe we should not consider fingerprint caching or schema denormalization for performance improvements until we have the algorithms implemented correctly and calculateRelativeDifferenceValues producing a demonstrably correct number of change detections.

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.

RelativeDifference detection performance: reduce recursive CTE queries per upload

2 participants