Store complete intervals in cache and update logic for response

[nityanandagohain]

part of #7183

Follow up of #7201

• This PR makes sure data correct data gets stored in cache and make sure that response returns all the data without any filtering.
  Main function to review is filterSeriesPoints

Changes are not added to querier v1, once this is reviewed I will copy it to v1.

Case where end is equal to a complete window end

---

Important

Enhance caching logic to store complete intervals and update response handling in runBuilderQuery for accurate data retrieval.

• Behavior:
  • Update runBuilderQuery in helper.go and v2/helper.go to store complete intervals in cache and ensure response returns all data.
  • Introduce FilterSeriesPoints in query_range.go to filter series points for caching.
  • Add GetSeriesFromCachedDataV2 in query_range.go to handle series data retrieval without removing points.
• Caching:
  • Update FindMissingTimeRangesV2 and MergeWithCachedSeriesDataV2 in query_range_cache.go for improved cache handling.
  • Add StoreSeriesInCache in query_range_cache.go to store merged data in cache.
• Interfaces:
  • Update QueryCache interface in interface.go to include new methods for cache handling.
• Testing:
  • Add test cases in query_range_test.go for FilterSeriesPoints and GetSeriesFromCachedDataV2.

^{This description was created by for da627b9. It will automatically update as commits are pushed.}

[ellipsis-dev]

❌ Changes requested. Reviewed everything up to 4eb2e0b in 3 minutes and 7 seconds

More details

• Looked at 888 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 10 drafted comments based on config settings.

1. pkg/query-service/querycache/query_range_cache.go:292

• Draft comment:
  Verify that 'min' and 'max' functions used during the merge of cached data are defined; otherwise, implement them to handle interval merging correctly.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. pkg/query-service/app/querier/v2/helper.go:90

• Draft comment:
  Magic number 1000 is repeated several times for converting seconds to milliseconds. Consider defining a constant, e.g. msPerSecond, to improve clarity and maintainability.
• Reason this comment was not posted:
  Marked as duplicate.

3. pkg/query-service/app/querier/v2/helper.go:117

• Draft comment:
  The filtering logic removes the first point if its timestamp is less than missStart. Verify this logic covers scenarios where multiple initial points might fall outside the complete aggregation window.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify the logic, which violates the rule against asking for verification or confirmation of intention. It does not provide a specific suggestion or point out a clear issue with the code.

4. pkg/query-service/querycache/query_range_cache.go:112

• Draft comment:
  Usage of undefined functions 'min' and 'max' for int64 comparisons. Please add helper definitions or inline the logic to ensure proper type-safe comparisons.
• Reason this comment was not posted:
  Marked as duplicate.

5. pkg/query-service/querycache/query_range_cache.go:161

• Draft comment:
  The adjustment 'adjustStep := int64(math.Min(float64(step), 60))' uses a hardcoded value (60). Consider defining a named constant providing contextual meaning to this limit.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. pkg/query-service/querycache/query_range_cache.go:268

• Draft comment:
  In MergeWithCachedSeriesDataV2, if unmarshalling cached data fails, the error is returned and later silently handled. It may be beneficial to log this error for debugging purposes.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The error handling pattern here is intentional - errors are propagated up and handled at a higher level. While logging could help with debugging, it's not strictly necessary since the error is handled gracefully. The function already has a clear error return path. Adding logging would be a minor enhancement but not a critical issue.
  The comment identifies a potential improvement for debugging, but may be overstepping by suggesting changes to an intentional error handling pattern. The current approach of propagating errors up is a valid design choice.
  While logging could help debugging, the current error handling is clean and functional. The benefit of adding logging here is minimal since the error case is already handled appropriately.
  The comment should be removed as it suggests a minor enhancement to an already functional error handling pattern. The current code handles errors appropriately by propagating them up.

7. pkg/query-service/querycache/query_range_cache.go:295

• Draft comment:
  When merging overlapping intervals, ensure that mergeSeries properly removes duplicate points to avoid redundancy in merged data.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/app/querier/v2/helper.go:128

• Draft comment:
  Typo: The comment 'making sure that empty range doesn't doesn't enter the cache' contains a duplicate "doesn't". Consider removing one instance.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  While this is technically correct, comments are not part of the logic and fixing typos in comments is very low priority. The rules state to only keep comments that require code changes. This is just a documentation fix. The meaning is still clear despite the typo.
  The typo does make the code less professional looking and could be fixed with minimal effort. Multiple instances of this typo suggest it may be copy-pasted.
  While easy to fix, the rules clearly state to only comment on logic issues, not documentation or style issues. The meaning is still clear despite the typo.
  Delete this comment as it only addresses a documentation typo and not a code logic issue.

