From f9b9c13aefd92c7227d858286a6fedff78bd8715 Mon Sep 17 00:00:00 2001 From: David Leong Date: Tue, 1 Sep 2026 04:45:45 +0000 Subject: [PATCH 1/5] fix(model): Close two chunking parity gaps with the Python reference Signed-off-by: David Leong --- .../openjd-model/src/job/step_param_space.rs | 338 ++++++++++++++++-- specs/model/parameter-space.md | 35 +- specs/model/public-api.md | 24 +- 3 files changed, 362 insertions(+), 35 deletions(-) diff --git a/crates/openjd-model/src/job/step_param_space.rs b/crates/openjd-model/src/job/step_param_space.rs index 09a65f08..df8aa38d 100644 --- a/crates/openjd-model/src/job/step_param_space.rs +++ b/crates/openjd-model/src/job/step_param_space.rs @@ -406,12 +406,56 @@ impl ContiguousChunkNode { } } +impl ContiguousChunkNode { + /// The chunk at `index`, without walking the chunks before it. + /// + /// Intervals still have to be walked, because which interval holds a given chunk is + /// only known from the chunk counts of the intervals before it. The chunk *within* + /// that interval is computed arithmetically, so one huge contiguous interval costs + /// O(1) instead of O(index) — which is what keeps a 100-billion-value range usable. + fn chunk_at(&self, index: usize) -> Option { + let mut state = ContiguousChunkIterState::new(self); + let mut remaining = index; + while state.start_next_interval() { + let cc = state.interval_chunk_count; + if remaining < cc { + let small = state.interval_small; + let lo = state.interval_leftovers; + // Number of size-(small+1) chunks before chunk `j`. `next_chunk` gives + // chunk j the extra value when ceil((j+1)*lo/cc) > ceil(j*lo/cc), so + // summing that over j telescopes to ceil(j*lo/cc). + let before = |j: usize| if lo == 0 { 0 } else { (j * lo).div_ceil(cc) }; + let offset = remaining * small + before(remaining); + let size = small + before(remaining + 1) - before(remaining); + let start = state.interval_start_val + offset as i64; + let end = start + size as i64 - 1; + return Some( + format!("{start}-{end}") + .parse::() + .expect("range string built from valid integers") + .with_contiguous(true), + ); + } + remaining -= cc; + } + None + } +} + impl Node for ContiguousChunkNode { fn len(&self) -> usize { self.num_chunks } - fn get(&self, _index: usize, _result: &mut TaskParameterSet) { - // Sequential-only; use iter() + fn get(&self, index: usize, result: &mut TaskParameterSet) { + if let Some(r) = self.chunk_at(index) { + result.insert( + self.name.clone(), + TaskParameterValue { + param_type: TaskParameterType::ChunkInt, + value: ExprValue::RangeExpr(r), + }, + ); + } } fn validate_containment(&self, params: &TaskParameterSet) -> Result<(), String> { let v = params.get(&self.name).ok_or_else(|| { @@ -1237,6 +1281,10 @@ pub struct StepParameterSpaceIterator { adaptive_chunk_size: Option>, node_iter: Option>, chunks_param_name: Option, + /// Effective chunk size for a non-adaptive chunked space. `None` when the space has + /// no chunked parameter. Adaptive spaces read `adaptive_chunk_size` instead, since + /// theirs is mutable. + chunk_size: Option, /// True when iteration must be sequential (adaptive or contiguous chunking). sequential: bool, } @@ -1293,12 +1341,28 @@ impl StepParameterSpaceIterator { adaptive_chunk_size: None, node_iter: None, chunks_param_name: None, + chunk_size: None, sequential: false, }); } let expr = space.combination.as_deref().unwrap_or("*"); + // The chunked parameter and its effective chunk size, independent of whether the + // chunking is adaptive. Reported for any CHUNK[INT] parameter, matching the Python + // reference: neither value is unknowable for a static space, since both are in the + // template. Adaptive additionally gets the mutable Arc below, because only an + // adaptive size can be changed part-way through iteration. + let mut chunks_param_name: Option = None; + let mut chunk_size: Option = None; + for (name, param) in &space.task_parameter_definitions { + if let job::TaskParameter::ChunkInt { chunks, .. } = param { + chunks_param_name = Some(name.clone()); + chunk_size = Some(chunk_override.unwrap_or(chunks.default_task_count).max(1)); + break; + } + } + // Check if any parameter needs adaptive chunking let mut adaptive_info: Option<(String, Arc)> = None; if chunk_override.is_none() { @@ -1345,12 +1409,12 @@ impl StepParameterSpaceIterator { }; let adaptive = adaptive_info.is_some(); - let chunks_param_name = adaptive_info.as_ref().map(|(n, _)| n.clone()); let adaptive_chunk_size = adaptive_info.map(|(_, rc)| rc); - // Use iterator path if any node requires sequential iteration - // (adaptive chunking or contiguous chunking with gaps) - let needs_sequential = adaptive || has_contiguous_chunks(space); + // Only adaptive chunking needs the sequential path now: its chunk size can change + // mid-iteration, so chunk N is not a function of N alone. Contiguous chunking is + // random-access via ContiguousChunkNode::chunk_at. + let needs_sequential = adaptive; let node_iter = if needs_sequential { Some(root.iter()) } else { @@ -1365,6 +1429,7 @@ impl StepParameterSpaceIterator { adaptive_chunk_size, node_iter, chunks_param_name, + chunk_size, sequential: needs_sequential, }) } @@ -1436,9 +1501,13 @@ impl StepParameterSpaceIterator { /// Current default_task_count for adaptive chunking. pub fn chunks_default_task_count(&self) -> Option { - self.adaptive_chunk_size - .as_ref() - .map(|a| a.load(Ordering::Relaxed)) + match &self.adaptive_chunk_size { + // Adaptive: read the live value, which `set_chunks_default_task_count` may + // have changed since construction. + Some(a) => Some(a.load(Ordering::Relaxed)), + // Non-adaptive: the template's size, or the override that replaced it. + None => self.chunk_size, + } } /// Update the chunk size for adaptive chunking. @@ -1693,17 +1762,6 @@ fn make_leaf_node( } } -/// Check if any chunk parameter uses contiguous constraint (requires sequential iteration). -fn has_contiguous_chunks(space: &job::StepParameterSpace) -> bool { - space.task_parameter_definitions.values().any(|p| { - matches!( - p, - job::TaskParameter::ChunkInt { chunks, .. } - if chunks.range_constraint == RangeConstraint::Contiguous - ) - }) -} - /// Build a chunk node from a range and chunk config. Creates `AdaptiveChunkNode` when /// `target_runtime_seconds > 0`, `ContiguousChunkNode` for contiguous static chunking, /// or `StaticChunkNode` for noncontiguous static chunking. @@ -2146,4 +2204,244 @@ mod tests { assert_eq!(set["X"].value, ExprValue::Int(i as i64 + 1)); } } + + // ── Chunk metadata for non-adaptive spaces ── + // The reference implementation reports the chunked parameter's name and chunk size + // for any CHUNK[INT] parameter. Both were previously derived from adaptive detection, + // so both went silent the moment a space was not adaptive — including when a chunk + // override made an adaptive space static. + + fn noncontiguous_static_chunk_param( + expr: &str, + default_task_count: usize, + ) -> job::TaskParameter { + job::TaskParameter::ChunkInt { + range: job::TaskParamRange::RangeExpr(expr.parse::().unwrap()), + chunks: job::ResolvedChunks { + default_task_count, + target_runtime_seconds: None, + range_constraint: RangeConstraint::Noncontiguous, + }, + } + } + + #[test] + fn test_static_chunk_metadata_is_reported() { + for param in [ + static_chunk_param("1-10", 5), + noncontiguous_static_chunk_param("1-10", 5), + ] { + let space = make_space(vec![("Frame", param)], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert!(!iter.chunks_adaptive()); + assert_eq!(iter.chunks_parameter_name(), Some("Frame")); + assert_eq!(iter.chunks_default_task_count(), Some(5)); + } + } + + #[test] + fn test_chunk_metadata_reports_the_override_not_the_template() { + let space = make_space(vec![("Frame", static_chunk_param("1-10", 5))], None); + let iter = StepParameterSpaceIterator::new_with_chunk_override(&space, Some(2)).unwrap(); + assert_eq!(iter.chunks_parameter_name(), Some("Frame")); + assert_eq!(iter.chunks_default_task_count(), Some(2)); + } + + #[test] + fn test_override_of_an_adaptive_space_reports_metadata_and_is_no_longer_adaptive() { + let space = make_space( + vec![("Frame", adaptive_chunk_param((1..=10).collect(), 5))], + None, + ); + let adaptive = StepParameterSpaceIterator::new(&space).unwrap(); + assert!(adaptive.chunks_adaptive()); + assert_eq!(adaptive.chunks_default_task_count(), Some(5)); + + // The override suppresses adaptive chunking, which previously also dropped both + // getters even though the caller had just supplied the size. + let overridden = + StepParameterSpaceIterator::new_with_chunk_override(&space, Some(1)).unwrap(); + assert!(!overridden.chunks_adaptive()); + assert_eq!(overridden.chunks_parameter_name(), Some("Frame")); + assert_eq!(overridden.chunks_default_task_count(), Some(1)); + } + + #[test] + fn test_no_chunk_metadata_without_a_chunked_parameter() { + let space = make_space(vec![("X", int_param(vec![1, 2, 3]))], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert_eq!(iter.chunks_parameter_name(), None); + assert_eq!(iter.chunks_default_task_count(), None); + } + + #[test] + fn test_adaptive_size_getter_still_tracks_the_live_value() { + // The mutable Arc must keep winning over the construction-time size, or the + // setter would appear to do nothing. + let space = make_space( + vec![("Frame", adaptive_chunk_param((1..=10).collect(), 5))], + None, + ); + let mut iter = StepParameterSpaceIterator::new(&space).unwrap(); + iter.set_chunks_default_task_count(3); + assert_eq!(iter.chunks_default_task_count(), Some(3)); + } + + // ── Random access for contiguous chunking ── + // Contiguous chunking used to force sequential iteration, so `get` and `__getitem__` + // declined for any contiguous chunked space while `len` still reported a count. + + /// The chunk string at `index`, via random access. + fn chunk_via_get(iter: &StepParameterSpaceIterator, index: usize) -> String { + match &iter.get(index).expect("index within bounds")["Frame"].value { + ExprValue::RangeExpr(r) => r.to_string(), + other => panic!("expected a RangeExpr chunk, got {other:?}"), + } + } + + /// Every chunk string, via iteration. + fn chunks_via_iteration(space: &job::StepParameterSpace) -> Vec { + StepParameterSpaceIterator::new(space) + .unwrap() + .map(|s| match &s["Frame"].value { + ExprValue::RangeExpr(r) => r.to_string(), + other => panic!("expected a RangeExpr chunk, got {other:?}"), + }) + .collect() + } + + #[test] + fn test_contiguous_random_access_agrees_with_iteration_and_the_reference() { + // Expected values are the pure-Python reference's output for the same shapes + // (`divide_int_list_into_contiguous_chunks`), so this pins the values themselves + // rather than only get-versus-iterate self-consistency. + // + // Uneven splits, exact splits, single-chunk, chunk-per-value, and ranges with + // gaps and steps, since each drives a different branch of the interval walk. + let cases: Vec<(&str, usize, Vec<&str>)> = vec![ + ("1-10", 3, vec!["1-3", "4-5", "6-8", "9-10"]), + ("1-10", 5, vec!["1-5", "6-10"]), + ("1-12", 5, vec!["1-4", "5-8", "9-12"]), + ( + "1-10", + 1, + vec![ + "1-1", "2-2", "3-3", "4-4", "5-5", "6-6", "7-7", "8-8", "9-9", "10-10", + ], + ), + ("1-10", 20, vec!["1-10"]), + ("1-7", 2, vec!["1-2", "3-4", "5-6", "7-7"]), + ("1-5,8-12", 3, vec!["1-3", "4-5", "8-10", "11-12"]), + ( + "1-5,8-12", + 2, + vec!["1-2", "3-4", "5-5", "8-9", "10-11", "12-12"], + ), + ( + "1-20:2", + 3, + vec![ + "1-1", "3-3", "5-5", "7-7", "9-9", "11-11", "13-13", "15-15", "17-17", "19-19", + ], + ), + ( + "1-3,7,11-15", + 2, + vec!["1-2", "3-3", "7-7", "11-12", "13-14", "15-15"], + ), + ]; + for (expr, dtc, reference) in cases { + let space = make_space(vec![("Frame", static_chunk_param(expr, dtc))], None); + + // Iteration matches the reference. + assert_eq!( + chunks_via_iteration(&space), + reference, + "iteration disagrees with the reference for {expr} dtc={dtc}" + ); + + // Random access matches it too, index for index. + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert_eq!( + iter.len(), + reference.len(), + "len disagrees with the reference for {expr} dtc={dtc}" + ); + let by_index: Vec = (0..iter.len()).map(|i| chunk_via_get(&iter, i)).collect(); + assert_eq!( + by_index, reference, + "get disagrees with the reference for {expr} dtc={dtc}" + ); + } + } + + #[test] + fn test_contiguous_random_access_is_lazy_on_a_huge_range() { + // One interval of 100 billion values. Indexing near the end must not walk the + // chunks before it, which is the whole reason the chunk is computed + // arithmetically rather than by advancing an iterator. + let space = make_space(vec![("Frame", static_chunk_param(HUGE_RANGE, 1000))], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert_eq!(iter.len(), 100_000_000); + assert_eq!(chunk_via_get(&iter, 0), "1-1000"); + assert_eq!(chunk_via_get(&iter, 1), "1001-2000"); + assert_eq!(chunk_via_get(&iter, 99_999_999), "99999999001-100000000000"); + } + + #[test] + fn test_contiguous_random_access_out_of_range_is_none() { + let space = make_space(vec![("Frame", static_chunk_param("1-10", 5))], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert_eq!(iter.len(), 2); + assert!(iter.get(2).is_none()); + assert!(iter.get(1000).is_none()); + } + + #[test] + fn test_contiguous_chunking_is_no_longer_sequential() { + // The flag is what gates `get`, and it is also what makes `Iterator::next` take + // the random-access path — so this pins that contiguous iteration now runs + // through `chunk_at` rather than the node iterator. + let space = make_space(vec![("Frame", static_chunk_param("1-10", 3))], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert!(!iter.sequential); + assert!(iter.node_iter.is_none()); + } + + #[test] + fn test_contiguous_random_access_within_a_product() { + // ProductNode::get divides the index across children, so a chunked child has to + // answer for an index that is not the outer index. + let space = make_space( + vec![ + ("A", int_param(vec![1, 2])), + ("Frame", static_chunk_param("1-10", 5)), + ], + None, + ); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert_eq!(iter.len(), 4); // 2 values x 2 chunks + let expected: Vec<(i64, String)> = StepParameterSpaceIterator::new(&space) + .unwrap() + .map(|s| { + let a = match &s["A"].value { + ExprValue::Int(v) => *v, + other => panic!("expected Int, got {other:?}"), + }; + let f = match &s["Frame"].value { + ExprValue::RangeExpr(r) => r.to_string(), + other => panic!("expected RangeExpr, got {other:?}"), + }; + (a, f) + }) + .collect(); + for (i, want) in expected.iter().enumerate() { + let set = iter.get(i).unwrap(); + let a = match &set["A"].value { + ExprValue::Int(v) => *v, + other => panic!("expected Int, got {other:?}"), + }; + assert_eq!(&(a, chunk_via_get(&iter, i)), want, "product index {i}"); + } + } } diff --git a/specs/model/parameter-space.md b/specs/model/parameter-space.md index e3b17511..977f4318 100644 --- a/specs/model/parameter-space.md +++ b/specs/model/parameter-space.md @@ -29,9 +29,15 @@ impl Iterator for StepParameterSpaceIterator { `new` returns `Result` because construction can fail (e.g., if the product of parameter dimensions overflows `usize`). `get` returns `Option` -(returns `None` for out-of-bounds indices or when sequential iteration is required). +(returns `None` for out-of-bounds indices, or for an adaptive space, where chunk +boundaries are not a function of the index alone). `set_chunks_default_task_count` takes `&mut self` (no `Arc` indirection). +`chunks_parameter_name` and `chunks_default_task_count` report for *any* chunked space, +adaptive or not: both values are in the template, and a chunk override replaces the size. +Only `set_chunks_default_task_count` is adaptive-specific, because only an adaptive size +is mutable mid-walk. + `new_with_chunk_override` accepts an optional chunk size that overrides the template's `defaultTaskCount` for all chunk nodes. When `Some(n)`, it also suppresses adaptive chunking (the parameter is treated as static with chunk size `n`). @@ -39,7 +45,7 @@ chunking (the parameter is treated as static with chunk size `n`). `reset` rewinds the iterator so a fresh `Iterator::next` walk yields the same elements again, without rebuilding from the parameter space. For non-sequential (random-access) iterators it sets the internal `current_index` to 0; for sequential iterators (adaptive -chunking, contiguous chunks with gaps) it delegates to the inner node iterator's reset. +chunking) it delegates to the inner node iterator's reset. The adaptive `Arc` chunk-size override set via `set_chunks_default_task_count` is preserved across `reset` calls — `reset` does not restore the template's original `defaultTaskCount`. @@ -146,9 +152,17 @@ counting is instant. `chunk_count = ceil(interval_len / default_task_count)`, then `small = interval_len / chunk_count` with leftovers distributed evenly across chunks. -`ContiguousChunkNode` is sequential-only (no random access via `get()`) because chunk -boundaries depend on scanning for gaps from the beginning. The exact chunk count is -cached at construction time. +`ContiguousChunkNode` supports random access through `chunk_at(index)`. Finding which +interval holds a given chunk still means walking the intervals, since that is only known +from the chunk counts of the intervals before it — O(R) in sub-ranges, not O(N) in values, +reusing the same walk as the count. The chunk *within* that interval is then computed +arithmetically, so a single large contiguous interval costs O(1) rather than O(index). +The exact chunk count is cached at construction time. + +Within an interval, chunk `j` gets one extra value when +`ceil((j+1)*leftovers/chunk_count) > ceil(j*leftovers/chunk_count)`, so the number of +larger chunks before `j` telescopes to `ceil(j*leftovers/chunk_count)`. That is what lets +the offset and size of chunk `j` be computed without walking the chunks before it. ### Noncontiguous Static Chunking @@ -184,10 +198,13 @@ chunking is suppressed (the parameter uses static chunking with the override siz ### Sequential Iteration -When any node in the tree requires sequential iteration (adaptive chunking or contiguous -chunking), the `StepParameterSpaceIterator` uses the `node_iter` path instead of -random-access `get()`. The `sequential` flag tracks this. `len()` still returns the -exact count (except for adaptive, which returns 0). +Only adaptive chunking requires sequential iteration: its chunk size can change part-way +through a walk, so chunk N is not a function of N alone. For an adaptive space the +`StepParameterSpaceIterator` uses the `node_iter` path instead of random-access `get()`, +and the `sequential` flag tracks this. Every other space, contiguous chunking included, +takes the random-access path — which means `Iterator::next` is itself implemented as +`get(current_index)` there. `len()` returns the exact count except for adaptive, which +returns 0. ## Design Decisions diff --git a/specs/model/public-api.md b/specs/model/public-api.md index 0547208d..da67a8ef 100644 --- a/specs/model/public-api.md +++ b/specs/model/public-api.md @@ -1077,17 +1077,25 @@ impl StepParameterSpaceIterator { pub fn len(&self) -> usize; pub fn is_empty(&self) -> bool; - /// Random access. Returns `None` for out-of-bounds and for - /// sequential-only spaces (adaptive chunking, contiguous chunking). + /// Random access. Returns `None` for out-of-bounds and for adaptive + /// chunking, the one case where chunk N is not a function of N. pub fn get(&self, index: usize) -> Option; pub fn contains(&self, params: &TaskParameterSet) -> bool; pub fn validate_containment(&self, params: &TaskParameterSet) -> Result<(), String>; - /// Adaptive chunking (TASK_CHUNKING with `targetRuntimeSeconds`). + /// True only for adaptive chunking (TASK_CHUNKING with + /// `targetRuntimeSeconds`). pub fn chunks_adaptive(&self) -> bool; + + /// The chunked parameter and its chunk size, for any chunked space — + /// adaptive or static, and reflecting a chunk override when one was + /// given. `None` when the space has no chunked parameter. pub fn chunks_parameter_name(&self) -> Option<&str>; pub fn chunks_default_task_count(&self) -> Option; + + /// Adaptive-only: a static chunk size cannot change mid-walk, so this + /// is a no-op for a non-adaptive space. pub fn set_chunks_default_task_count(&mut self, value: usize); /// Rewind the iterator so a fresh `Iterator::next` walk yields the @@ -1106,9 +1114,13 @@ impl Iterator for StepParameterSpaceIterator { Random-access indexing uses `O(1)` arithmetic on a product-of-factors representation — submitters that want to shard a large parameter space across workers can compute per-worker index slices without -iterating the whole space. Adaptive chunking (TASK_CHUNKING §4 / -RFC 0001) forces sequential iteration because chunk size depends on -runtime feedback; for that case, callers mutate +iterating the whole space. Contiguous chunking costs an additional +walk over the range's sub-ranges to locate the chunk's interval, which +is O(R) in sub-ranges rather than O(N) in values. + +Adaptive chunking (TASK_CHUNKING §4 / RFC 0001) is the one case that +forces sequential iteration, because chunk size depends on runtime +feedback; for that case, callers mutate `set_chunks_default_task_count` while iterating to reshape chunks dynamically. From 0252f59ca3e27ba2230e7674637a9325ee89643d Mon Sep 17 00:00:00 2001 From: David Leong Date: Tue, 1 Sep 2026 04:59:59 +0000 Subject: [PATCH 2/5] fix(model): Keep contiguous iteration sequential and stop cloning the range Signed-off-by: David Leong --- .../openjd-model/src/job/step_param_space.rs | 219 ++++++++++++------ specs/model/parameter-space.md | 40 ++-- specs/model/public-api.md | 7 +- 3 files changed, 177 insertions(+), 89 deletions(-) diff --git a/crates/openjd-model/src/job/step_param_space.rs b/crates/openjd-model/src/job/step_param_space.rs index df8aa38d..b8fed8c9 100644 --- a/crates/openjd-model/src/job/step_param_space.rs +++ b/crates/openjd-model/src/job/step_param_space.rs @@ -414,20 +414,24 @@ impl ContiguousChunkNode { /// that interval is computed arithmetically, so one huge contiguous interval costs /// O(1) instead of O(index) — which is what keeps a 100-billion-value range usable. fn chunk_at(&self, index: usize) -> Option { - let mut state = ContiguousChunkIterState::new(self); + // Borrows the range rather than cloning it: this runs per random access, and a + // `List` range would otherwise copy every value on each call. + let mut cursor = 0usize; let mut remaining = index; - while state.start_next_interval() { - let cc = state.interval_chunk_count; + while cursor < self.total_len { + let first = range_value_at(&self.range, cursor); + let end_idx = interval_end_index(&self.range, cursor); + let last = range_value_at(&self.range, end_idx); + let interval_len = (last - first + 1) as usize; + cursor = end_idx + 1; + + let (cc, small, lo) = interval_chunking(interval_len, self.default_task_count); if remaining < cc { - let small = state.interval_small; - let lo = state.interval_leftovers; - // Number of size-(small+1) chunks before chunk `j`. `next_chunk` gives - // chunk j the extra value when ceil((j+1)*lo/cc) > ceil(j*lo/cc), so - // summing that over j telescopes to ceil(j*lo/cc). - let before = |j: usize| if lo == 0 { 0 } else { (j * lo).div_ceil(cc) }; - let offset = remaining * small + before(remaining); - let size = small + before(remaining + 1) - before(remaining); - let start = state.interval_start_val + offset as i64; + let offset = remaining * small + larger_chunks_before(remaining, lo, cc); + let size = small + + (larger_chunks_before(remaining + 1, lo, cc) + - larger_chunks_before(remaining, lo, cc)); + let start = first + offset as i64; let end = start + size as i64 - 1; return Some( format!("{start}-{end}") @@ -528,20 +532,64 @@ impl ContiguousChunkIterState { } fn get_value(&self, i: usize) -> i64 { - match &self.range { - job::TaskParamRange::List(v) => v[i], - // i is always bounded by the range length via cursor/total_len checks in callers. - job::TaskParamRange::RangeExpr(r) => { - r.get(i as i64).expect("index within range bounds") - } - } + range_value_at(&self.range, i) } - /// Find the last index of the contiguous interval starting at `start`. - /// For `RangeExpr`, uses sub-range structure to skip step-1 ranges in O(R). - /// For `List`, scans values in O(interval_len). fn find_interval_end(&self, start: usize) -> usize { - match &self.range { + interval_end_index(&self.range, start) + } +} + +/// Value at index `i` of a task parameter range. +/// +/// `i` must be within the range length; every caller bounds it by `total_len` first. +fn range_value_at(range: &job::TaskParamRange, i: usize) -> i64 { + match range { + job::TaskParamRange::List(v) => v[i], + job::TaskParamRange::RangeExpr(r) => r.get(i as i64).expect("index within range bounds"), + } +} + +/// Chunk count and even-distribution parameters for one contiguous interval, as +/// `(chunk_count, small, leftovers)`. Shared so sequential iteration and random access +/// cannot drift apart on how an interval is split. +fn interval_chunking(interval_len: usize, default_task_count: usize) -> (usize, usize, usize) { + let chunk_count = interval_len.div_ceil(default_task_count); + if chunk_count >= interval_len { + (chunk_count, 1, 0) + } else if chunk_count <= 1 { + (chunk_count, interval_len, 0) + } else { + ( + chunk_count, + interval_len / chunk_count, + interval_len % chunk_count, + ) + } +} + +/// Number of size-`small + 1` chunks before chunk `j` of an interval. +/// +/// A chunk takes one extra value when `ceil((j+1)*leftovers/chunk_count)` exceeds +/// `ceil(j*leftovers/chunk_count)`, so summing that over the chunks before `j` +/// telescopes to the single ceiling below. That is what lets a chunk's offset and size be +/// computed without walking the chunks before it. +fn larger_chunks_before(j: usize, leftovers: usize, chunk_count: usize) -> usize { + if leftovers == 0 { + 0 + } else { + (j * leftovers).div_ceil(chunk_count) + } +} + +/// Find the last index of the contiguous interval starting at `start`. +/// +/// For `RangeExpr`, uses sub-range structure to skip step-1 ranges in O(R). For `List`, +/// scans values in O(interval_len). Note a step > 1 sub-range yields one interval per +/// value, so a stepped range has as many intervals as values. +fn interval_end_index(range: &job::TaskParamRange, start: usize) -> usize { + { + match range { job::TaskParamRange::List(v) => { let mut end = start; while end + 1 < v.len() && v[end + 1] == v[end] + 1 { @@ -594,7 +642,9 @@ impl ContiguousChunkIterState { } } } +} +impl ContiguousChunkIterState { /// Advance cursor to find the next contiguous interval and set up chunking state. fn start_next_interval(&mut self) -> bool { if self.cursor >= self.total_len { @@ -608,15 +658,8 @@ impl ContiguousChunkIterState { let interval_len = (last - first + 1) as usize; self.cursor = end_idx + 1; - // Compute even chunk distribution for this interval - let chunk_count = interval_len.div_ceil(self.default_task_count); - let (small, leftovers) = if chunk_count >= interval_len { - (1, 0) - } else if chunk_count <= 1 { - (interval_len, 0) - } else { - (interval_len / chunk_count, interval_len % chunk_count) - }; + let (chunk_count, small, leftovers) = + interval_chunking(interval_len, self.default_task_count); self.interval_start_val = first; self.interval_pos = first; @@ -636,36 +679,13 @@ impl ContiguousChunkIterState { } } - // Compute chunk size using Python's even distribution: - // chunk_sizes[(i * chunk_count) // leftovers] += 1 - let mut size = self.interval_small; - if self.interval_leftovers > 0 - && (self.interval_chunk_index * self.interval_chunk_count) / self.interval_leftovers - != ((self.interval_chunk_index + 1) * self.interval_chunk_count) - / self.interval_leftovers - { - // This is a simpler equivalent: check if this index gets a +1 - // by testing if floor((i+1)*count/left) > floor(i*count/left) - } - // Actually, replicate the Python algorithm directly: - // chunk_sizes = [small] * chunk_count - // for i in range(leftovers): chunk_sizes[(i * chunk_count) // leftovers] += 1 - // Check if current chunk_index is one of the +1 slots - if self.interval_leftovers > 0 { - let idx = self.interval_chunk_index; - let cc = self.interval_chunk_count; - let lo = self.interval_leftovers; - // The +1 slots are at indices: (i * cc) // lo for i in 0..lo - // Equivalently, idx gets +1 if there exists i such that (i * cc) / lo == idx - // which means: idx * lo <= i * cc < (idx + 1) * lo - // i.e., ceil(idx * lo / cc) <= i < ceil((idx+1) * lo / cc) - // If that range is non-empty, this index gets +1 - let i_start = (idx * lo).div_ceil(cc); - let i_end = ((idx + 1) * lo).div_ceil(cc); - if i_start < i_end && i_start < lo { - size += 1; - } - } + // Even distribution, matching the reference's + // `chunk_sizes[(i * chunk_count) // leftovers] += 1`. + let idx = self.interval_chunk_index; + let cc = self.interval_chunk_count; + let lo = self.interval_leftovers; + let size = self.interval_small + + (larger_chunks_before(idx + 1, lo, cc) - larger_chunks_before(idx, lo, cc)); let start = self.interval_pos; let end = start + size as i64 - 1; @@ -1411,10 +1431,13 @@ impl StepParameterSpaceIterator { let adaptive = adaptive_info.is_some(); let adaptive_chunk_size = adaptive_info.map(|(_, rc)| rc); - // Only adaptive chunking needs the sequential path now: its chunk size can change - // mid-iteration, so chunk N is not a function of N alone. Contiguous chunking is - // random-access via ContiguousChunkNode::chunk_at. - let needs_sequential = adaptive; + // Which path `Iterator::next` takes. Contiguous chunking stays sequential here + // even though it now supports random access: locating a chunk means walking the + // intervals before it, so driving a full walk through `get` would be + // O(chunks x intervals) — quadratic for a range whose values are mostly isolated, + // where every value is its own interval. Random access is gated separately, on + // `adaptive` alone. + let needs_sequential = adaptive || has_contiguous_chunks(space); let node_iter = if needs_sequential { Some(root.iter()) } else { @@ -1455,9 +1478,13 @@ impl StepParameterSpaceIterator { } /// Random access to a specific task parameter set by index. - /// Returns `None` for out-of-bounds or when sequential iteration is required. + /// + /// Returns `None` for out-of-bounds, and for adaptive chunking, where the chunk at a + /// given index is not a function of the index alone. Gated on `adaptive` rather than + /// `sequential`: contiguous chunking prefers sequential *iteration* for cost reasons + /// but can still answer for a single index. pub fn get(&self, index: usize) -> Option { - if self.sequential { + if self.adaptive { return None; } if index >= self.root.len() { @@ -1762,6 +1789,21 @@ fn make_leaf_node( } } +/// Whether any chunk parameter uses the contiguous constraint. +/// +/// Such a space prefers sequential iteration: `ContiguousChunkNode` can answer a single +/// index, but only by walking the intervals before it, so a full walk driven through +/// `get` would be quadratic where values are mostly isolated. +fn has_contiguous_chunks(space: &job::StepParameterSpace) -> bool { + space.task_parameter_definitions.values().any(|p| { + matches!( + p, + job::TaskParameter::ChunkInt { chunks, .. } + if chunks.range_constraint == RangeConstraint::Contiguous + ) + }) +} + /// Build a chunk node from a range and chunk config. Creates `AdaptiveChunkNode` when /// `target_runtime_seconds > 0`, `ContiguousChunkNode` for contiguous static chunking, /// or `StaticChunkNode` for noncontiguous static chunking. @@ -2398,14 +2440,43 @@ mod tests { } #[test] - fn test_contiguous_chunking_is_no_longer_sequential() { - // The flag is what gates `get`, and it is also what makes `Iterator::next` take - // the random-access path — so this pins that contiguous iteration now runs - // through `chunk_at` rather than the node iterator. + fn test_contiguous_chunking_keeps_sequential_iteration_but_gains_random_access() { + // The two concerns are deliberately decoupled. Iteration stays on the node + // iterator, because driving a full walk through `get` would re-walk the intervals + // for every index. Random access is gated on `adaptive` alone, so `get` answers. let space = make_space(vec![("Frame", static_chunk_param("1-10", 3))], None); let iter = StepParameterSpaceIterator::new(&space).unwrap(); - assert!(!iter.sequential); - assert!(iter.node_iter.is_none()); + assert!( + iter.sequential, + "iteration should stay on the node iterator" + ); + assert!(iter.node_iter.is_some()); + assert!(!iter.adaptive); + assert!(iter.get(0).is_some(), "random access should still work"); + } + + #[test] + fn test_many_interval_iteration_stays_linear() { + // A stepped range makes every value its own interval, which is the shape that + // would go quadratic if `Iterator::next` were routed through `chunk_at`. 20k + // values would be ~4e8 interval walks; this completes promptly because iteration + // uses the sequential path. + let space = make_space(vec![("Frame", static_chunk_param("1-40000:2", 1))], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + assert_eq!(iter.len(), 20_000); + let chunks: Vec = StepParameterSpaceIterator::new(&space) + .unwrap() + .map(|s| match &s["Frame"].value { + ExprValue::RangeExpr(r) => r.to_string(), + other => panic!("expected a RangeExpr chunk, got {other:?}"), + }) + .collect(); + assert_eq!(chunks.len(), 20_000); + assert_eq!(chunks[0], "1-1"); + assert_eq!(chunks[19_999], "39999-39999"); + // Random access agrees at both ends of the same space. + assert_eq!(chunk_via_get(&iter, 0), "1-1"); + assert_eq!(chunk_via_get(&iter, 19_999), "39999-39999"); } #[test] diff --git a/specs/model/parameter-space.md b/specs/model/parameter-space.md index 977f4318..be1c9cf2 100644 --- a/specs/model/parameter-space.md +++ b/specs/model/parameter-space.md @@ -152,12 +152,19 @@ counting is instant. `chunk_count = ceil(interval_len / default_task_count)`, then `small = interval_len / chunk_count` with leftovers distributed evenly across chunks. -`ContiguousChunkNode` supports random access through `chunk_at(index)`. Finding which -interval holds a given chunk still means walking the intervals, since that is only known -from the chunk counts of the intervals before it — O(R) in sub-ranges, not O(N) in values, -reusing the same walk as the count. The chunk *within* that interval is then computed -arithmetically, so a single large contiguous interval costs O(1) rather than O(index). -The exact chunk count is cached at construction time. +`ContiguousChunkNode` supports random access through `chunk_at(index)`, and the exact +chunk count is cached at construction time. + +Finding which interval holds a given chunk means walking the intervals, since that is only +known from the chunk counts of the intervals before it. For step-1 sub-ranges that is O(R) +in sub-ranges rather than O(N) in values, but a sub-range with step > 1 contributes one +interval *per value*, so a stepped or heavily-gapped range is O(N). The chunk *within* the +located interval is then computed arithmetically, so one large contiguous interval costs +O(1) rather than O(index). + +Because of that walk, contiguous chunking still uses the sequential path for +`Iterator::next` — see "Sequential Iteration" below. Random access is for callers that +genuinely want one index, not a way to drive a full walk. Within an interval, chunk `j` gets one extra value when `ceil((j+1)*leftovers/chunk_count) > ceil(j*leftovers/chunk_count)`, so the number of @@ -198,13 +205,20 @@ chunking is suppressed (the parameter uses static chunking with the override siz ### Sequential Iteration -Only adaptive chunking requires sequential iteration: its chunk size can change part-way -through a walk, so chunk N is not a function of N alone. For an adaptive space the -`StepParameterSpaceIterator` uses the `node_iter` path instead of random-access `get()`, -and the `sequential` flag tracks this. Every other space, contiguous chunking included, -takes the random-access path — which means `Iterator::next` is itself implemented as -`get(current_index)` there. `len()` returns the exact count except for adaptive, which -returns 0. +Two separate questions, tracked by two different flags. + +**Which path `Iterator::next` takes** is the `sequential` flag: adaptive chunking or +contiguous chunking. Adaptive must be sequential because its chunk size can change +part-way through a walk. Contiguous *could* answer per index, but locating a chunk walks +the intervals before it, so driving a full walk through `get()` would be +O(chunks x intervals) — quadratic for a range whose values are mostly isolated. Both +therefore use the `node_iter` path. Every other space takes the random-access path, where +`Iterator::next` is implemented as `get(current_index)`. + +**Whether `get()` answers at all** is gated on adaptive alone. A contiguous space supports +random access; an adaptive one does not, because chunk N is not a function of N. + +`len()` returns the exact count except for adaptive, which returns 0. ## Design Decisions diff --git a/specs/model/public-api.md b/specs/model/public-api.md index da67a8ef..b18e1000 100644 --- a/specs/model/public-api.md +++ b/specs/model/public-api.md @@ -1079,6 +1079,8 @@ impl StepParameterSpaceIterator { /// Random access. Returns `None` for out-of-bounds and for adaptive /// chunking, the one case where chunk N is not a function of N. + /// A contiguous chunked space answers, though `Iterator::next` still + /// walks it sequentially — see parameter-space.md. pub fn get(&self, index: usize) -> Option; pub fn contains(&self, params: &TaskParameterSet) -> bool; @@ -1115,8 +1117,9 @@ Random-access indexing uses `O(1)` arithmetic on a product-of-factors representation — submitters that want to shard a large parameter space across workers can compute per-worker index slices without iterating the whole space. Contiguous chunking costs an additional -walk over the range's sub-ranges to locate the chunk's interval, which -is O(R) in sub-ranges rather than O(N) in values. +walk over the range's intervals to locate the chunk's — O(R) in +sub-ranges for step-1 sub-ranges, but O(N) where a range is stepped or +heavily gapped, since each such value is its own interval. Adaptive chunking (TASK_CHUNKING §4 / RFC 0001) is the one case that forces sequential iteration, because chunk size depends on runtime From 18637e74665bfad15094c71953612e8ac50a3505 Mon Sep 17 00:00:00 2001 From: David Leong Date: Tue, 1 Sep 2026 05:10:37 +0000 Subject: [PATCH 3/5] fix(model): Widen chunk offset math and merge stepped runs into adjacent ones Signed-off-by: David Leong --- .../openjd-model/src/job/step_param_space.rs | 170 ++++++++++++++---- 1 file changed, 137 insertions(+), 33 deletions(-) diff --git a/crates/openjd-model/src/job/step_param_space.rs b/crates/openjd-model/src/job/step_param_space.rs index b8fed8c9..d9063aaf 100644 --- a/crates/openjd-model/src/job/step_param_space.rs +++ b/crates/openjd-model/src/job/step_param_space.rs @@ -576,10 +576,15 @@ fn interval_chunking(interval_len: usize, default_task_count: usize) -> (usize, /// computed without walking the chunks before it. fn larger_chunks_before(j: usize, leftovers: usize, chunk_count: usize) -> usize { if leftovers == 0 { - 0 - } else { - (j * leftovers).div_ceil(chunk_count) + return 0; } + // Widened to u128 for the product only. `j` is bounded by `chunk_count` and + // `leftovers` is `interval_len % chunk_count`, so `j * leftovers` approaches + // `(interval_len/2)^2` and overflows usize once an interval passes roughly 8.6e9 + // values — well inside the range sizes this module is built to handle. The quotient + // is bounded by `leftovers`, so narrowing back is lossless. + let product = (j as u128) * (leftovers as u128); + (product.div_ceil(chunk_count as u128)) as usize } /// Find the last index of the contiguous interval starting at `start`. @@ -616,16 +621,23 @@ fn interval_end_index(range: &job::TaskParamRange, start: usize) -> usize { let sr = &sub_ranges[sr_idx]; - if sr.step() != 1 { - // Step > 1: each value is isolated - return start; - } - - // Current sub-range is step-1: interval extends to end of this sub-range - let mut end = sr_offset + sr.len() - 1; - - // Check subsequent sub-ranges for adjacency - let mut last_val = sr.end(); + // Where this interval ends within the current sub-range, and the value it + // ends on. A step > 1 sub-range has a gap between each of its own values, + // so the interval is that single value — but if it is the sub-range's + // *last* value it can still merge into an adjacent following sub-range, + // which is what `count_contiguous_chunks_from_sub_ranges` does. Returning + // early here instead left iteration and `len()` disagreeing: `1-5:2,6-10` + // at defaultTaskCount 2 counted 5 chunks but yielded 6. + let (mut end, mut last_val) = if sr.step() != 1 { + let is_last_of_sub_range = start == sr_offset + sr.len() - 1; + if !is_last_of_sub_range { + return start; + } + (start, range_value_at(range, start)) + } else { + // Step-1: the interval extends to the end of this sub-range. + (sr_offset + sr.len() - 1, sr.end()) + }; for next_sr in &sub_ranges[sr_idx + 1..] { if next_sr.start() == last_val + 1 && next_sr.step() == 1 { end += next_sr.len(); @@ -1368,32 +1380,33 @@ impl StepParameterSpaceIterator { let expr = space.combination.as_deref().unwrap_or("*"); - // The chunked parameter and its effective chunk size, independent of whether the - // chunking is adaptive. Reported for any CHUNK[INT] parameter, matching the Python - // reference: neither value is unknowable for a static space, since both are in the - // template. Adaptive additionally gets the mutable Arc below, because only an - // adaptive size can be changed part-way through iteration. + // The chunked parameter, its effective chunk size, and whether it is adaptive, all + // taken from the *same* parameter: the first CHUNK[INT] in definition order, which + // is what the Python reference reports too. + // + // Deriving them separately would let `chunks_parameter_name` name one parameter + // while `chunks_adaptive` described another. Template validation rejects more than + // one CHUNK[INT] per step, but `StepParameterSpace` is publicly constructible and + // deserializable — the same reason this function re-validates the value bound + // above — and openjd-cli feeds `chunks_parameter_name` back into adaptive + // chunk-size adjustment, so a mismatch would corrupt that feedback. + // + // Name and size are reported for any chunked space; only adaptive gets the mutable + // Arc, because only an adaptive size can change part-way through iteration. let mut chunks_param_name: Option = None; let mut chunk_size: Option = None; + let mut adaptive_info: Option<(String, Arc)> = None; for (name, param) in &space.task_parameter_definitions { if let job::TaskParameter::ChunkInt { chunks, .. } = param { + let effective = chunk_override.unwrap_or(chunks.default_task_count).max(1); chunks_param_name = Some(name.clone()); - chunk_size = Some(chunk_override.unwrap_or(chunks.default_task_count).max(1)); - break; - } - } - - // Check if any parameter needs adaptive chunking - let mut adaptive_info: Option<(String, Arc)> = None; - if chunk_override.is_none() { - for (name, param) in &space.task_parameter_definitions { - if let job::TaskParameter::ChunkInt { chunks, .. } = param { - if chunks.target_runtime_seconds.is_some_and(|t| t > 0) { - let arc = Arc::new(AtomicUsize::new(chunks.default_task_count.max(1))); - adaptive_info = Some((name.clone(), arc)); - break; - } + chunk_size = Some(effective); + // An override pins the size, which rules out adaptive. + if chunk_override.is_none() && chunks.target_runtime_seconds.is_some_and(|t| t > 0) + { + adaptive_info = Some((name.clone(), Arc::new(AtomicUsize::new(effective)))); } + break; } } @@ -2391,6 +2404,19 @@ mod tests { 2, vec!["1-2", "3-3", "7-7", "11-12", "13-14", "15-15"], ), + // A step > 1 sub-range followed by an adjacent step-1 one: the last value of + // the stepped sub-range merges into the run that follows it, so `5` and + // `6-10` form one interval of six values. + ("1-5:2,6-10", 2, vec!["1-1", "3-3", "5-6", "7-8", "9-10"]), + ("1-5:2,6-10", 3, vec!["1-1", "3-3", "5-7", "8-10"]), + ("1-3:2,4-6", 2, vec!["1-1", "3-4", "5-6"]), + ( + "1-9:2,10-20", + 2, + vec![ + "1-1", "3-3", "5-5", "7-7", "9-10", "11-12", "13-14", "15-16", "17-18", "19-20", + ], + ), ]; for (expr, dtc, reference) in cases { let space = make_space(vec![("Frame", static_chunk_param(expr, dtc))], None); @@ -2430,6 +2456,84 @@ mod tests { assert_eq!(chunk_via_get(&iter, 99_999_999), "99999999001-100000000000"); } + #[test] + fn test_chunk_offsets_do_not_overflow_on_a_huge_range() { + // `j * leftovers` approaches (interval_len/2)^2, which passes usize::MAX once an + // interval is over roughly 8.6e9 values. A default_task_count that divides the + // range evenly leaves `leftovers == 0` and never multiplies, so the arithmetic has + // to be exercised with one that leaves a remainder. + let space = make_space(vec![("Frame", static_chunk_param(HUGE_RANGE, 3))], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + let n = iter.len(); + assert_eq!(n, 33_333_333_334); + + // Both ends, plus the last index, where the product is largest. + assert_eq!(chunk_via_get(&iter, 0), "1-3"); + assert_eq!(chunk_via_get(&iter, n - 1), "99999999999-100000000000"); + + // Chunks must tile the range without gaps or overlaps. Spot-check adjacency + // deep into the space, where a wrapped product would show up as a wild offset. + for i in [1usize, 2, 1_000_000, 30_000_000_000, n - 2] { + let a = chunk_via_get(&iter, i); + let b = chunk_via_get(&iter, i + 1); + let a_end: i64 = a.split('-').next_back().unwrap().parse().unwrap(); + let b_start: i64 = b.split('-').next().unwrap().parse().unwrap(); + assert_eq!(b_start, a_end + 1, "chunks {i} and {} do not tile", i + 1); + } + } + + #[test] + fn test_len_agrees_with_iteration_across_mixed_step_ranges() { + // `len()` comes from `count_contiguous_chunks_for_range` while iteration walks + // intervals via `interval_end_index`. The two used to disagree whenever an + // interval began in a step > 1 sub-range and continued into an adjacent step-1 + // one, which made `len()` under-report and left the last chunk unreachable. + for (expr, dtc) in [ + ("1-5:2,6-10", 2), + ("1-5:2,6-10", 3), + ("1-3:2,4-6", 2), + ("1-9:2,10-20", 2), + ("1-20:2", 3), + ("1-5,8-12", 3), + ] { + let space = make_space(vec![("Frame", static_chunk_param(expr, dtc))], None); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + let walked = chunks_via_iteration(&space); + assert_eq!( + iter.len(), + walked.len(), + "len disagrees with iteration for {expr} dtc={dtc}" + ); + // The final chunk must be reachable by index, which it is not when len() + // under-reports. + assert!( + iter.get(walked.len() - 1).is_some(), + "last chunk unreachable for {expr} dtc={dtc}" + ); + } + } + + #[test] + fn test_chunk_metadata_and_adaptive_come_from_the_same_parameter() { + // Template validation rejects two CHUNK[INT] parameters per step, but + // StepParameterSpace is publicly constructible, so the getters must not describe + // different parameters. openjd-cli feeds chunks_parameter_name back into adaptive + // chunk-size adjustment, so a mismatch would corrupt that feedback. + let space = make_space( + vec![ + ("StaticFrame", noncontiguous_static_chunk_param("1-10", 5)), + ("AdaptiveFrame", adaptive_chunk_param((1..=10).collect(), 2)), + ], + None, + ); + let iter = StepParameterSpaceIterator::new(&space).unwrap(); + // The first CHUNK[INT] in definition order decides everything, as in the + // reference, which breaks out of its scan on the first one it finds. + assert_eq!(iter.chunks_parameter_name(), Some("StaticFrame")); + assert!(!iter.chunks_adaptive()); + assert_eq!(iter.chunks_default_task_count(), Some(5)); + } + #[test] fn test_contiguous_random_access_out_of_range_is_none() { let space = make_space(vec![("Frame", static_chunk_param("1-10", 5))], None); From 5e243ad77db974a7289b070d93a0ff0e90dfae38 Mon Sep 17 00:00:00 2001 From: David Leong Date: Tue, 1 Sep 2026 05:22:23 +0000 Subject: [PATCH 4/5] test(model): Pin stepped-follower interval merging against the reference Signed-off-by: David Leong --- crates/openjd-model/src/job/step_param_space.rs | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/crates/openjd-model/src/job/step_param_space.rs b/crates/openjd-model/src/job/step_param_space.rs index d9063aaf..3ccac0c0 100644 --- a/crates/openjd-model/src/job/step_param_space.rs +++ b/crates/openjd-model/src/job/step_param_space.rs @@ -2417,6 +2417,17 @@ mod tests { "1-1", "3-3", "5-5", "7-7", "9-10", "11-12", "13-14", "15-16", "17-18", "19-20", ], ), + // A step-1 run followed by an adjacent stepped sub-range: the follower's + // first value joins the run, the rest are isolated. + ("1-3,4-8:2", 2, vec!["1-2", "3-4", "6-6", "8-8"]), + ("1-3,4-8:2", 3, vec!["1-2", "3-4", "6-6", "8-8"]), + // Two adjacent stepped sub-ranges. + ("1-4:3,5-9:2", 2, vec!["1-1", "4-5", "7-7", "9-9"]), + ( + "2-8:2,9-12", + 2, + vec!["2-2", "4-4", "6-6", "8-9", "10-11", "12-12"], + ), ]; for (expr, dtc, reference) in cases { let space = make_space(vec![("Frame", static_chunk_param(expr, dtc))], None); @@ -2495,6 +2506,9 @@ mod tests { ("1-9:2,10-20", 2), ("1-20:2", 3), ("1-5,8-12", 3), + ("1-3,4-8:2", 2), + ("1-4:3,5-9:2", 2), + ("2-8:2,9-12", 2), ] { let space = make_space(vec![("Frame", static_chunk_param(expr, dtc))], None); let iter = StepParameterSpaceIterator::new(&space).unwrap(); From 6686b39e5b20b850babd8e2d386a72665b09ca57 Mon Sep 17 00:00:00 2001 From: David Leong Date: Tue, 1 Sep 2026 05:48:04 +0000 Subject: [PATCH 5/5] fix(model): Bridge a length-1 stepped sub-range between adjacent runs Signed-off-by: David Leong --- .../openjd-model/src/job/step_param_space.rs | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/crates/openjd-model/src/job/step_param_space.rs b/crates/openjd-model/src/job/step_param_space.rs index 3ccac0c0..fedd36bc 100644 --- a/crates/openjd-model/src/job/step_param_space.rs +++ b/crates/openjd-model/src/job/step_param_space.rs @@ -643,9 +643,18 @@ fn interval_end_index(range: &job::TaskParamRange, start: usize) -> usize { end += next_sr.len(); last_val = next_sr.end(); } else if next_sr.start() == last_val + 1 && next_sr.step() > 1 { - // First value is adjacent, but subsequent values have gaps + // The first value is adjacent, so it joins the interval. For a + // multi-value stepped sub-range the *second* value has a gap + // before it, so the interval ends here. A length-1 stepped + // sub-range has no second value, so it bridges into whatever + // follows instead of terminating the interval -- which is what + // `count_contiguous_chunks_from_sub_ranges` does, and leaving it + // out made `len()` under-report for e.g. `1-2,3-3:2,4-8`. end += 1; - break; + if next_sr.len() > 1 { + break; + } + last_val = next_sr.end(); } else { break; } @@ -2428,6 +2437,10 @@ mod tests { 2, vec!["2-2", "4-4", "6-6", "8-9", "10-11", "12-12"], ), + // A length-1 stepped sub-range between two step-1 runs. Its single value is + // adjacent on both sides, so the whole range is one contiguous interval and + // the stepped sub-range must not terminate it. + ("1-2,3-3:2,4-8", 2, vec!["1-2", "3-4", "5-6", "7-8"]), ]; for (expr, dtc, reference) in cases { let space = make_space(vec![("Frame", static_chunk_param(expr, dtc))], None); @@ -2509,6 +2522,7 @@ mod tests { ("1-3,4-8:2", 2), ("1-4:3,5-9:2", 2), ("2-8:2,9-12", 2), + ("1-2,3-3:2,4-8", 2), ] { let space = make_space(vec![("Frame", static_chunk_param(expr, dtc))], None); let iter = StepParameterSpaceIterator::new(&space).unwrap();