deviceFilters: stop computing six Pinot queries to answer one - #1679
deviceFilters: stop computing six Pinot queries to answer one#1679ivan-flamingo wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough
ChangesDevice-filter facet selection
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GraphQL as DeviceDataFetcher
participant Service as DeviceFilterService
participant Executor as DeviceFilterFacetExecutor
participant Repository as DeviceFilterRepository
GraphQL->>Service: requested DeviceFilterFacet values
Service->>Executor: submit selected facet queries
Executor->>Repository: execute repository queries
Repository-->>Service: facet results
Service-->>GraphQL: DeviceFilters result
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
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
`@openframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.java`:
- Around line 26-27: Make the DeviceFilterFacet.ALL collection unmodifiable
while preserving its complete EnumSet contents, so callers cannot mutate the
shared default used by the single-argument service overload and null-selection
fallback.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 63a5f53a-30a0-4349-af08-a88599c0a2a4
📒 Files selected for processing (6)
openframe-api-lib/src/main/java/com/openframe/api/config/DeviceFilterExecutorConfig.javaopenframe-api-lib/src/main/java/com/openframe/api/dto/device/DeviceFilterFacet.javaopenframe-api-lib/src/main/java/com/openframe/api/service/DeviceFilterService.javaopenframe-api-lib/src/test/java/com/openframe/api/service/DeviceFilterServiceTest.javaopenframe-api-service-core/src/main/java/com/openframe/api/datafetcher/DeviceDataFetcher.javaopenframe-api-service-core/src/test/java/com/openframe/api/datafetcher/DeviceFiltersSelectionSetTest.java
ecf64c4 to
a4ec242
Compare
c14b195 to
102c856
Compare
What this fix does not do:
Collapse six facets into one Pinot query when six facets are genuinely requested.
What this fix do:
After the PR, the cost is one query per selected facet:
filteredCountGET_DEVICE_COUNTS_QUERY)statuses,organizationIds,filteredCountMerging the facets that are requested into a single Pinot query is a separate axis, and is limited by the self-exclusion faceting: when filters are active, each facet query has a different WHERE clause (each facet drops the filter on its own field), so only the unfiltered case could be merged.
Summary by CodeRabbit
Performance
Compatibility