9. pkg/query-service/app/querier/v2/helper.go:158

• Draft comment:
  Typo: The comment 'making a local clone since we should not update the global params if there is sift by' contains 'sift by' which should likely be 'shift by'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. pkg/query-service/interfaces/interface.go:99

• Draft comment:
  Typo detected: The return type in GetOverallStateTransitions is declared as []model.ReleStateItem. Considering the context and naming consistency with other rule-related types (e.g., RuleStateHistory), it seems this should be []model.RuleStateItem.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_9cIlPR306yAeBAK8

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on c6bd1dd in 1 minute and 27 seconds

More details

• Looked at 43 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 6 drafted comments based on config settings.

1. pkg/query-service/app/querier/v2/helper.go:89

• Draft comment:
  Consider adding a guard to ensure stepInterval is > 0 before using it to calculate stepMs. This avoids potential division/modulo by zero errors.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While division by zero is a valid concern, I need to consider: 1) Is this about changed code? Yes, the stepMs calculation was just added. 2) Is this a real risk? Looking at the codebase, StepInterval comes from builderQuery which likely has validation elsewhere. 3) Would this check add value? The operations (*, %) would panic anyway if stepInterval was 0. 4) Is this obvious? A basic validation that should be handled at a higher level.
  I may be assuming too much about validation happening elsewhere. Zero could theoretically get through to this function.
  However, this is a helper function that should be able to assume its inputs are valid. Input validation belongs at the API/entry point level, not in internal helper functions.
  The comment should be deleted. Input validation belongs at API boundaries, not in internal helper functions. Adding the check here would be defensive programming that adds noise.

2. pkg/query-service/app/querier/v2/helper.go:125

• Draft comment:
  Ensure series.Points is non-empty before accessing the last element to avoid potential panic. Although likely validated earlier, an explicit length check here would improve safety.
• Reason this comment was not posted:
  Comment was on unchanged code.

3. pkg/query-service/app/querier/v2/helper.go:89

• Draft comment:
  Consider replacing the hardcoded multiplier (1000) with a named constant (e.g., msPerSecond) to improve clarity and avoid magic numbers for time conversions.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  While using named constants for magic numbers is generally good practice, this case is debatable. The 1000 multiplier for ms/s conversion is a well-known standard value, and its usage is clear from the variable name stepMs. The value is also localized to this function and used consistently. The benefit of a named constant here would be minimal.
  The suggestion follows good coding practices of avoiding magic numbers. The conversion between time units can sometimes be confusing and a named constant could make the code more maintainable.
  However, 1000ms/s is an extremely well-known conversion that doesn't need explanation, and the variable name stepMs already makes the intent clear. This feels like over-engineering for minimal benefit.
  The comment should be deleted as it suggests a change that would add complexity without meaningful benefit, given that the time conversion is already clear from context and variable naming.

4. pkg/query-service/app/querier/v2/helper.go:125

• Draft comment:
  The update to use 'stepMs' for modulo operations is consistent. However, ensure that 'stepInterval' is never zero to avoid any potential division-by-zero errors.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

5. pkg/query-service/app/querier/v2/helper.go:130

• Draft comment:
  Typo: The comment 'making sure that empty range doesn't doesn't enter the cache' contains a duplicate 'doesn't'. Please remove one instance so it reads 'doesn't enter the cache'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. pkg/query-service/app/querier/v2/helper.go:299

• Draft comment:
  Typo: In the comment, 'non-existant' should be corrected to 'non-existent'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_cKCPfNeM6gqCWHwm

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 5bf79ed in 29 seconds

More details

• Looked at 14 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. pkg/query-service/querycache/query_range_cache.go:267

• Draft comment:
  Consider adding a debug log message when q.cache is nil instead of silently returning. This could help in troubleshooting unexpected nil cache scenarios.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

2. pkg/query-service/querycache/query_range_cache.go:266

• Draft comment:
  Good check to avoid nil pointer panic. Consider documenting or logging a debug/warning message when q.cache is nil so that it's clear why caching is skipped.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. pkg/query-service/querycache/query_range_cache.go:232

• Draft comment:
  Typo: The variable name 'seriesesByLabels' seems awkward and potentially a spelling mistake. Consider renaming it to 'seriesByLabels' or 'seriesMapByLabels' for improved readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_KT1I7T4YRw9cBzlp

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on f61e33a in 2 minutes and 5 seconds

More details

• Looked at 50 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 8 drafted comments based on config settings.

1. pkg/query-service/app/querier/v2/helper.go:129

• Draft comment:
  Filtering only the first point may miss additional points below missStart. Consider looping to remove all points with timestamps < missStart.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. pkg/query-service/app/querier/v2/helper.go:135

• Draft comment:
  Similar to the start filtering, the last point filtering only checks a single point. Verify if additional out-of-bound points at the end need to be removed.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. pkg/query-service/app/querier/v2/helper.go:110

• Draft comment:
  Typo: The comment has a duplicate word “data data”. Consider removing one occurrence for clarity.
• Reason this comment was not posted:
  Marked as duplicate.

4. pkg/query-service/app/querier/v2/helper.go:128

• Draft comment:
  If multiple leading points fall outside the aggregation window, this single check removes only one. Consider using a loop to drop all points with timestamps < missStart if that scenario is possible.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

5. pkg/query-service/app/querier/v2/helper.go:134

• Draft comment:
  Similarly, for filtering the last point, consider if a loop is needed to remove all trailing points that may fall on an incomplete window.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. pkg/query-service/app/querier/v2/helper_test.go:226

• Draft comment:
  Test case names should be unique. There are duplicate names (e.g. 'Filter last point') which can make identifying failures harder.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. pkg/query-service/app/querier/v2/helper.go:110

• Draft comment:
  Typographical error: The comment contains a duplicate word 'data data'. Please change it to 'if data for the series is empty, then we will add it to the cache'.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While this is a real typo, comments about typos in comments are generally very low value. The meaning is still clear despite the typo. The rules say to only keep comments that require clear code changes - typos in comments are more like suggestions than required changes.
  The typo does make the code slightly less professional looking. Clear communication in comments can be important for maintainability.
  While clean comments are good, this is an extremely minor issue that doesn't impact functionality or maintainability in any meaningful way. The rules specifically say not to make purely informative comments.
  Delete this comment as it's too minor and doesn't require a code change - it's just about a typo in a comment.

8. pkg/query-service/app/querier/v2/helper.go:140

• Draft comment:
  Typographical error: The comment 'making sure that empty range doesn't doesn't enter the cache' contains a duplicate 'doesn't'. Please remove one instance.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_UGprifgvBtsmq6IC

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on a7ff27e in 1 minute and 28 seconds

More details

• Looked at 13 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 1 drafted comments based on config settings.

1. pkg/query-service/app/querier/v2/helper_test.go:225

• Draft comment:
  Updated test name to 'respect actual empty series' clarifies that the function retains an empty series (with empty Points) instead of removing it entirely. This naming aligns better with the intended behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is purely informative, explaining a change in the test name. It doesn't provide a suggestion or ask for confirmation on any specific code behavior.

Workflow ID: wflow_cO0zc8RRtpnWsZVX

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

❌ Changes requested. Incremental review on 8563bcd in 3 minutes and 3 seconds

More details

• Looked at 273 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 15 drafted comments based on config settings.

1. pkg/query-service/app/querier/helper.go:149

• Draft comment:
  Using GetSeriesFromCachedDataV2 now requires passing builderQuery.StepInterval. Please verify that StepInterval is in seconds (and correctly converted to milliseconds inside the function).
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to verify the conversion of StepInterval from seconds to milliseconds, which is not allowed according to the rules. It does not provide a specific suggestion or point out a clear issue with the code.

2. pkg/query-service/app/querier/v2/helper.go:84

• Draft comment:
  Ensure the filtering logic in filterSeriesPoints correctly handles aggregation window boundaries. Consider adding comments to clarify why the first/last points are removed.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. pkg/query-service/app/querier/v2/helper.go:232

• Draft comment:
  Verify that merging of filteredMissedSeries (for cache storage) and the raw missedSeries (for the response) is intended and correctly implemented.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to verify their intention and ensure correctness, which violates the rule against asking for confirmation or verification of intent. It does not provide a specific suggestion or point out a clear issue.

4. pkg/query-service/querycache/query_range_cache.go:287

• Draft comment:
  MergeWithCachedSeriesDataV2 now omits returning errors and falls back to newData if unmarshalling fails. Confirm that this fallback behavior is acceptable and won’t mask unexpected issues.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to confirm that the fallback behavior is acceptable and won't mask unexpected issues. This falls under the rule of not asking the author to confirm their intention or ensure behavior is intended. Therefore, this comment should be removed.

5. pkg/query-service/app/querier/helper.go:148

• Draft comment:
  Changed call to use GetSeriesFromCachedDataV2 with step interval to ensure complete intervals are returned. Confirm that this aligns with the intended cache behavior.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to confirm their intention regarding the use of GetSeriesFromCachedDataV2. This violates the rule against asking for confirmation of intention. The comment does not provide a specific code suggestion or ask for a test to be written. Therefore, it should be removed.

6. pkg/query-service/app/querier/v2/helper.go:84

• Draft comment:
  The filterSeriesPoints function adjusts aggregation window boundaries by trimming first/last points. Verify that these conditions correctly handle all edge cases and consider adding more unit tests for boundary scenarios.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. pkg/query-service/app/querier/v2/helper.go:238

• Draft comment:
  Merged series are retrieved using GetSeriesFromCachedData (not V2). Confirm that this choice is intentional, as it may filter out points that the V2 version would retain.
• Reason this comment was not posted:
  Comment was on unchanged code.

8. pkg/query-service/common/query_range.go:128

• Draft comment:
  For clarity, consider adding extra parentheses in the expression 'start - (start%step*1000)' (e.g. 'start - ((start % step) * 1000)') so that the intended order of operations is unmistakable.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  In Go, operator precedence is well-defined - multiplication has higher precedence than modulo, which has higher precedence than subtraction. The current expression and suggested expression are mathematically equivalent. The existing pattern is already used elsewhere in the codebase. While extra parentheses might make it slightly more readable, this seems like a minor stylistic preference rather than a significant improvement or bug risk.
  The comment does identify a potentially confusing expression that could be made clearer. Some developers might misread the operator precedence at first glance.
  However, the expression follows established patterns in the codebase and Go's operator precedence rules are unambiguous. The benefit of extra parentheses is minimal compared to maintaining consistency with existing code.
  Delete the comment as it suggests a minor stylistic change that doesn't significantly improve code quality or reduce bugs, and would create inconsistency with similar expressions elsewhere in the codebase.

9. pkg/query-service/common/query_range_test.go:10

• Draft comment:
  Test cases here are basic. Consider adding more comprehensive tests covering edge cases such as misaligned start/end times relative to the aggregation window.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

10. pkg/query-service/interfaces/interface.go:152

• Draft comment:
  The removal of the error return from MergeWithCachedSeriesDataV2 simplifies its interface. Ensure that all call sites properly handle fallback cases in the absence of an error.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that all call sites handle fallback cases, which is similar to asking them to double-check or verify something. This violates the rule against asking the author to ensure behavior is intended or tested.

11. pkg/query-service/app/querier/helper.go:214

• Draft comment:
  Typographical error: In the comment explaining the caching logic, 'non-existant' should be corrected to 'non-existent' for clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. pkg/query-service/app/querier/v2/helper.go:140

• Draft comment:
  Typo: The comment 'making sure that empty range doesn't doesn't enter the cache' contains a duplicated word. Please remove one instance of 'doesn't' to improve readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. pkg/query-service/app/querier/v2/helper.go:170

• Draft comment:
  Typo: In the comment 'making a local clone since we should not update the global params if there is sift by', 'sift' should be corrected to 'shift' to match the actual parameter name.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. pkg/query-service/app/querier/v2/helper.go:303

• Draft comment:
  Typo: The comment 'A non-existant cache key means that the query is not cached.' uses 'non-existant' which is a misspelling. Consider changing it to 'nonexistent' for correctness.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

15. pkg/query-service/common/query_range.go:127

• Draft comment:
  Typo: In the comment, consider changing "because doesn't remove a point" to "because it doesn't remove a point" for clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the suggestion would make the comment more grammatically correct, this is a very minor issue that doesn't affect code functionality or understanding. The meaning is clear even without the suggested change. The rules state not to make comments that are obvious or unimportant. Grammar nitpicks generally fall into this category unless they significantly impact clarity.
  The comment does point out a real grammatical issue, and proper documentation is important for code maintainability.
  While documentation quality matters, this minor grammatical fix doesn't meaningfully improve code understanding or maintainability. The meaning is already clear.
  Delete this comment as it's too minor and doesn't meaningfully improve the code or documentation.

Workflow ID: wflow_WvzjGuJElZhsZQaS

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 064a522 in 1 minute and 51 seconds

More details

• Looked at 26 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 14 drafted comments based on config settings.

