refactor(intake): complete ClickHouse repository migration - #941
Merged
BrianNewsom merged 10 commits intoJul 28, 2026
Conversation
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
shanaiabuggy
approved these changes
Jul 28, 2026
Base automatically changed from
clickhouse-evaluation-read-service/brnewsom
to
main
July 28, 2026 18:03
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
Contributor
📝 WalkthroughWalkthroughIntroduces domain repository interfaces, migrates ClickHouse repositories from the raw client to ChangesIntake repository migration
Sequence Diagram(s)sequenceDiagram
participant IntakeSpansService
participant ClickHouseTraceRepository
participant ClickHouseExecutor
participant ClickHouseSpanClient
IntakeSpansService->>ClickHouseTraceRepository: list traces
ClickHouseTraceRepository->>ClickHouseExecutor: fetch_scalar and fetch_all
ClickHouseExecutor->>ClickHouseSpanClient: execute ClickHouseQuery
ClickHouseSpanClient-->>ClickHouseExecutor: count and row results
ClickHouseExecutor-->>ClickHouseTraceRepository: normalized rows
ClickHouseTraceRepository-->>IntakeSpansService: PaginatedResult
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
services/intake/src/nmp/intake/repository/clickhouse/trace.py (1)
101-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate total_results pattern. Same as annotations.py; see consolidated comment.
🤖 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 `@services/intake/src/nmp/intake/repository/clickhouse/trace.py` around lines 101 - 113, Update the total_results calculation in the trace repository to use the same consolidated helper or shared pattern established for annotations.py, instead of duplicating the fetch_scalar, count query, and fallback logic. Preserve the existing traces.list.count query parameters and zero-result behavior.services/intake/src/nmp/intake/repository/clickhouse/annotations.py (1)
86-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
fetch_scalar+or 0+int()pattern for total_results.Same total_results computation is repeated verbatim across annotations.py, evaluator_results.py, span.py (x2), and trace.py. See consolidated comment.
🤖 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 `@services/intake/src/nmp/intake/repository/clickhouse/annotations.py` around lines 86 - 111, Extract the repeated total_results computation into a shared helper and reuse it from the annotation listing flow around _executor.fetch_scalar and the equivalent listing methods in evaluator_results.py, span.py, and trace.py. Preserve the existing count query, parameters, and fallback behavior while removing the duplicated fetch_scalar/or 0/int pattern.services/intake/src/nmp/intake/repository/clickhouse/span.py (2)
160-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate total_results pattern (second occurrence in this file). See consolidated comment.
🤖 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 `@services/intake/src/nmp/intake/repository/clickhouse/span.py` around lines 160 - 169, Remove the duplicate total_results calculation in the spans repository and reuse the consolidated total-results value or shared pattern already established elsewhere in the file. Update the surrounding list-groups flow without changing its query or fallback behavior.
94-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate total_results pattern. Same as annotations.py; see consolidated comment.
🤖 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 `@services/intake/src/nmp/intake/repository/clickhouse/span.py` around lines 94 - 104, Remove the duplicated total_results count logic from the spans repository method and reuse the shared pagination/count helper established for annotations.py. Update the flow around self._executor.table(ClickHouseTable.SPANS) and the spans.list.count query while preserving the existing filters and zero-result behavior.services/intake/src/nmp/intake/repository/clickhouse/evaluator_results.py (1)
83-108: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate total_results pattern. Same as annotations.py; see consolidated comment.
🤖 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 `@services/intake/src/nmp/intake/repository/clickhouse/evaluator_results.py` around lines 83 - 108, Replace the duplicated total_results count-query pattern in the evaluator-results listing flow with the consolidated pagination/count approach already used by annotations.py. Update the code around the evaluator-results repository method while preserving its table, filters, and parameters, and keep the existing row pagination behavior unchanged.
🤖 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 `@services/intake/src/nmp/intake/repository/clickhouse/annotations.py`:
- Around line 86-111: Extract the repeated total_results computation into a
shared helper and reuse it from the annotation listing flow around
_executor.fetch_scalar and the equivalent listing methods in
evaluator_results.py, span.py, and trace.py. Preserve the existing count query,
parameters, and fallback behavior while removing the duplicated fetch_scalar/or
0/int pattern.
In `@services/intake/src/nmp/intake/repository/clickhouse/evaluator_results.py`:
- Around line 83-108: Replace the duplicated total_results count-query pattern
in the evaluator-results listing flow with the consolidated pagination/count
approach already used by annotations.py. Update the code around the
evaluator-results repository method while preserving its table, filters, and
parameters, and keep the existing row pagination behavior unchanged.
In `@services/intake/src/nmp/intake/repository/clickhouse/span.py`:
- Around line 160-169: Remove the duplicate total_results calculation in the
spans repository and reuse the consolidated total-results value or shared
pattern already established elsewhere in the file. Update the surrounding
list-groups flow without changing its query or fallback behavior.
- Around line 94-104: Remove the duplicated total_results count logic from the
spans repository method and reuse the shared pagination/count helper established
for annotations.py. Update the flow around
self._executor.table(ClickHouseTable.SPANS) and the spans.list.count query while
preserving the existing filters and zero-result behavior.
In `@services/intake/src/nmp/intake/repository/clickhouse/trace.py`:
- Around line 101-113: Update the total_results calculation in the trace
repository to use the same consolidated helper or shared pattern established for
annotations.py, instead of duplicating the fetch_scalar, count query, and
fallback logic. Preserve the existing traces.list.count query parameters and
zero-result behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5944027d-9a2a-4d65-a6d5-7e67a4307221
📒 Files selected for processing (16)
services/intake/src/nmp/intake/repository/annotations.pyservices/intake/src/nmp/intake/repository/clickhouse/annotations.pyservices/intake/src/nmp/intake/repository/clickhouse/evaluation_session.pyservices/intake/src/nmp/intake/repository/clickhouse/evaluator_results.pyservices/intake/src/nmp/intake/repository/clickhouse/executor.pyservices/intake/src/nmp/intake/repository/clickhouse/span.pyservices/intake/src/nmp/intake/repository/clickhouse/trace.pyservices/intake/src/nmp/intake/repository/evaluator_results.pyservices/intake/src/nmp/intake/repository/span.pyservices/intake/src/nmp/intake/repository/trace.pyservices/intake/src/nmp/intake/spans/api/dependencies.pyservices/intake/src/nmp/intake/spans/service.pyservices/intake/tests/test_clickhouse_architecture.pyservices/intake/tests/test_clickhouse_executor.pyservices/intake/tests/test_spans_clickhouse_repository.pyservices/intake/tests/test_traces_clickhouse_repository.py
💤 Files with no reviewable changes (1)
- services/intake/tests/test_clickhouse_architecture.py
Contributor
|
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 28, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 28, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 28, 2026
Signed-off-by: Brian Newsom <brnewsom@nvidia.com>
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Jul 28, 2026
BrianNewsom
deleted the
clickhouse-complete-repository-migration/brnewsom
branch
July 28, 2026 21:28
This was referenced Jul 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stack
Validation
tyacross repositories, service wiring, and repository tests: passedSummary by CodeRabbit