Skip to content

[WIP] feat: Add AggregateField and Sum with Query - #1705

Open
KKimj wants to merge 6 commits into
firebase:mainfrom
KKimj:feat/add-aggregate-sum
Open

[WIP] feat: Add AggregateField and Sum with Query#1705
KKimj wants to merge 6 commits into
firebase:mainfrom
KKimj:feat/add-aggregate-sum

Conversation

@KKimj

@KKimj KKimj commented Apr 1, 2025

Copy link
Copy Markdown

#1703

Description

Provide details of the change, and generalize the change in the PR title above.


Testing

Describe how you've tested these changes. Link any manually triggered Integration tests or CPP binary SDK Packaging Github Action workflows, if applicable.


Type of Change

Place an x the applicable box:

  • Bug fix. Add the issue # below if applicable.
  • New feature. A non-breaking change which adds functionality.
  • Other, such as a build process or documentation change.

Notes

  • Bug fixes and feature changes require an update to the Release Notes section of release_build_files/readme.md.
  • Read the contribution guidelines CONTRIBUTING.md.
  • Changes to the public API require an internal API review. If you'd like to help us make Firebase APIs better, please propose your change in a feature request so that we can discuss it together.

@jwinarske

jwinarske commented Sep 2, 2026

Copy link
Copy Markdown

One thing to fix before this goes further — QueryInternal::Aggregate in
firestore/src/main/query_main.cc:

AggregateQuery QueryInternal::Aggregate(const AggregateField& aggregate_field) {
  // TODO: Implement Aggregate
  // return MakePublic(aggregate_field);
}

That falls off the end of a non-void function, which is UB — with optimization
on you get whatever happens to be in the return slot, and AggregateQuery's
destructor then runs on it. It's also the desktop path, so as it stands this PR
adds SUM for Android and leaves desktop worse than untouched.

The good news is that the desktop implementation is small. The vendored core
already has everything:

api::Query::Aggregate(std::vector<model::AggregateField>&&) const
api::AggregateQuery::GetAggregate(callback<StatusOr<model::ObjectValue>>)
model::AggregateField::OpKind { Sum, Avg, Count }

So Aggregate is a matter of building the AggregateField vector with an
alias, calling query_.Aggregate(...), and reading the result back out of the
ObjectValue by that alias in AggregateQueryInternal::Get — the existing
Get uses the count-specific Get(StatusOr<int64_t>) callback, which won't
serve a sum.

Two details that cost me time: a sum of integers comes back as
integer_value and a sum of doubles as double_value, so you need both; and
a missing alias means the aggregation matched nothing rather than summed to
zero.

I have this working on desktop against the Firestore emulator as a downstream
patch. It's desktop-only and doesn't use the AggregateField public shape this
PR introduces, so it isn't a drop-in, but happy to share it if it's useful.

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.

2 participants