1. pkg/query-service/app/querier/helper.go:149

• Draft comment:
  Using GetSeriesFromCachedData() here removes the stepInterval parameter. Ensure that v1 behavior remains consistent with previous logic that used GetSeriesFromCachedDataV2 and that no edge cases are affected.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that the behavior remains consistent with the previous logic and that no edge cases are affected. This falls under the rule of not asking the author to ensure behavior is intended or tested. Therefore, this comment should be removed.

2. pkg/query-service/app/querier/v2/helper.go:238

• Draft comment:
  Ensure that using GetSeriesFromCachedDataV2 with the stepInterval parameter correctly aligns with the new complete interval filtering logic introduced in filterSeriesPoints.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that a specific function usage aligns with new logic. It doesn't provide a specific suggestion or point out a clear issue, which violates the rule against asking the author to ensure behavior is intended.

3. pkg/query-service/app/querier/v2/helper.go:84

• Draft comment:
  In filterSeriesPoints, consider potential performance impact due to sorting every series, and double-check boundary removal logic to ensure no valid data is inadvertently dropped.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. pkg/query-service/app/querier/helper.go:149

• Draft comment:
  Using common.GetSeriesFromCachedData without passing builderQuery.StepInterval – confirm this omission is intentional for correct series slicing.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  The comment asks for confirmation/verification, which violates the rule "Do NOT ask the PR author to confirm their intention". Additionally, the consistent usage elsewhere in the file suggests this is an intentional pattern. The comment is not making a clear assertion that something is wrong, just asking for verification.
  Perhaps the removal of stepInterval could cause actual functional issues that I'm missing. The V2 to non-V2 version change could be significant.
  While the version change could be significant, the comment doesn't make a clear case for why this is problematic. It just asks for verification, which our rules explicitly discourage.
  Delete the comment because it violates the rule against asking for verification/confirmation and doesn't make a clear case for why the change is problematic.

5. pkg/query-service/app/querier/v2/helper.go:129

• Draft comment:
  Consider using the adjusted startTime (computed below) for filtering the first point instead of comparing directly with missStart.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

6. pkg/query-service/app/querier/v2/helper.go:135

• Draft comment:
  Consider filtering the last point using the adjusted endTime instead of comparing with missEnd - (missEnd % stepMs) for clarity and consistency.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

7. pkg/query-service/app/querier/v2/helper.go:140

• Draft comment:
  Remove duplicate 'doesn't' in the comment for improved clarity.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/app/querier/v2/helper.go:225

• Draft comment:
  Clarify why two arrays (filteredMissedSeries for caching and missedSeries for response) are maintained—explain the rationale for caching only complete intervals while returning unfiltered data.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. pkg/query-service/app/querier/v2/helper.go:120

• Draft comment:
  If series.Points are expected to be pre-sorted, the sort.Slice call may be redundant. Consider checking if sorting is required to improve performance.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

10. pkg/query-service/app/querier/v2/helper.go:307

• Draft comment:
  Ensure that passing metricsV3.Options{} to metricsV4.PrepareMetricQuery is intentional and that no required options are omitted.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. pkg/query-service/app/querier/helper.go:213

• Draft comment:
  There's a typographical error in the comment on lines 213-215: 'non-existant' should be corrected to 'non-existent'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. pkg/query-service/app/querier/v2/helper.go:170

• Draft comment:
  Typo in comment: 'sift by' should be corrected to 'shift by' to accurately describe the intended behavior.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

13. pkg/query-service/app/querier/v2/helper.go:140

• Draft comment:
  Typo in comment: the phrase 'doesn't doesn't' contains a duplicate and should be corrected to a single 'doesn't'.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

14. pkg/query-service/app/querier/v2/helper.go:303

• Draft comment:
  Typo in comment: 'non-existant' should be corrected to 'non-existent' for proper spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_cxmxu5zsZEedPuQk

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on ca65b41 in 1 minute and 21 seconds

More details

• Looked at 14 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 4 drafted comments based on config settings.

1. pkg/query-service/app/querier/v2/helper.go:217

• Draft comment:
  Ensure that caching empty results is intended. The added condition (len(series)==0) differentiates between filtered-out data and original empty series; confirm edge case tests cover both scenarios.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to confirm their intention regarding caching empty results and to ensure edge case tests cover specific scenarios. This violates the rule against asking the author to confirm their intention or ensure testing. Therefore, this comment should not be approved.

2. pkg/query-service/app/querier/v2/helper.go:218

