fix: added backticks for tags containing dots while generating query … #6727
Conversation
This file contains 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
…for v3 and v4 | 6069
|
Aniket seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to b409724 in 1 minute and 54 seconds
More details
- Looked at
153lines of code in5files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. pkg/query-service/app/metrics/v3/query_builder.go:220
- Draft comment:
Ensureutils.AddBackTickToFormatTagsis applied consistently across all functions that handle tags, such asgroupingSets,groupBy,groupSelect, andorderBy. This is crucial for SQL syntax correctness when dealing with tags containing dots. - Reason this comment was not posted:
Comment did not seem useful.
2. pkg/query-service/app/metrics/v4/helpers/clauses.go:22
- Draft comment:
Ensureutils.AddBackTickToFormatTagis applied consistently across all functions that handle tags, such asGroupingSetsByAttributeKeyTags,GroupByAttributeKeyTags, andOrderByAttributeKeyTags. This is crucial for SQL syntax correctness when dealing with tags containing dots. - Reason this comment was not posted:
Marked as duplicate.
3. pkg/query-service/app/metrics/v4/helpers/sub_query.go:325
- Draft comment:
Ensureutils.AddBackTickToFormatTagis applied consistently across all functions that handle tags, such as inPrepareTimeseriesFilterQueryandPrepareTimeseriesFilterQueryV3. This is crucial for SQL syntax correctness when dealing with tags containing dots. - Reason this comment was not posted:
Marked as duplicate.
4. pkg/query-service/app/metrics/v3/query_builder.go:217
- Draft comment:
Avoid using thecomponent/index.tsxfile structure approach, as it makes it difficult to debug and find components using global search tools like VS Code. This is applicable in other files as well. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_SSyBHF56SP0ESaiE
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
reviewed
Dec 27, 2024
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 0fd63ca in 52 seconds
More details
- Looked at
284lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/metrics/v4/helpers/clauses.go:48
- Draft comment:
Modifyingitem.ColumnNamedirectly can lead to side effects. Consider using a local variable for the modified column name instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
In Go, range loop variables for slices are copies, not references. Modifying item.ColumnName only affects the copy, not the original slice. The modified value is only used immediately after in the fmt.Sprintf() call. There's no evidence of any actual side effects or issues. The suggestion to use a local variable would just add unnecessary verbosity.
I could be wrong about the scope of usage - perhaps the modified items slice is used elsewhere in the codebase. Also, using a local variable could make the code more readable.
Even if the items slice is used elsewhere, modifying the loop variable is still safe due to Go's value semantics. The code is already quite readable and a local variable would add unnecessary complexity.
The comment raises a non-issue since modifying the loop variable is safe in this context and the current code is clear and concise.
2. pkg/query-service/app/metrics/v4/helpers/clauses.go:54
- Draft comment:
Redundant backtick formatting fortag.Keyhere. It's already formatted in the loop above. Remove this line to avoid unnecessary operations. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_7ogB5F8afupdoRzK
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
…ining dots | 6069
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on df9f630 in 22 seconds
More details
- Looked at
80lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. pkg/query-service/app/metrics/v4/query_builder_test.go:537
- Draft comment:
The test case for handling tags with dots in their names is well-structured and ensures that the query is correctly formatted with backticks. This aligns with the intended fix. - Reason this comment was not posted:
Confidence changes required:0%
The test case added for handling tags with dots in their names is appropriate and follows the pattern of existing test cases. It ensures that the query is correctly formatted with backticks for such tags.
2. pkg/query-service/app/metrics/v4/query_builder_test.go:533
- Draft comment:
Use design tokens or predefined color constants instead of hardcoding color values to maintain consistency in design and theming. This applies to the expected query strings in the test cases. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_2WYZ6GfOD85wua3E
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
reviewed
Dec 30, 2024
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 93864d4 in 55 seconds
More details
- Looked at
335lines of code in3files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. pkg/query-service/app/metrics/v3/query_builder.go:270
- Draft comment:
Consider applyingutils.AddBackTickToFormatTagstotagsbefore the loop to ensure all tags are consistently formatted with backticks. This will prevent any potential inconsistencies iftagsis used elsewhere without backticks. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The change in the code appears deliberate and actually improves the code by applying backticks at the exact point where each tag is used. The old approach of formatting all tags upfront was less precise. The comment is suggesting to revert to a less optimal approach. The new code is cleaner and there's no evidence of any inconsistency issues.
Could there be a performance benefit to formatting all tags at once rather than individually? Could there be edge cases where formatting tags individually produces different results?
The performance difference would be negligible since this is just string manipulation. The individual formatting approach is actually safer as it ensures each tag is formatted exactly when needed, avoiding any potential issues with tags being used in different contexts.
Delete the comment. The code change is an improvement that formats tags at the point of use, which is cleaner and more precise than formatting all tags upfront.
Workflow ID: wflow_uDDj6CSywdJupZxo
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
srikanthccv
reviewed
Jan 2, 2025
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on ebf50fc in 52 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. frontend/src/constants/env.ts:3
- Draft comment:
Optional chaining withprocess.envis unnecessary sinceprocess.envis always defined in Node.js. Consider removing the?.operators for cleaner code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is technically correct - in Node.js, process.env is always defined. However, this code is in the frontend directory and appears to be client-side code. In browser environments, process may be undefined unless explicitly provided. The comment makes an incorrect assumption about the execution environment.
Am I certain this is client-side code? Could this be used in both Node.js and browser environments?
The file path frontend/src/constants/env.ts strongly indicates this is client-side code. Even if used in both environments, the optional chaining makes the code more robust for browser usage.
The comment should be deleted because it incorrectly assumes Node.js environment for frontend code where optional chaining is actually beneficial.
2. frontend/src/constants/env.ts:2
- Draft comment:
Avoid using inline styles in React components. Use external stylesheets, CSS classes, or styled components instead. - Reason this comment was not posted:
Comment was not on a valid diff hunk.
Workflow ID: wflow_QhJgSWEohiDR3WNB
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
@aniketio-ctrl please revert unwanted changes. |
hey already reverted , pushed those by mistakes |
srikanthccv
approved these changes
Jan 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
…for v3 and v4 | 6069
Summary
Click house query generated with tags that contains dot in their key name was throwing error, encapsulated that specific keys with backticks.
Related Issues / PR's
#6069
Screenshots
NA
Affected Areas and Manually Tested Areas
Clickhouse queries with tags that have dot in them wont work
Important
Fix ClickHouse query errors by adding backticks to tags with dots in query building and formatting functions.
query_builder.go,clauses.go, andsub_query.go.AddBackTickToFormatTagandAddBackTickToFormatTagsinformat.goto handle tags with dots.groupingSets,groupBy,groupSelect, andorderByinquery_builder.goto use backticks for tags.GroupingSetsByAttributeKeyTags,GroupByAttributeKeyTags, andOrderByAttributeKeyTagsinclauses.goto add backticks.PrepareTimeseriesFilterQueryandPrepareTimeseriesFilterQueryV3insub_query.goto format tags with backticks.query_builder_test.go,table_test.go, andtimeseries_test.gofor cumulative and delta metrics to verify queries with tags containing dots.This description was created by
for ebf50fc. It will automatically update as commits are pushed.