fix: changes in code to support ch 25.5

[nityanandagohain]

Changes in the trace code to replace the alias columns by the actual column in the builder

---

Important

Replace alias columns with actual column names in SQL queries and update related functions and tests.

• Behavior:
  • Replace alias columns with actual column names in clickhouse_queries.go, query.go, and reader.go.
  • Remove BuildTracesFilter function in query_builder.go and integrate its functionality into BuildTracesFilterQuery.
• Functions:
  • Update getColumnName() in query_builder.go to support alias replacement.
  • Modify BuildTracesFilterQuery() in query_builder.go to handle alias replacement and remove BuildTracesFilter.
• Constants:
  • Add new mappings in constants.go for deprecated and new trace fields.
• Tests:
  • Update tests in query_builder_test.go to reflect changes in column name handling and alias replacement.

^{This description was created by for 6f300d3. You can customize this summary. It will automatically update as commits are pushed.}

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 483eda8 in 2 minutes and 9 seconds. Click for details.

• Reviewed 597 lines of code in 5 files
• Skipped 0 files when reviewing.
• Skipped posting 7 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. pkg/query-service/constants/constants.go:717

• Draft comment:
  The mapping in ChDataTypeMap maps 'int64' to 'Float64'. Please confirm if this is intentional; if not, consider mapping int64 to the proper ClickHouse type.
• 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/constants/constants.go:696

• Draft comment:
  In the init() function, NewStaticFieldsTraces is cloned and then merged with DeprecatedStaticFieldsTraces using maps.Clone and maps.Copy. Consider adding documentation to explain the merging logic and note the Go version dependency.
• 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/constants/constants.go:707

• Draft comment:
  The constant 'maxJSONFlatteningDepth' (line 707) is defined in lowercase whereas most environment key constants are uppercase. Ensure naming consistency if this constant should be exported or clarify that it’s meant for internal use.
• 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/constants/constants.go:656

• Draft comment:
  The OldToNewTraceFieldsMap contains mappings from deprecated field names to new ones. Consider adding more inline documentation to clarify its intended lifecycle and deprecation plan.
• Reason this comment was not posted:
  Comment did not seem useful. Confidence is useful = 0% <= threshold 50% The comment is asking for more inline documentation to clarify the lifecycle and deprecation plan of a mapping. This is a request for additional information and does not provide a specific code suggestion or highlight a potential issue with the code itself. It violates the rule against asking the PR author to explain or ensure things.

5. pkg/query-service/app/traces/v4/query_builder_test.go:203

• Draft comment:
  Typo: "depricated" should be "deprecated" in the test case name.
• 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, I need to consider if this kind of comment is useful enough to keep. The rules say to only keep comments that require clear code changes and to avoid obvious or unimportant comments. A typo in a test case name doesn't affect functionality and is a very minor issue. The typo is real and fixing it would improve code quality slightly. Test names are part of documentation. However, the rules specifically say to avoid obvious and unimportant comments. A typo in a test name is very minor and doesn't impact functionality or maintainability in any meaningful way. The comment should be deleted as it points out a very minor issue that doesn't meaningfully impact code quality or functionality.

6. pkg/query-service/app/traces/v4/query_builder_test.go:217

• Draft comment:
  Typo: "non depricated Columns" should be "non deprecated Columns" in the test case name.
• 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 spelling corrections can be valuable, this is a test case name in a test file. Test case names are internal implementation details that don't affect functionality. The misspelling doesn't impact code behavior or readability significantly. The comment creates noise without adding much value. The spelling error could propagate if other developers copy this test case name pattern. Consistent spelling helps maintain code quality. The benefit of fixing this spelling in a test case name is minimal compared to the noise it adds to the PR review. Test names are internal implementation details that don't propagate to user-facing code. Delete the comment as it addresses a minor spelling issue in a test case name that doesn't meaningfully impact code quality or functionality.

7. pkg/query-service/app/traces/v4/query_builder_test.go:655

• Draft comment:
  There appear to be extra whitespace characters in the SQL string (e.g., double spaces before 'AND parent_span_id' and before 'order by'). Consider cleaning these up for consistency if they are not intentional.