• Draft comment:
  The new condition correctly caches empty series when no data is returned (i.e. len(series)==0), preventing valid empty responses from being filtered out. Ensure this behavior is covered by unit tests.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The comment asks to "Ensure" something is tested, which violates the rule about not making verification requests. While test coverage is important, especially for caching behavior changes, the rules explicitly state not to make comments that start with "Ensure that..." or ask authors to verify/check things. The behavioral change is significant but asking for test coverage in this way is not the right approach.
  The change in caching behavior is important and could cause bugs if not tested properly. Maybe we're being too strict about the "Ensure that..." rule when it comes to test coverage.
  While test coverage is important, the rules are clear about not making verification requests. There are better ways to suggest test coverage, like proposing specific test cases or making a direct code suggestion.
  The comment should be deleted because it violates the rule about not making verification requests, even though the underlying concern about test coverage is valid.

3. pkg/query-service/app/querier/v2/helper.go:141

• Draft comment:
  Typo: The comment 'making sure that empty range doesn't doesn't enter the cache' contains a duplicate 'doesn't'. Please change it to 'doesn't' so it reads correctly.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

4. pkg/query-service/app/querier/v2/helper.go:304

• Draft comment:
  Typo: The comment contains 'non-existant cache key'. 'Non-existant' should be corrected to 'non-existent' (or 'nonexistent') for proper spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_gsKCY0UIuZtN39v2

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[ellipsis-dev]

👍 Looks good to me! Incremental review on 52780a7 in 42 seconds

More details

• Looked at 12 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 3 drafted comments based on config settings.

1. pkg/query-service/querycache/query_range_cache.go:288

• Draft comment:
  Consider including the cacheKey in the error log message for better debugging context.
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

2. pkg/query-service/querycache/query_range_cache.go:289

• Draft comment:
  Include the cache key in the error log for better debugging context (e.g., zap.L().Error(..., zap.String("cacheKey", cacheKey))).
• Reason this comment was not posted:
  Confidence changes required: 33% <= threshold 50%
  None

3. pkg/query-service/querycache/query_range_cache.go:232

• Draft comment:
  The variable name 'seriesesByLabels' feels awkward and could be a typographical mistake. Consider renaming it to 'seriesByLabels' or similar for improved clarity and consistency. This change is trivial and only affects readability.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_OAPgSVfdmHBQo6FQ

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[srikanthccv]

as the filter uses the <= here https://github.com/SigNoz/signoz/blob/main/pkg/query-service/app/logs/v4/query_builder.go#L362. Here is another case that's not handled properly.

		{
			name:         "Some test case",
			missStart:    1609466400000, // 01 Jan 2021 02:00:00 UTC
			missEnd:      1609470000000, // 01 Jan 2021 03:00:00 UTC
			stepInterval: 3600,          // 1 hour
			seriesList: []*v3.Series{
				{
					Points: []v3.Point{
						{Timestamp: 1609466400000, Value: 2.0}, // 01 Jan 2021 02:00:00 UTC
						{Timestamp: 1609470000000, Value: 3.0}, // 01 Jan 2021 03:00:00 UTC
					},
				},
			},
			expectedPoints: []*v3.Series{
				{
					Points: []v3.Point{
						{Timestamp: 1609466400000, Value: 2.0},
					},
				},
			},
			expectedStart: 1609466400000,
			expectedEnd:   1609470000000,
		},

If there is a data in the DB whose timestamp matches with the end, then CH will return data for it and it will be incorrectly stored in the cache even though it doesn't cover the full range but only exact timestamp that could also be the timestamp that is multiple of step interval.

[ellipsis-dev]

❌ Changes requested. Incremental review on c9da600 in 2 minutes and 38 seconds

More details

• Looked at 501 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 12 drafted comments based on config settings.

1. pkg/query-service/app/querier/helper.go:84

• Draft comment:
  The function filterSeriesPoints sorts the points for each series. If the data is already guaranteed to be sorted, this may be redundant and impact performance. Consider verifying sorting assumptions or documenting why sorting is needed here.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 50% <= threshold 50%
  The comment suggests verifying sorting assumptions or documenting why sorting is needed, which falls under asking the PR author to confirm their intention or ensure behavior. This violates the rules against asking for confirmation or verification. However, it does point out a potential performance issue, which is useful. I would rate this as partially useful.

2. pkg/query-service/app/querier/helper.go:135

