Export fixes #3193
Export fixes #3193
Conversation
| type forExportKey struct{} | ||
|
|
There was a problem hiding this comment.
Can we pass this as an explicit parameter or property instead? Context values are similar to reflection, best only to use them if truly necessary
| colName, err := metricsViewDimensionToSafeColumn(mv, q.DimensionName) | ||
| if err != nil { | ||
| return "", nil, err | ||
| } | ||
|
|
||
| selectCols := []string{colName} | ||
| for _, m := range ms { | ||
| expr := fmt.Sprintf(`%s as %s`, m.Expression, safeName(m.Name)) | ||
| expr := fmt.Sprintf(`%s as %s`, m.Expression, safeName(q.measureNameToLabel[m.Name])) |
There was a problem hiding this comment.
Measure labels are not guaranteed to be unique. They can't be used inside the query for ordering/filtering/etc.
They should only be used at the export stage or in the query passed to COPY TO (where DuckDB will add numbers to disambiguate).
| safeName(q.measureNameToLabel[m.Name]), | ||
| safeName(q.measureNameToLabel[m.Name]+"__previous"), | ||
| safeName(q.measureNameToLabel[m.Name]+"__delta_abs"), | ||
| safeName(q.measureNameToLabel[m.Name]+"__delta_rel"), |
There was a problem hiding this comment.
Do human-friendly formatting like Label (prev), Label (Δ), Label (Δ%)
|
@pjain1 : Do we still need these fixes ? Lets get the comments & conflicts resolved or close the PR if its not required anymore. |
|
Closing since the limit issue was fixed in another PR, and the headers issue will be addressed in a separate PR |
Fixes #3178 and #3177