planner/optimizer — cost_model.rs's ingest_cost and query_cost both bill for the memory used by retained windows on Merge/Subtract candidates, and the two charges aren't representing different things — they're the same bytes, charged twice.
The double-count. ingest_cost's mem_retain term (around line 91-94) already bills, continuously, for holding retained completed windows in memory: candidate.n_windows * n * mem_bytes_per_instance for Tumbling. query_cost's Merge branch (around line 120-126) then bills the same num_windows * n * mem_bytes_per_instance quantity again, this time weighted by query_mem and scaled by f_a (query frequency) in total_cost_rate. So every query incurs a second rent payment, on top of the rent already being paid continuously via ingest_cost. There's no real resource that justifies the second charge — if it were meant to represent something different (e.g. a transient compute buffer needed during the merge operation itself, rather than the retained set), it would be sized differently; instead it's sized identically to the retained set, which is the signature of double-billing the same bytes rather than billing a different, smaller thing.
The same applies to QueryMethod::Subtract's mem term (2.0 * n * mem_bytes_per_instance, around line 128-134) once the magnitude bug below is fixed.
A second, related bug: wrong magnitude for Subtract. ingest_cost's mem_retain always uses candidate.n_windows for Tumbling, regardless of query_method — it has no visibility into which method was actually chosen. But Subtract structurally only ever needs 2 retained checkpoints (the current cumulative value and the checkpoint from range_a ago), no matter how large n_windows is — that's exactly why query_cost's own Subtract branch already hardcodes 2.0, not num_windows. So today, ingest_cost overbills Subtract candidates as if they needed full Merge-style retention (e.g. 20 instances) when only 2 are ever needed. This is related to the retention-count mismatch in #412, but is a separate bug specific to the cost model's mem_retain calculation.
Proposed fix (one coherent change for both problems):
- Make
ingest_cost's mem_retain query-method-aware: n_windows for Merge, 2 for Subtract, 0 for Direct/Exact (currently it can't tell, since it only receives candidate.n_windows, not candidate.query_method).
- Drop the mem term from
query_cost entirely for Merge and Subtract, keeping only the CPU terms (merges * merge_cpu_secs / subtract_cpu_secs) — since the retained memory is already fully accounted for, once correctly, on the ingest side.
Until fixed, cost comparisons between Merge, Subtract, Sliding, and Direct candidates are skewed — Merge/Subtract candidates are penalized more than they should be relative to Sliding/Direct, which don't have this double-billing.
Found during code review of PR #407.
planner/optimizer —
cost_model.rs'singest_costandquery_costboth bill for the memory used by retained windows onMerge/Subtractcandidates, and the two charges aren't representing different things — they're the same bytes, charged twice.The double-count.
ingest_cost'smem_retainterm (around line 91-94) already bills, continuously, for holding retained completed windows in memory:candidate.n_windows * n * mem_bytes_per_instanceforTumbling.query_cost'sMergebranch (around line 120-126) then bills the samenum_windows * n * mem_bytes_per_instancequantity again, this time weighted byquery_memand scaled byf_a(query frequency) intotal_cost_rate. So every query incurs a second rent payment, on top of the rent already being paid continuously viaingest_cost. There's no real resource that justifies the second charge — if it were meant to represent something different (e.g. a transient compute buffer needed during the merge operation itself, rather than the retained set), it would be sized differently; instead it's sized identically to the retained set, which is the signature of double-billing the same bytes rather than billing a different, smaller thing.The same applies to
QueryMethod::Subtract's mem term (2.0 * n * mem_bytes_per_instance, around line 128-134) once the magnitude bug below is fixed.A second, related bug: wrong magnitude for
Subtract.ingest_cost'smem_retainalways usescandidate.n_windowsforTumbling, regardless ofquery_method— it has no visibility into which method was actually chosen. ButSubtractstructurally only ever needs 2 retained checkpoints (the current cumulative value and the checkpoint fromrange_aago), no matter how largen_windowsis — that's exactly whyquery_cost's ownSubtractbranch already hardcodes2.0, notnum_windows. So today,ingest_costoverbillsSubtractcandidates as if they needed fullMerge-style retention (e.g. 20 instances) when only 2 are ever needed. This is related to the retention-count mismatch in #412, but is a separate bug specific to the cost model'smem_retaincalculation.Proposed fix (one coherent change for both problems):
ingest_cost'smem_retainquery-method-aware:n_windowsforMerge,2forSubtract,0forDirect/Exact(currently it can't tell, since it only receivescandidate.n_windows, notcandidate.query_method).query_costentirely forMergeandSubtract, keeping only the CPU terms (merges * merge_cpu_secs/subtract_cpu_secs) — since the retained memory is already fully accounted for, once correctly, on the ingest side.Until fixed, cost comparisons between
Merge,Subtract,Sliding, andDirectcandidates are skewed —Merge/Subtractcandidates are penalized more than they should be relative toSliding/Direct, which don't have this double-billing.Found during code review of PR #407.