• Draft comment:
  The removal of the last point combines two conditions for complete and incomplete aggregation windows. Ensure both conditions cover all edge cases. It may help to add inline comments clarifying the intent for maintainability.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the author to ensure that edge cases are covered and to add inline comments for maintainability. This is a request for clarification and testing, which violates the rules against asking the author to ensure behavior or add comments for maintainability.

3. pkg/query-service/app/querier/v2/helper.go:135

• Draft comment:
  The filter logic in v2's filterSeriesPoints mirrors that of v1. Ensure changes in caching logic (using MergeWithCachedSeriesDataV2) and filtering criteria are thoroughly tested for consistency.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that changes are thoroughly tested, which violates the rule against asking for confirmation or testing. It doesn't provide a specific suggestion or point out a specific issue with the code.

4. pkg/query-service/app/querier/v2/helper_test.go:243

• Draft comment:
  Test coverage for filtering logic appears comprehensive. Verify that edge cases (e.g., half range, empty series) are correctly maintained. Consider adding comments to critical test cases for clarity.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment is asking the PR author to verify edge cases and add comments for clarity. This falls under asking the author to ensure behavior is intended and tested, which is against the rules.

5. pkg/query-service/app/querier/helper.go:129

• Draft comment:
  Potential slice out‐of‐range: After removing the first point by slicing ‘points’, the subsequent check for the last point uses the original 'series.Points' (and then slices ‘points’) without verifying that the filtered slice isn’t empty. Consider checking that len(points) > 0 and using points[len(points)-1] in the condition to avoid a possible panic.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

6. pkg/query-service/app/querier/v2/helper.go:134

• Draft comment:
  Similar potential issue in filterSeriesPoints: The function copies and possibly slices off the first point into 'points', but the subsequent check for the last point still uses 'series.Points' without verifying that 'points' is non‐empty. This could lead to a panic if the removal makes the slice empty. Consider checking len(points) > 0 and using the filtered slice for the condition.
• Reason this comment was not posted:
  Comment was on unchanged code.

7. pkg/query-service/app/querier/v2/helper.go:150

• Draft comment:
  Consider refactoring the duplicated filterSeriesPoints implementation found in both the root querier and the v2 package to a shared utility. This would reduce code duplication and ease future maintenance.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

8. pkg/query-service/app/querier/helper.go:142

• Draft comment:
  Typo: The comment contains a duplicated word "doesn't doesn't". It would be clearer as "making sure that empty range doesn't enter the cache".
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
  While the comment is technically correct about the typo, fixing typos in comments is a very minor issue that doesn't affect functionality. The comment is still understandable despite the typo. According to the rules, we should not make purely informative comments or comments about obvious/unimportant issues.
  The typo does make the code slightly less professional looking, and fixing it would improve code quality marginally.
  However, the benefit is extremely minor and this type of nitpicking about comment wording creates noise in code reviews that distracts from more important issues.
  This comment should be deleted as it addresses a very minor typo that doesn't meaningfully impact code quality or understanding.

9. pkg/query-service/app/querier/helper.go:216

• Draft comment:
  Typo: The comment contains a duplicated word "doesn't doesn't". Please remove the extra instance to improve clarity.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  While this is a real typo in a comment that was added in this diff, fixing typos in comments is not a critical code quality issue. The meaning is still clear despite the typo. The rules don't specifically mention comment typos, but they do emphasize only keeping comments that require clear code changes.
  The typo does affect readability slightly, and fixing it would improve code quality marginally. It's a valid observation.
  However, this is an extremely minor issue that doesn't impact functionality or maintainability in any meaningful way. The rules emphasize focusing on substantial issues.
  Delete this comment as it points out a trivial typo in a comment that doesn't meaningfully impact code quality or maintainability.

10. pkg/query-service/app/querier/helper.go:304

• Draft comment:
  Typo: The comment uses "non-existant"; consider correcting it to "non-existent" for proper spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

11. pkg/query-service/app/querier/v2/helper.go:141

• Draft comment:
  Typographical issue: The comment reads "making sure that empty range doesn't doesn't enter the cache". Please remove the duplicate "doesn't".
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

12. pkg/query-service/app/querier/v2/helper.go:305

• Draft comment:
  Typographical issue: The comment uses "non-existant". Consider changing it to "non-existent" for correct spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_GZ3btzt42HSUesWe

---

Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

[srikanthccv]

Final question. Otherwise, LGTM.

[ellipsis-dev]

👍 Looks good to me! Incremental review on da627b9 in 1 minute and 41 seconds

More details

