feat(ui): add quantity metadata to operator statistics - #471
Conversation
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesOperator statistics now store values with optional quantity metadata. Rust conversion, simulator quantity mapping and scaling, public type exports, quantity-aware formatting, and UI parsing/tests support the updated serialized shape. Operator statistics quantity metadata
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
ui/packages/@quent/components/src/lib/queryBundle.utils.test.ts-67-79 (1)
67-79: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBuild these fixtures from generated operator types.
Type the helper and explicit-quantity fixture with
Pick<Operator, 'statistics'>andOperatorStatistic(or a shared builder), then let the helper accept an optional quantity. The current lookalike objects can silently drift from the serialized contract. As per coding guidelines and path instructions, “Build test fixtures from canonical production or generated types usingPick,Partial, or shared builders.”Also applies to: 103-110
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/`@quent/components/src/lib/queryBundle.utils.test.ts around lines 67 - 79, Update makeOperator and the explicit-quantity fixture to use the generated Operator and OperatorStatistic types, constraining the fixture with Pick<Operator, 'statistics'> and accepting an optional quantity through the helper or a shared builder. Preserve the current default null quantity while allowing explicit quantities, and construct the serialized statistics shape from the canonical types rather than lookalike objects.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@ui/packages/`@quent/components/src/lib/queryBundle.utils.test.ts:
- Around line 67-79: Update makeOperator and the explicit-quantity fixture to
use the generated Operator and OperatorStatistic types, constraining the fixture
with Pick<Operator, 'statistics'> and accepting an optional quantity through the
helper or a shared builder. Preserve the current default null quantity while
allowing explicit quantities, and construct the serialized statistics shape from
the canonical types rather than lookalike objects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 399123a3-3e3f-4f10-a294-a3c72e72af86
📒 Files selected for processing (6)
domains/query_engine/analyzer/src/operator.rsdomains/query_engine/ui/src/lib.rsexamples/simulator/analyzer/src/lib.rsui/packages/@quent/components/src/lib/queryBundle.utils.test.tsui/packages/@quent/components/src/lib/queryBundle.utils.tsui/packages/@quent/utils/src/types/index.ts
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
|
@johanpel This is great, exactly what we needed. The one thing I'm seeing is all the time-related fields are getting a Perhaps this is a known limitation, but just wanted to point it out. Otherwise looks good! |
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@examples/simulator/analyzer/src/lib.rs`:
- Around line 108-126: Add focused tests for operator_statistic_quantity,
scale_operator_statistic, and quantity_specs(). Verify byte and second statistic
names, unknown names, nanosecond-to-second conversion, unchanged None and
non-U64 values, and that quantity_specs() exposes the expected keys.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: 0661faf7-412b-47f6-b070-9a9e43ed5852
📒 Files selected for processing (1)
examples/simulator/analyzer/src/lib.rs
| fn operator_statistic_quantity(name: &str) -> Option<&'static str> { | ||
| if BYTE_OPERATOR_STATISTICS.contains(&name) { | ||
| Some(QUANTITY_BYTES) | ||
| } else if SECOND_OPERATOR_STATISTICS.contains(&name) { | ||
| Some(QUANTITY_SECONDS) | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
|
|
||
| fn scale_operator_statistic(name: &str, value: &mut Option<DynamicValue>) { | ||
| if !SECOND_OPERATOR_STATISTICS.contains(&name) { | ||
| return; | ||
| } | ||
| if let Some(DynamicValue::U64(nanoseconds)) = value { | ||
| let seconds = *nanoseconds as f64 / 1_000_000_000.0; | ||
| *value = Some(DynamicValue::F64(seconds)); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== File outline ==\n'
ast-grep outline examples/simulator/analyzer/src/lib.rs --view expanded || true
printf '\n== Relevant symbols/search ==\n'
rg -n "operator_statistic_quantity|scale_operator_statistic|quantity_specs|mod tests|#\\[test\\]" examples/simulator/analyzer/src/lib.rs examples/simulator/analyzer/src -S || true
printf '\n== File line counts ==\n'
wc -l examples/simulator/analyzer/src/lib.rs || true
printf '\n== Targeted source slices ==\n'
sed -n '1,220p' examples/simulator/analyzer/src/lib.rsRepository: rapidsai/quent
Length of output: 10902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Search for tests in nearby files ==\n'
fd -a -t f 'test' examples/simulator/analyzer || true
fd -a -t f . examples/simulator/analyzer/src || true
printf '\n== Cargo manifest ==\n'
sed -n '1,220p' examples/simulator/analyzer/Cargo.toml || true
printf '\n== Search whole analyzer tree for helper names ==\n'
rg -n "operator_statistic_quantity|scale_operator_statistic|quantity_specs" examples/simulator/analyzer -S || trueRepository: rapidsai/quent
Length of output: 2127
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Focused test search in workspace ==\n'
rg -n "operator_statistic_quantity|scale_operator_statistic|quantity_specs|nanoseconds|QUANTITY_BYTES|QUANTITY_SECONDS" . -S --glob '!target' --glob '!dist' --glob '!build' || trueRepository: rapidsai/quent
Length of output: 3347
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Tail of lib.rs ==\n'
sed -n '1180,1213p' examples/simulator/analyzer/src/lib.rs
printf '\n== Any test modules in lib.rs ==\n'
rg -n "mod tests|#\\[cfg\\(test\\)\\]|#\\[test\\]" examples/simulator/analyzer/src/lib.rs -S || trueRepository: rapidsai/quent
Length of output: 1412
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Tail of lib.rs ==\n'
sed -n '1180,1213p' examples/simulator/analyzer/src/lib.rs
printf '\n== Test markers in lib.rs ==\n'
rg -n "mod tests|#\\[cfg\\(test\\)\\]|#\\[test\\]" examples/simulator/analyzer/src/lib.rs -S || trueRepository: rapidsai/quent
Length of output: 1408
Add tests for the new quantity helpers. Cover byte/second names, unknown names, nanosecond-to-second scaling, unchanged None/non-U64 values, and the quantity_specs() keys.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@examples/simulator/analyzer/src/lib.rs` around lines 108 - 126, Add focused
tests for operator_statistic_quantity, scale_operator_statistic, and
quantity_specs(). Verify byte and second statistic names, unknown names,
nanosecond-to-second conversion, unchanged None and non-U64 values, and that
quantity_specs() exposes the expected keys.
Source: Coding guidelines
Should be fixed in: |
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
|
I added this commit: 3d748c5 If the analyzer provides a value with a "seconds" quantityspec derived, it is confusing to keep the "_ns" suffix in the field name, so this commit fixes that. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ui/packages/@quent/utils/src/formatters.ts (1)
363-381: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for
formatStatWithQuantity's quantity and fallback paths.
formatters.test.tscoversformatQuantityandinferFieldFormatter, but not this helper directly. Add cases for a matching spec, missing quantity, and unknown quantity so theinferFieldFormatter(key)fallback stays intact.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ui/packages/`@quent/utils/src/formatters.ts around lines 363 - 381, Add direct tests for formatStatWithQuantity in formatters.test.ts covering a matching quantity spec, an absent quantity, and an unknown quantity. Verify matching specs use formatQuantity with the Occupancy kind, while missing or unknown quantities preserve the inferFieldFormatter(key) fallback behavior.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@ui/packages/`@quent/utils/src/formatters.ts:
- Around line 363-381: Add direct tests for formatStatWithQuantity in
formatters.test.ts covering a matching quantity spec, an absent quantity, and an
unknown quantity. Verify matching specs use formatQuantity with the Occupancy
kind, while missing or unknown quantities preserve the inferFieldFormatter(key)
fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: QUIET
Plan: Enterprise
Run ID: c140d124-d583-484b-846a-73c8716d7bf8
📒 Files selected for processing (2)
ui/packages/@quent/utils/src/formatters.tsui/packages/@quent/utils/src/index.ts
| ); | ||
|
|
||
| // Build a stat-key → QuantitySpec map for quantity-aware legend formatting. | ||
| const statQuantitySpecs = useMemo((): Record<string, QuantitySpec> => { |
There was a problem hiding this comment.
I think we could avoid assembling this object + deep prop passing of quantitySpecs and statQuantitySpecs by using the useQueryBundle hook to have access to the quantitySpecs wherever in the tree. Or implementing a useQuantitySpecs hook that would pull it out for you.
There was a problem hiding this comment.
Went ahead and replaced this with two atoms - quantitySpecsAtom (raw quantity_specs from the bundle) and statQuantitySpecsAtom (stat-key QuantitySpec derived from DAG node data). Now QueryPlan sets quantitySpecsAtom via a useEffect when the bundle loads, and FlowLayout sets statQuantitySpecsAtom after processing nodes. All components that need the data now just read from the atoms.
| max, | ||
| palette, | ||
| isDark, | ||
| quantitySpec, |
There was a problem hiding this comment.
This seems like a generic enough component, we could/should keep "quantity spec" idea out of it and pass a fmt function, and the DAG* components could deal with it.
There was a problem hiding this comment.
Went ahead and removed that to keep it generic - now just takes an optional formatValue function and falls back to inferFieldFormatter
9prady9
left a comment
There was a problem hiding this comment.
code under domains folder looks good to me. Just one clarification needed for me.
| statistic.quantity = | ||
| operator_statistic_quantity(&name).map(str::to_owned); |
There was a problem hiding this comment.
shouldn't this be gated ? on scale_operator_statistic returning non-None value ?
What would happen for stat that is _ns that has value type that is not U64 ?
Signed-off-by: Johan Peltenburg <johan.peltenburg+code@gmail.com>
|
@cmatzenbach will leave dealing with the TS comments from @johallar to your expert hands, feel free to merge after fixing those |
| ): string { | ||
| if (quantity && specs) { | ||
| const spec = specs[quantity]; | ||
| if (spec) return formatQuantity(value, spec, 'Occupancy'); |
There was a problem hiding this comment.
Are we sure that all of the custom_statistics will all be 'Occupancy' kinds and not 'Rate'? Is this actually something we may need from the BE in addition to the capacity key @johanpel ? Or does a Rate kind not make sense in this context.
There was a problem hiding this comment.
This was something I had looked into - currently all the ones we are receiving are of the Occupancy type. However, if we were to receive 'Rate' types, then yes, we would need this from the BE as well, as we have no way of determining in the FE. But I wasn't sure myself if 'Rate' types made any sense here.
…omplicated scaffolding
| palette={palette} | ||
| isDark={isDark} | ||
| /> | ||
| <ContinuousLegend field={field} min={coloring.min} max={coloring.max} palette={palette} isDark={isDark} /> |
There was a problem hiding this comment.
Do we need to pass (v: number) => formatQuantity(v, quantitySpec, 'Occupancy') for the format prop now after moving to that pattern? I think it will awlays fall back to inferring the formatter RN.
There was a problem hiding this comment.
Nope you're right, that's pointless after the big refactor. Removed
There was a problem hiding this comment.
I think we need to add (v: number) => formatQuantity(v, quantitySpec, 'Occupancy') as the formatValue prop don't we? Right now I think it will always fall back to the infer formatter path
There was a problem hiding this comment.
Is this one resolved? Don't we need to pass the formatValue prop through to take advantage of the capacity or am I misinterpreting
| </div> | ||
| <DagPlayhead startTimeUnixNs={queryBundle.start_time_unix_ns} /> | ||
| <DAGNodeInfoPanel isDark={isDark} /> | ||
| <DAGNodeInfoPanel isDark={isDark} quantitySpecs={queryBundle.quantity_specs} /> |
There was a problem hiding this comment.
nit: DAGNodeInfo could get this itself from useQueryBundle
There was a problem hiding this comment.
I don't think it's worth it - we would need to pass down two additional props (queryId and engineId) for it to be able to do so - I think it's better to just pass down the one prop it needs for rendering
There was a problem hiding this comment.
You can grab those from the router, since they're in the FE route 😄 const { engineId } = Route.useParams(); I believe
There was a problem hiding this comment.
The joys of a type safe router
| ? formatStatWithQuantity( | ||
| value, | ||
| key, | ||
| quantity && quantitySpecs ? quantitySpecs[quantity] : undefined |
There was a problem hiding this comment.
golf: quantity ? quantitySpecs?.[quantity] : undefined
johallar
left a comment
There was a problem hiding this comment.
One substantive comment: https://github.com/rapidsai/quent/pull/471/changes#r3692590810
Couple other style things, feel free to merge after the format prop comment is addressed
|
/merge |
Quickfix follow-up to PR #471 - creates statQuantitySpecs map and passes down to legend for formatting Authors: - Chris Matzenbach (https://github.com/cmatzenbach) Approvers: - Joe O'Hallaron (https://github.com/johallar) URL: #527
Description
Add optional quantity metadata to operator statistics, referencing the quantity specifications in the query bundle.
bytesquantity specification.Written by Codex.