fix: add canonical name aliasing for select fields#10860
fix: add canonical name aliasing for select fields#10860tushar-signoz wants to merge 3 commits intomainfrom
Conversation
tushar-signoz
changed the title
tushar-signoz
changed the title
tushar-signoz
force-pushed
the
tvats-alias-select-columns
branch
from
8bd696d to
f1cbc80
Compare
nityanandagohain
left a comment
nityanandagohain
left a comment
There was a problem hiding this comment.
Thanks for the PR, have some comments and we should definately add integration test in this PR
|
|
||
| for idx, ptr := range slots { | ||
| name := colNames[idx] | ||
| key := telemetrytypes.GetFieldKeyFromCanonicalName(colNames[idx]) |
There was a problem hiding this comment.
This is running (rows * column) number of times we should move it out and make it run just column number of times.
| // 3 cases: | ||
| // 1. log.timestamp:number in select fields; we skip | ||
| // 2. timestamp is present in select fields; we skip | ||
| // 3. attribute.timestamp in select fields; we add it. |
There was a problem hiding this comment.
but in the UI we don't have support for this right as of now? so with this change it will return timestamp as well as timestamp in attributes. Did you check if the existing UI breaks and needs some handling before we merge this ?
There was a problem hiding this comment.
This is specifically for api users. signoz UI doesn't specify selectFields
There was a problem hiding this comment.
In trace ui it does specify select fields right ?
| ) { | ||
| var toAdd []telemetrytypes.TelemetryFieldKey | ||
| for i := range defaultSelectFields { | ||
| if !isDefaultSelectFieldCovered(&defaultSelectFields[i], query.SelectFields) { |
There was a problem hiding this comment.
nit: create a selectFields map will be better ?
|
|
||
| fieldExpression, fieldForErr := fm.FieldFor(ctx, startNs, endNs, field) | ||
| if errors.Is(fieldForErr, qbtypes.ErrColumnNotFound) { | ||
| if fieldForErr != nil { |
There was a problem hiding this comment.
so the previous change of running the else case if there was any other error was incorrect ?
| sb.SelectMore(fmt.Sprintf("`%s` AS `;%s;`", LogsV2ScopeNameColumn, LogsV2ScopeNameColumn)) | ||
| sb.SelectMore(fmt.Sprintf("`%s` AS `;%s;`", LogsV2ScopeVersionColumn, LogsV2ScopeVersionColumn)) | ||
| sb.SelectMore(fmt.Sprintf("`%s` AS `;%s;`", LogsV2BodyColumn, LogsV2BodyColumn)) | ||
| if querybuilder.BodyJSONQueryEnabled { |
There was a problem hiding this comment.
in this case I don't think we should select both, we should select old or new. Else handling might be required somewhere else ?
nityanandagohain
left a comment
nityanandagohain
left a comment
There was a problem hiding this comment.
- What happens if there is a field
field;name. Didn't see a testcase for that
| // A select field covers a default field when the names match and the select field's | ||
| // context is either unspecified or equal to the default field's context. | ||
| // 3 cases: | ||
| // 1. log.timestamp:number in select fields; we skip |
There was a problem hiding this comment.
If by mistake someone add log.timestamp:string it will be converted to number right, can you add that we dont consider the type and why.
Also add 1/2 unit tests.
2 participants
Summary
context;name;datatype) so the querier can correctly map result columns back to their telemetry field keys, even when multiple fields share the same short name.key.Text()when two columns resolve to the same simple name.MaybeAddDefaultSelectFieldshelper to prepend default fields (id, timestamp) to list queries only when not already present.ErrColumnNotFounderrors fromFieldForinCollisionHandledFinalExprinstead of silently swallowing them.Test plan
json_stmt_builder_test.goandstmt_builder_test.goto match new aliased column names🤖 Generated with Claude Code