Advanced Query Processing Architecture

[akshayutture-augment]

Test 4

---

Replicated from ai-code-review-evaluation/grafana-greptile#4

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[akshayutture-augment]

@claude review this PR

[akshayutture-augment]

@claude review this PR

[claude]

Claude finished @akshayutture-augment's task —— View job

---

Code Review Complete ✅

I've reviewed the PR "Advanced Query Processing Architecture" which refactors query interpolation to happen earlier in the query splitting process.

---

Summary

This PR changes when template variable interpolation occurs in the query processing pipeline - moving it from later in the process to the very start. The changes affect both time-based and shard-based query splitting functions.

Files Changed:

• querySplitting.ts - Added early interpolation in runSplitQuery()
• shardQuerySplitting.ts - Replaced interpolateVariablesInQueries() with applyTemplateVariables()
• Both test files - Added new test cases and updated mocks

---

Findings

✅ Positive Aspects

1. Consistency Improvement: Both runSplitQuery() and runShardSplitQuery() now use the same pattern for variable interpolation
2. Good Test Coverage: New test cases verify that interpolation happens correctly (lines 75-88 in querySplitting.test.ts and 84-101 in shardQuerySplitting.test.ts)
3. API Alignment: Switching from interpolateVariablesInQueries() to applyTemplateVariables() appears to be using a more standard datasource method

⚠️ Issues & Concerns

1. Critical: Mutation Pattern
In shardQuerySplitting.ts:49-52, the code mutates the query object:

.map((query) => datasource.applyTemplateVariables(query, request.scopedVars, request.filters));

This is concerning because:

• The test mock shows applyTemplateVariables() mutates the input: query.expr = query.expr.replace(...) (line 57)
• If applyTemplateVariables() returns the same mutated reference, this could cause issues with the original request.targets array
• Consider whether the method should return a new object or if callers need to be aware of mutation

Recommendation: Verify that applyTemplateVariables() either:

• Returns a new query object (preferred), OR
• Documents that it mutates the input, so callers can defensively copy if needed

2. Inconsistent Call Count Expectations
In shardQuerySplitting.test.ts:112:

expect(datasource.applyTemplateVariables).toHaveBeenCalledTimes(5);

Previously it expected 1 call to interpolateVariablesInQueries (batch operation), now it expects 5 calls to applyTemplateVariables (one per query). This is correct for the new implementation, but:

• The comment on line 79 says "5 shards" - this might be confusing since the call count relates to how many times interpolation happens, not the number of shards
• Consider adding a clarifying comment about why it's called 5 times vs the previous 1 time

3. Repeated Test Setup Pattern
In both test files, the interpolation test (lines 75-88 in querySplitting.test.ts and 84-101 in shardQuerySplitting.test.ts) recreates the datasource with a custom template variable function. Consider:

• Whether this could be a shared test helper
• The mock implementation uses simple string replacement which may not match real-world behavior

4. Missing Error Handling Context
The changes don't show how errors in applyTemplateVariables() are handled. If variable interpolation fails:

• Will it throw an exception?
• Will it be silently ignored?
• Should there be try-catch blocks?

📝 Minor Issues

1. Line 296-299 in querySplitting.ts: The chained .filter().filter().map() could potentially be combined for better performance, though this is a micro-optimization

2. Test Consistency: The new interpolation test in shardQuerySplitting.test.ts expects a different output format with __stream_shard__ in the result (line 97). Make sure this is intentional behavior specific to shard splitting.

---

Questions for Clarification

1. Why the change? - What was the motivation for moving interpolation earlier in the pipeline? This context would help reviewers understand if there are related issues that should be tested.

2. Breaking change? - Does interpolateVariablesInQueries() still exist in the codebase, or is it being deprecated? Are there other call sites that need updating?

3. Performance impact? - Are there performance implications of interpolating queries multiple times (once per query) vs batch interpolation?

---

Recommendation

Overall Assessment: The changes are reasonable and well-tested, but need clarification on the mutation behavior of applyTemplateVariables().

Action Items:

• ✅ Verify applyTemplateVariables() mutation behavior
• ✅ Add comments explaining why applyTemplateVariables is called 5 times in the test
• ⚠️ Consider adding error handling if variable interpolation can fail
• 💡 Update PR description to explain the motivation for this change

---