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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
|
|
||
| const statQuantitySpecs = useMemo(() => { | ||
| const quantitySpecs = queryBundle.quantity_specs; | ||
| const result: Record<string, import('@quent/utils').QuantitySpec> = {}; |
There was a problem hiding this comment.
Prefer imports at the top
There was a problem hiding this comment.
There's more of this throughout just FYI, prefer it not inline anywhere
| ); | ||
|
|
||
| // 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.
| 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.
9prady9
left a comment
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 |
5 participants
Description
Add optional quantity metadata to operator statistics, referencing the quantity specifications in the query bundle.
bytesquantity specification.Written by Codex.