• Looked at 1224 lines of code in 6 files
• Skipped 0 files when reviewing.
• Skipped posting 11 drafted comments based on config settings.

1. pkg/query-service/common/query_range.go:206

• Draft comment:
  The filtering logic in FilterSeriesPoints removes only the very first point if it is misaligned. Consider whether multiple initial points might need removal to strictly enforce complete windows.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
  The code sorts points by timestamp first, so the first point will be the earliest. The function adjusts startTime to the next complete window if missStart isn't aligned. The comment has a valid point - there could be multiple points before missStart that should be removed. However, this seems more like a speculative edge case since the points are sorted.
  The comment identifies a real potential issue, but we don't have evidence that this is actually causing problems in practice. The current implementation might be intentionally simple.
  While technically correct, without evidence that this is causing actual issues, this feels like a speculative "what if" comment rather than a clear problem that needs fixing.
  The comment should be removed as it's speculative and doesn't point to a definite issue that needs fixing. The current implementation, while simple, may be sufficient for the actual use case.

2. pkg/query-service/common/query_range.go:145

• Draft comment:
  Please confirm that the inclusive condition (<= end) in GetSeriesFromCachedDataV2 is the desired behavior for including the endpoint.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to confirm their intention, which violates the rule against asking for confirmation of intention. It does not provide a specific code suggestion or highlight a clear issue.

3. pkg/query-service/common/query_range.go:170

• Draft comment:
  The adjustment for an incomplete aggregation window returns an empty series. Ensure this behavior is always desired and documented.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  This comment is asking the PR author to ensure that a behavior is desired and documented. It violates the rule against asking the author to confirm their intention or ensure behavior is intended.

4. pkg/query-service/common/query_range.go:207

• Draft comment:
  Consider whether a single removal of the first point is sufficient. If multiple points in a series have timestamps below missStart, only one is removed. A loop-based filter might be safer to ensure all out-of-window points are excluded.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50%
  The code sorts points by timestamp before filtering, so the first point will be the earliest point. Since points are sorted, if we remove the first point that's below missStart, any remaining points must be >= missStart. Therefore, a single removal is actually sufficient and the comment's concern is incorrect. The sorting on line 199 is key context that makes the current approach correct.
  I could be wrong if there's some way the sort could fail or if points could be added after sorting. Also, I'm assuming the sort is stable.
  The sort is using Go's standard library sort.Slice which is guaranteed to be stable since Go 1.8. The points array isn't modified between sort and filter operations.
  The comment should be deleted because it raises an incorrect concern - the code is actually correct due to the sorting step that happens first.

5. pkg/query-service/common/query_range.go:212

• Draft comment:
  The logic for filtering the last point uses a compound conditional relying on 'endCompleteWindow'. Consider adding inline comments to clarify these boundary conditions for future maintainers.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

6. pkg/query-service/common/query_range.go:144

• Draft comment:
  The lower bound in GetSeriesFromCachedDataV2 (using start - (start % (step*1000))) might benefit from an explicit comment explaining why this adjustment is needed.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
  The comment suggests adding an explicit comment to explain a specific code adjustment. This is a request for clarification, which is not allowed by the rules. The comment does not provide a specific code suggestion or identify a potential issue with the code itself.

7. pkg/query-service/common/query_range.go:198

• Draft comment:
  Since the points array is sorted on every call, if upstream guarantees ordering, you might skip this sort for performance. If not, consider documenting the rationale for re-sorting.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50%
  None

8. pkg/query-service/app/querier/helper.go:144

• Draft comment:
  Typographical error: the comment contains a repeated 'doesn't doesn't'. Please remove the extra instance to improve clarity.
• Reason this comment was not posted:
  Comment was on unchanged code.

9. pkg/query-service/app/querier/helper.go:232

• Draft comment:
  Typographical error: the comment uses 'non-existant' instead of 'non-existent'. Please fix the spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

10. pkg/query-service/app/querier/v2/helper.go:146

• Draft comment:
  The comment on line 145 contains a duplicate 'doesn't' ('empty range doesn't doesn't enter the cache'). It should be corrected to remove the duplication.
• Reason this comment was not posted:
  Comment was on unchanged code.

11. pkg/query-service/app/querier/v2/helper.go:233

• Draft comment:
  The comment on line 233 uses the term 'non-existant'. Consider replacing it with 'non-existent' (or 'nonexistent') for proper spelling.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

Workflow ID: wflow_urPg7eme17zXnIpn

---

You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.