-
-
Notifications
You must be signed in to change notification settings - Fork 63
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
fix(eap-api): Snuba receives and returns doubles, while still supporting floats #6782
Conversation
val_double
and val_float
3f05554
to
ecc5a6b
Compare
❌ 1 Tests Failed:
View the top 1 failed tests by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
9ce32e8
to
8ecee4e
Compare
val_double
and val_float
c15bab2
to
8f6cca1
Compare
6b756f9
to
08e2ece
Compare
08e2ece
to
628c371
Compare
3a96a04
to
b7259e9
Compare
@@ -37,6 +37,8 @@ | |||
|
|||
CUSTOM_COLUMN_PREFIX = "__snuba_custom_column__" | |||
|
|||
_FLOATING_POINT_PRECISION = 8 |
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.
A Float32
would provide us with at least 8 digits of precision already I think, we should extend that to 12 if we're going to support Float64
.
Hi I don't really understand what exactly you're trying to do so im finding it hard to review. Do you mind adding more context behind the PR to the description? |
ef01b9a
to
adc9455
Compare
Function.FUNCTION_P99: cf.quantile(0.99)(field, **alias_dict), | ||
Function.FUNCTION_AVG: f.avg(field, **alias_dict), | ||
Function.FUNCTION_P50: f.round( | ||
cf.quantile(0.5)(field), _FLOATING_POINT_PRECISION, **alias_dict |
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.
why do you need to add this _FLOATING_POINT_PRECISION argument?
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.
We still have floating point precision errors :(
#83814) Update plan: 1. Sentry can receive doubles and floats 2. Snuba can receive (from Sentry) and return (to Sentry) doubles and floats. In the case of aggregation, replace float with double. This is fine because Sentry handles the case of receiving doubles already 3. Sentry asks for doubles only This PR addresses step 3. Step 2's PR is here: getsentry/snuba#6782 Note to self: don't merge until step 2's PR is fully deployed --------- Co-authored-by: Rachel Chen <[email protected]>
#83814) Update plan: 1. Sentry can receive doubles and floats 2. Snuba can receive (from Sentry) and return (to Sentry) doubles and floats. In the case of aggregation, replace float with double. This is fine because Sentry handles the case of receiving doubles already 3. Sentry asks for doubles only This PR addresses step 3. Step 2's PR is here: getsentry/snuba#6782 Note to self: don't merge until step 2's PR is fully deployed --------- Co-authored-by: Rachel Chen <[email protected]>
https://github.com/getsentry/eap-planning/issues/148
Update plan:
This PR addresses step 2. Step 1's PR is here: getsentry/sentry#83566
For every test that involved floats, I added a corresponding test with doubles. It ensures backward compatibility by verifying that Snuba can handle both floats and doubles