-
Type:
Task
-
Resolution: Unresolved
-
Priority:
Minor - P4
-
None
-
Affects Version/s: None
-
Component/s: None
-
Query Integration
-
None
-
None
-
None
-
None
-
None
-
None
-
None
Context
Copilot flagged the following on a PR review comment in WindowFunctionPercentileCommon::computePercentile:
Value WindowFunctionPercentileCommon::computePercentile(double p) const { const double n = _values.size(); ... if (_method != PercentileMethodEnum::kContinuous) { const double rank = DiscretePercentile::computeTrueRank(n, p); ...
n is derived from _values.size() (a size_t) but stored as double and passed to computeTrueRank(int, ...), requiring an implicit double -> int narrowing conversion. If the window ever contained more than INT_MAX elements, the conversion is UB.
Copilot's suggestion: store n as int (with a checked conversion) since the rank APIs already take int.
Impact Estimate
sroche noted in the PR thread:
* $setWindowFields default memory limit (internalDocumentSourceSetWindowFieldsMaxMemoryBytes): 100 MiB (see src/mongo/db/query/stage_memory_limit_knobs/stage_memory_limit_knobs.idl).bummer. I think technically the memory tracker/limits would cap out before we would reach this condition, but maybe it's worth fixing?
- INT_MAX ≈ 2.15 × 10^9.
- Value is at least ~16 bytes; a window holding INT_MAX Value s would consume >= ~34 GiB even ignoring container overhead — far above the 100 MiB default and above the SimpleMemoryUsageTracker that $setWindowFields plumbs through its exec classes.
At the default knob, the memory tracker should trip long before n approaches INT_MAX. However:
- The knob is settable at startup and runtime, so users can raise it arbitrarily.
- We should confirm this specific code path is actually behind that memory tracker (i.e., that _values growth is reported to the tracker before we call computePercentile). If it isn't, the "memory tracker saves us" argument doesn't apply.
Proposed fix (if we decide to)
Change n's type from double to int (or size_t with a checked narrowing conversion / tassert) so we don't rely on an implicit double -> int conversion. Match whatever computeTrueRank and computeTrueRank's continuous counterpart take.
Acceptance
- Confirm (or refute) the memory-tracker-saves-us claim by tracing whether _values contributes to the SimpleMemoryUsageTracker on the $percentile window function path.
- If confirmed and the fix is deemed unnecessary: close with a comment documenting the finding.
- If not confirmed, or if we still want defense-in-depth: apply the narrowing fix and add a tassert.