-
Notifications
You must be signed in to change notification settings - Fork 100
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Aggregation comparison API #4793
Conversation
if err != nil { | ||
return fmt.Errorf("error building query: %w", err) | ||
} | ||
fmt.Println(sqlString, args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Leftover from debugging?
proto/rill/runtime/v1/queries.proto
Outdated
@@ -323,9 +323,11 @@ message MetricsViewAggregationRequest { | |||
// Required | |||
repeated MetricsViewAggregationMeasure measures = 4; | |||
// Optional. Defaults to unsorted | |||
repeated MetricsViewAggregationSort sort = 5; | |||
repeated MetricsViewComparisonSort sort = 5; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you didn't add configuration for the comparison in the MetricsViewAggregationMeasure
type as discussed here? https://www.notion.so/rilldata/Pivot-APIs-improvements-8ee3db49882e4093af85de68198f5984
What should the frontend put in measures
to request a comparison value?
Also, if keeping this, we should rename the type to MetricsViewAggregationSort
to align with the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to this. There is no way to add having filter on comparison otherwise. So might need to build and pass aliases
to buildExpression
for having clause.
Because this is missing, i get this error when i use comparison in having
,
error building query: unknown column filter: total_records__delta_abs
Request object,
{
"metricsView": "AdBids_dimension_expression",
"measures": [{
"name": "total_records"
}],
"dimensions": [{
"name": "publisher"
}],
"having": {
"cond": {
"op": "OPERATION_AND",
"exprs": [{
"cond": {
"op": "OPERATION_GT",
"exprs": [{
"ident": "total_records__delta_abs"
}, {
"val": 0
}]
}
}]
}
},
"timeRange": {
"isoDuration": "P7D"
},
"comparisonTimeRange": {
"isoDuration": "P7D",
"isoOffset": "rill-PW"
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's the next step to adapt it to the new proposal
for _, sf := range q.Result.Schema.Fields { | ||
fmt.Printf("%v ", sf.Name) | ||
} | ||
fmt.Printf("\n") | ||
|
||
for i, row := range q.Result.Data { | ||
for _, sf := range q.Result.Schema.Fields { | ||
fmt.Printf("%v ", row.Fields[sf.Name].AsInterface()) | ||
} | ||
fmt.Printf(" %d \n", i) | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Avoid prints if possible
for _, sf := range q.Result.Schema.Fields { | ||
fmt.Printf("%v ", sf.Name) | ||
} | ||
fmt.Printf("\n") | ||
|
||
for i, row := range q.Result.Data { | ||
for _, sf := range q.Result.Schema.Fields { | ||
fmt.Printf("%v ", row.Fields[sf.Name].AsInterface()) | ||
} | ||
fmt.Printf(" %d \n", i) | ||
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also prints
@egor-ryashin Can you fix the merge conflicts here? |
No description provided.