• Reason this comment was not posted:
  Comment was on unchanged code.

Workflow ID: wflow_DvaHbtgIWnaQqFk5

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[eKuG]

LGTM

[srikanthccv]

Should you do this for v3 as well because there are some people who rely on /v3/query_range?

[nityanandagohain]

the v3 query builder is stopped some time back, I am removing the code step by step

signoz/pkg/query-service/app/traces/v3/query_builder.go

Line 361 in 210393e

return "", nil

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 6b106f5 in 1 minute and 54 seconds. Click for details.

• Reviewed 37 lines of code in 2 files
• Skipped 0 files when reviewing.
• Skipped posting 3 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. pkg/query-service/app/traces/v4/query_builder.go:86

• Draft comment:
  Using getColumnName(key, true) in existsSubQueryForFixedColumn now properly replaces deprecated alias columns (CH 25.5). Ensure this change doesn’t accidentally affect non-deprecated or non‐column fields.
• 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 ensure that the change doesn't accidentally affect non-deprecated or non-column fields. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.

2. pkg/query-service/app/traces/v4/query_builder_test.go:317

• Draft comment:
  The test case for exists/not-exists on column keys (httpMethod, serviceName) now expects actual column names. Confirm these tests cover all edge cases for alias replacement.
• 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 tests cover all edge cases, which violates the rule against asking for confirmation or ensuring behavior. It doesn't provide a specific suggestion or point out a specific issue.

3. pkg/query-service/app/traces/v4/query_builder_test.go:317

• Draft comment:
  Typo noticed in the test case name on line 317: "Test exists, nexists". Please confirm if "nexists" is intended or if it should be something like "not exists" 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 = 0% vs. threshold = 50% The comment is suggesting a change that doesn't improve understanding since it's not actually testing "not exists" - it's testing two EXISTS operators. The test name is concise and matches what it's testing. The comment is making assumptions about what the test name should be without understanding the test's purpose. Could the test name be confusing to new developers who might expect to see a "not exists" test based on the name? No - the test case clearly shows it's testing two EXISTS operators in the code. The name is accurate as is. Changing it to "not exists" would actually make it less accurate. Delete this comment. The test name accurately reflects what's being tested (two EXISTS operators), and changing it to "not exists" would make it less accurate.

Workflow ID: wflow_acJR3BZnoHvQbnXw

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[srikanthccv]

the v3 query builder is stopped some time back

Are they don't work or are they redirected to v4 and v3 requests work with v4 query builder code?

[nityanandagohain]

We use the V4 query builder, by default. Previously we allowed the users to use the old v3 query builder which used the old tables with a parameter, but we have stopped writing to those tables and we use the V4 by default.

[srikanthccv]

I don't mean to ask related traces schema migration. I meant it from the API perspective, we have /v3/query_range and /v4/query_range. The difference between them was the metrics query builder, do the requests to /v3/query_range with traces work?

[nityanandagohain]

Yeah, the changes applies to that as well, i.e the keys will be mapped.

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 6f300d3 in 1 minute and 8 seconds. Click for details.

• Reviewed 252 lines of code in 1 files
• Skipped 0 files when reviewing.
• Skipped posting 2 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. pkg/modules/tracefunnel/clickhouse_queries.go:34

• Draft comment:
  You've replaced the alias 'serviceName' with the full column name 'resource_string_service$$name'. Please confirm that this column identifier is valid in CH 25.5 and doesn’t require additional escaping (e.g. quoting) due to the '$$' characters.
• 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 the validity of a column identifier in a specific version of ClickHouse. This falls under asking the author to confirm or double-check something, which is against the rules. Therefore, this comment should not be approved.

2. pkg/modules/tracefunnel/clickhouse_queries.go:409

• Draft comment:
  The substitution of 'serviceName' with 'resource_string_service$$name' is done uniformly. Ensure that changing the column name does not break any downstream logic that may be expecting the original alias.
• 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 ensure that changing the column name does not break any downstream logic. This falls under the rule of not asking the author to ensure behavior is intended or to double-check things. Therefore, this comment should be removed.

Workflow ID: wflow_rBC900WPZQQ6LcDp

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}