feat(eap): Add NULL-safe division and ConditionalFormula support for formulas#7697
Open
feat(eap): Add NULL-safe division and ConditionalFormula support for formulas#7697
Conversation
Add test coverage for querying EAP with OCCURRENCE item type to calculate hourly event rates. The tests validate: - Counting occurrences grouped by group_id (issue) - Calculating hourly rates using division formulas (count / WEEK_IN_HOURS) - Taking P95 of hourly_event_rate across issues - Conditional aggregations based on issue age (old vs new issues) The hourly event rate formula tested: - Old issues (>1 week): rate = event_count / WEEK_IN_HOURS - New issues (<1 week): rate = event_count / hours_since_first_seen Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ed query patterns Updated tests to properly categorize: SUPPORTED: - Count and min(timestamp) per group - Division by static literal (count / WEEK_IN_HOURS) - P95 of pre-computed attribute values UNSUPPORTED (xfail): - Division by nested formula containing aggregate (returns null) - Conditional expression with aggregate in condition (no 'if' operator) - P95 of calculated aggregate formula across groups (nested aggregation) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Adds test_countif_with_aggregate_in_condition to demonstrate the core limitation: we cannot use an aggregate result (e.g., min(timestamp) as first_seen) in a conditional aggregation filter. What WORKS: - countIf(timestamp > X) - filtering individual rows What DOESN'T WORK: - countIf(min(timestamp) > X) - condition uses an aggregate This is marked xfail as AttributeConditionalAggregation.filter can only compare row-level attributes to literals, not aggregate results. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…formulas Phase 1 - NULL-safe division: - Modified _formula_to_expression() to use if(isNull(right) OR right = 0, default, left / right) when default_value is specified, protecting against NULL/zero divisors - This fixes hourly rate calculations where divisor uses aggregates like min(timestamp) Phase 2 - ConditionalFormula support (code ready, waiting for sentry-protos PR #173): - Added _has_proto_field() helper to safely check for proto fields that may not exist yet - Added COMPARISON_OP_TO_EXPR mapping for conditional formula comparison operators - Added _conditional_formula_to_expression() using existing if_cond() DSL - Updated _column_to_expression() to handle conditional_formula field - Updated _get_reliability_context_columns() for conditional formulas - Updated ColumnWrapper.accept() in proto_visitor.py to traverse conditional formula children Tests: - test_hourly_rate_with_dynamic_divisor_using_aggregate now passes - test_conditional_rate_based_on_aggregate_first_seen updated to use ConditionalFormula proto structure (skips until proto is available) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
phacops
changed the title
kylemumma
reviewed
Feb 5, 2026
Member
kylemumma
left a comment
kylemumma
left a comment
There was a problem hiding this comment.
is there any way for us to re-use "conditional to expression" logic defined in another endpoint? it seems like a code smell to have this translation separately defined in both endpoints
Comment on lines
+27
to
+41
| def _has_proto_field(proto: ProtobufMessage, field_name: str) -> bool: | ||
| """ | ||
| Safely check if a proto message has a field set. | ||
|
|
||
| This handles the case where the field doesn't exist in the proto definition yet | ||
| (e.g., when preparing for a new proto field that will be added later). | ||
| HasField() throws ValueError if the field doesn't exist, so we catch that. | ||
| """ | ||
| try: | ||
| return proto.HasField(field_name) | ||
| except ValueError: | ||
| # Field doesn't exist in the proto definition | ||
| return False | ||
|
|
||
|
|
Member
There was a problem hiding this comment.
I don't think this is right, it will return false if the field exists but isnt set https://googleapis.dev/python/protobuf/latest/google/protobuf/message.html#google.protobuf.message.Message.HasField
Comment on lines
+84
to
+99
|
|
||
| def _has_proto_field(proto: Any, field_name: str) -> bool: | ||
| """ | ||
| Safely check if a proto message has a field set. | ||
|
|
||
| This handles the case where the field doesn't exist in the proto definition yet | ||
| (e.g., when preparing for a new proto field that will be added later). | ||
| HasField() throws ValueError if the field doesn't exist, so we catch that. | ||
| """ | ||
| try: | ||
| return bool(proto.HasField(field_name)) | ||
| except ValueError: | ||
| # Field doesn't exist in the proto definition | ||
| return False | ||
|
|
||
|
|
Member
There was a problem hiding this comment.
since you defined this twice across two files, should it be extracted and only have one definition?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds two features to support complex formula calculations in EAP queries:
1. NULL-safe division
When a
BinaryFormulawithOP_DIVIDEhas adefault_valuespecified, the division now safely handles NULL and zero divisors:This enables calculations like
count / hours_since_first_seenwherehours_since_first_seenderives frommin(timestamp).2. ConditionalFormula support (waiting for sentry-protos PR #173)
Adds support for
Column.ConditionalFormulato express conditional expressions:The code is implemented and ready but dormant until the proto is released. Uses
_has_proto_field()helper to safely check for proto fields that don't exist yet.Changes
resolver_trace_item_table.py:_has_proto_field()helper for safe proto field checkingCOMPARISON_OP_TO_EXPRmapping for conditional formula operators_formula_to_expression()for NULL-safe division_conditional_formula_to_expression()using existingif_cond()DSL_column_to_expression()to handleconditional_formula_get_reliability_context_columns()for conditional formulasproto_visitor.py:ColumnWrapper.accept()to traverse conditional formula childrenTest plan
test_hourly_rate_with_dynamic_divisor_using_aggregatepasses (NULL-safe division)test_conditional_rate_based_on_aggregate_first_seenskips until proto available🤖 Generated with Claude Code