Skip to content

Conversation

@kylemumma
Copy link
Member

@kylemumma kylemumma requested review from a team as code owners January 29, 2026 18:42
# aggregation is deprecated, it gets converted to conditional_aggregation
if state.get_int_config("aggregation_deprecation_enabled", 1):
for expr in in_msg.expressions:
assert expr.WhichOneof("expression") != "aggregation"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably do not want an assert here. If we need to fail a query, we should return a proper exception with a 400 and a useful error message.

@kylemumma
Copy link
Member Author

I switched the assertion to a RuntimeError so that we can include a clearer description of what went wrong.

Regarding in_msg: TimeSeriesRequest, I considered changing the in_msg type to something stricter. It would help us enforce the invariants from converted legacy fields at compile-time, which would be cleaner. However, we’d have to manually update that object every time the protobuf changes. I decided it wasn't worth the maintenance cost, but I'm open to suggestions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants