Skip to content

Comments

chore: pass through substitutions for CH query#7389

Merged
srikanthccv merged 5 commits intomainfrom
all-values
Mar 26, 2025
Merged

chore: pass through substitutions for CH query#7389
srikanthccv merged 5 commits intomainfrom
all-values

Conversation

@srikanthccv
Copy link
Member

@srikanthccv srikanthccv commented Mar 20, 2025

Summary

Part of #6934

Explanation - https://www.loom.com/share/491ae4b986944f2bb259bbf4bce875cb


Important

Adds ClickHouse query transformation to handle variables, especially 'all', with new processing and testing components.

  • Behavior:
    • Adds chTransformQuery() in parser.go to transform ClickHouse queries with variables, especially handling 'all' selections.
    • Integrates chTransformQuery() into ParseQueryRangeParams() to process ClickHouse queries.
  • New Components:
    • Adds QueryProcessor and QueryTransformer in processor.go and transform.go for query modifications.
    • Introduces FilterAction and FilterTransformer for handling query filters.
  • Testing:
    • Adds TestTransform in transfor_test.go to validate query transformations with various variable scenarios.
  • Dependencies:
    • Adds github.com/AfterShip/clickhouse-sql-parser to go.mod for SQL parsing.

This description was created by Ellipsis for 8b8eba9. It will automatically update as commits are pushed.

@github-actions github-actions bot added the chore label Mar 20, 2025
@srikanthccv srikanthccv marked this pull request as ready for review March 20, 2025 15:23
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to ebc3db9 in 3 minutes and 24 seconds

More details
  • Looked at 911 lines of code in 6 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. pkg/variables/clickhouse/transfor_test.go:15
  • Draft comment:
    Duplicate test case name ('Example 3') appears twice. Consider renaming one to ensure clarity in test reports.
  • Reason this comment was not posted:
    Marked as duplicate.
2. pkg/variables/clickhouse/transfor_test.go:1
  • Draft comment:
    Filename 'transfor_test.go' might be a typo. Consider renaming it to 'transform_test.go' for consistency.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. pkg/variables/clickhouse/transform.go:32
  • Draft comment:
    Template variable replacement is done via multiple strings.Replace calls. Ensure that all template styles used in the product are covered by these replacements.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
4. pkg/query-service/app/parser.go:841
  • Draft comment:
    The function chTransformQuery only logs the transformed query without returning or updating it. If this logging‐only behavior is intentional (for debugging pass-through substitutions), please add a clarifying comment. Otherwise, consider returning the transformed query so that it can be used later.
  • 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/variables/clickhouse/transfor_test.go:15
  • Draft comment:
    Test case naming: Multiple test cases are labeled 'Example 3'. Consider renaming them (e.g., 'Example 3' and 'Example 4') to clearly distinguish different scenarios.
  • Reason this comment was not posted:
    Marked as duplicate.
6. pkg/variables/clickhouse/transfor_test.go:1
  • Draft comment:
    The file name 'transfor_test.go' appears to be a typo. Consider renaming it to 'transform_test.go' to match the functionality and naming conventions.
  • 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 filename does appear to be misspelled. However, according to the rules, we should not comment on file renames as these would be obviously caught by the build system. File naming issues would be caught during code review or by the developer themselves. This is a new file being added, but the same principle applies.
    The filename typo could cause confusion for other developers and make the codebase less maintainable. It's a simple fix that could improve code quality.
    While fixing typos is good, the rules explicitly state not to comment on file renames or naming issues as these are obvious issues that don't need to be pointed out.
    The comment should be deleted as it violates the rule about not commenting on file renames and naming issues.
7. pkg/variables/clickhouse/transform.go:32
  • Draft comment:
    Variable substitution in the NewQueryTransformer constructor uses multiple calls to strings.Replace. Consider using a single regex replacement or a loop over patterns to improve clarity and guard against edge cases where variable names may overlap.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
    The suggestion is technically valid - using regex could make the code more concise and potentially handle edge cases better. However, the current approach is explicit, easy to understand, and likely performs well enough. The edge case of overlapping variable names seems unlikely given the variable syntax patterns. The current code is also more maintainable for developers who may not be regex experts.
    The current implementation could miss some edge cases if new template formats are added. The repeated string operations might have a minor performance impact with many variables.
    The benefits of using regex (slightly less code, theoretical edge cases) don't outweigh the benefits of the current explicit, readable approach. Performance impact would be negligible in real-world usage.
    Delete this comment. While technically valid, the suggested change doesn't provide enough benefit to justify complicating the code with regex.

Workflow ID: wflow_w8uP7ZYIHKX1tlJR


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 srikanthccv enabled auto-merge (squash) March 26, 2025 12:51
@srikanthccv srikanthccv merged commit 88be23c into main Mar 26, 2025
12 of 14 checks passed
@srikanthccv srikanthccv deleted the all-values branch March 26, 2025 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants