Skip to content
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

Sum aggregations for long values precision #50538

Closed
wants to merge 4 commits into from

Conversation

hackerwin7
Copy link

Create long type for InternalSum and SumAggregator to process the long field type of values without loosing precision.

relates #50503

Change-Id: I6e4b1dfd590aea68dd092318edb11a2baa2f9d65
Change-Id: Ied28bb7b561fd58fdb438199a9be85ea91dc5536
Change-Id: Iebd02310792eac05edb661816e1a455d148db23f
Change-Id: I4b93e948f0130727531cbbf9020168209e486941
@ghost
Copy link

ghost commented Dec 31, 2019

Hi @hackerwin7, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in your Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

@hackerwin7
Copy link
Author

retest

@hackerwin7
Copy link
Author

hackerwin7 commented Dec 31, 2019

ping @colings86 @jpountz

@cbuescher cbuescher added the :Analytics/Aggregations Aggregations label Jan 2, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-analytics-geo (:Analytics/Aggregations)

@polyfractal
Copy link
Contributor

Hi @hackerwin7, thanks for the PR :) Please don't ping individual members though, when tickets are labeled it will ping the correct team to take a look.

I'm going to mark this ticket for team discussion, but I suspect we won't be able to merge it. There is some discussion about supporting more precise aggregations in this issue, for background: #46934

There are a few issues which make this problematic:

  • Doubles are used everywhere in aggregations for several reasons, but one prominent reason is that it reduces the chance of overflows. With this change, a user could easily overflow the long and not realize
  • Consistency. Since doubles are used everywhere, having just the sum agg switch behavior would be very confusing
  • This breaks backwards compatibility, both at the wire transport level (fixable) but also in behavior. E.g. a sum agg would start to behave differently on the same data after upgrading. This is also fixable with flags and such, but again goes back to consistency... if we start to introduce "precise" aggs we should really support it for all metrics which is a much larger undertaking.
  • Better, but potentially not good enough. E.g. the discussion in Add BigDecimal data type #46934 shows that some folks want/need much more robust control over metric precision. Introducing a change like this is more of a bandaid than a fix that would satisfy many use-cases, but still carries maintenance cost.

Marking as team-discuss to see what the rest of the analytics team has to say!

@polyfractal
Copy link
Contributor

Related: #9545 and #43258

@hackerwin7
Copy link
Author

@polyfractal
Thanks for your reply, after reviewing the discuss #46934 , what the future works in Elastic for support to compelete accuracy of long field type sum aggregations or just have no plan on this?

@polyfractal
Copy link
Contributor

@hackerwin7 at the moment, no plans at all... just something we've been discussing. That's the main reason I didn't close this PR but marked it as team-discuss to see what the rest of the team thought. Sometimes small incremental improvements are better than nothing at all :)

@rjernst rjernst added the Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) label May 4, 2020
@elasticsearchmachine elasticsearchmachine changed the base branch from master to main July 22, 2022 23:14
@Laurianti
Copy link

Any news here?

@nik9000
Copy link
Member

nik9000 commented Feb 15, 2024

Actually yes! ESQL supports sum using long precision. FROM foo | STATS SUM(long_field) will sum a long.

@nik9000 nik9000 closed this Feb 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Analytics/Aggregations Aggregations Team:Analytics Meta label for analytical engine team (ESQL/Aggs/Geo) team-discuss
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants