Firestore COUNT #1207

tom-andersen · 2023-02-13T16:55:31Z

No description provided.

* Firestore COUNT API * Exempt from go/allstar check (#1173) * Token-changed listeners for iOS AppCheck (#1172) * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Setup Desktop test workflow that detects C++ SDK breakage caused by iOS SDK changes (#1162) * [Bugfx] Fix Nightly Test Report template (#1181) * Reduce number of RewardedAd loads on iOS in CI (#1180) The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id. This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com>

* Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Add self move assignment test * Format * Simplify

* Firestore COUNT API (#1174) * Firestore COUNT API * Exempt from go/allstar check (#1173) * Token-changed listeners for iOS AppCheck (#1172) * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Setup Desktop test workflow that detects C++ SDK breakage caused by iOS SDK changes (#1162) * [Bugfx] Fix Nightly Test Report template (#1181) * Reduce number of RewardedAd loads on iOS in CI (#1180) The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id. This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> * Tomandersen/count test (#1206) * Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Add self move assignment test * Format * Simplify * Pretty --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com>

* Firestore COUNT API (#1174) * Firestore COUNT API * Exempt from go/allstar check (#1173) * Token-changed listeners for iOS AppCheck (#1172) * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Setup Desktop test workflow that detects C++ SDK breakage caused by iOS SDK changes (#1162) * [Bugfx] Fix Nightly Test Report template (#1181) * Reduce number of RewardedAd loads on iOS in CI (#1180) The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id. This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> * Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Firestore COUNT API * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Disable getSessionId test on emulators. (#1193) * Disable getSessionId test on Android emulator. * Also skip on iOS simulator. * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format * Stub Android Impl * Tomandersen/count test (#1206) * Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Add self move assignment test * Format * Simplify * JNI Count Implementation * Pretty * Fix according PR comments * Pretty --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> Co-authored-by: Jon Simantov <jsimantov@google.com>

firestore/integration_test_internal/src/query_test.cc

github-actions · 2023-03-23T20:41:27Z

Integration test with FLAKINESS (succeeded after retry)

Requested by @tom-andersen on commit 9afe041
Last updated: Tue Apr 4 19:52 PDT 2023
View integration test log & download artifacts

Failures	Configs
firestore	[TEST] [FLAKINESS] [Android] [1/3 os: windows] [1/2 android_device: android_target] (1 failed tests) QueryNetworkTest.EnableDisableNetwork

Add flaky tests to go/fpl-cpp-flake-tracker

release_build_files/readme.md

firestore/src/include/firebase/firestore/query.h

firestore/src/include/firebase/firestore/aggregate_source.h

firestore/src/include/firebase/firestore/aggregate_query_snapshot.h

firestore/src/main/aggregate_query_snapshot_main.cc

firestore/src/android/promise_android.h

dconeybe · 2023-03-29T19:20:46Z

firestore/src/android/load_bundle_task_progress_android.cc

@@ -25,7 +25,6 @@ namespace firestore {
 namespace {

 using jni::Class;
-using jni::Constructor;


Is this change related to count queries? If not, please revert it.

Do you have a backlog item for removing unused import and using statements?

Nope, there is no backlog item for this. IMO it doesn't really matter and including it in this PR adds noise. I'm sure there are unused includes and using statements scattered everywhere in our code base!

For small things like this, we should probably take the Boy Scout Rule approach. Especially, if we can't justify tracking in backlog.

I tend to disagree with the "Boy Scout Rule" in this context. IMO PRs should be laser focused on what their purpose is. As unrelated changes bleed into a PR it makes it more difficult to review and understand after the fact. Also, if the PR needed to be reverted then those "unrelated" changes would also get reverted. You can always open a separate PR for unrelated cleanups like this and I would happily review them expeditiously. After all, I'm a huge fan of cleaning up the code base!

I can accept excluding the change since it is in a separate file, that is otherwise untouched.

I am however concerned that we are creating unnecessary barriers that will promote tech debt. I am not about to make a separate PR to just remove a couple of imports since there is higher priority work. Which is why this barrier to be laser focused is counter productive when applied to trivial changes IMO. Changes are likely never done, and will likely create a separate cognitive burden to readers of the code.

I don't find there is a cognitive burden to pruning imports in PRs since the CI would fail if it was required. But I suppose with a mindset of being laser focused, then any deviation creates questions and cognitive load.

I do find unused imports create a burden to the reader of code. I was looking for an example of how to use jni::Constructor in our code base, and this import was a red herring (among others) that made my work with the code slower.

firestore/src/android/aggregate_query_snapshot_android.cc

firestore/src/android/aggregate_query_android.h

dconeybe · 2023-03-29T19:31:22Z

firestore/integration_test_internal/src/query_test.cc

@@ -236,11 +236,18 @@ TEST_F(QueryTest, TestKeyOrderIsDescendingForDescendingInequality) {
                  {"e", {{"foo", FieldValue::Double(21.0)}}},
                  {"f", {{"foo", FieldValue::Integer(66)}}},
                  {"g", {{"foo", FieldValue::Double(66.0)}}}});
-  QuerySnapshot snapshot = ReadDocuments(
+  const Query query =


@ehsannas Here is the testing code we'd like your opinion on. This PR updates each query test to also run a count query.

My thought was that this isn't the right approach because (a) this reduces the focus of these tests on query features, (b) adds coverage for various query features that should be tested on the underlying SDKs, and (c) does not scale to additional aggregations like SUM and AVERAGE.

@tom-andersen Argues that (a) this is great test coverage to have, (b) does not need to be replicated for SUM and AVERAGE, and (c) the integration tests are already not very focussed so the possible reduction in the tests' "focus" is worth it. (please correct me if I've mis-represented your position, Tom).

For purpose of discussion, I think there are 3 different issues at play here:

Should the wrapper SDK do a thorough integration test, or simply verify it correctly wraps underlying SDK and defer to the underlying SDK to do a thorough test? A lot of tests (not just for count) could be simplified or removed if we defer to underlying SDK.

Currently the C++ SDK has integration tests, in the sense that it requires making real calls to emulator/backend to function, thereby exercising many layers of code simultaneously. I think what we really want is unit tests that can test units of code in isolation by mocking out dependencies. I think this is fundamental to our disagreement, in the sense that I think Denver is applying advice normally given to unit tests that can have a narrow focus, when in reality these are integration tests that exercise the full stack.

Contrast between Unit Testing and Integration Testing:

Unit Testing:

Focused

Understandable

Fast

Deterministic

Resilient

Hermetic

Integration Testing:

Not Focused and/or Not Hermetic

Here are some of my thoughts:

Integration tests tell a story of multiple interactions (create data, query, then delete, makes sure is deleted, etc). When considering query operators, a part of that story is what documents are returned and does the count match.

If an integration test fails, there are many layers that could be at fault (lack of focus). A good integration test will give hints to isolate problem. If the count is wrong, the next natural question will be if the returned documents are wrong as well. If both are wrong, maybe there is an error in how we construct request to backend (since they share logic for serializing filter operators).

Our test suite is slow (mainly because it is written as integration tests). Breaking tests up into smaller tests will mean more time is spent on test setup/teardown of populating documents and other tasks. This adds up, and we should course correct. Some of this might be mitigated by tests sharing setup/teardown environment, but that could also lead to interfering tests.

If we see tests through the lens of exercising filter operators, then count and returned documents both belong as a validation step. It makes sense to have count and query in close proximity with respect to a filter operator. When new filter operators are added, they should be exercised against both, and the close proximity will help enforce this pattern. Having tests in separate files will increase the likelihood of omitting a test.

# Conflicts: # release_build_files/readme.md

dconeybe

Thanks for this porting work! I've added the "Tests Requested: Quick" label to trigger the integration tests (they aren't run automatically because they are quite resource intensive). Once they pass you have my LGTM to merge.

dconeybe · 2023-04-04T17:03:12Z

@tom-andersen Note that the "Checks / check_integration_test_labels (pull_request)" label will change from "fail" to "pass" automatically once all of the integration tests pass. It is there merely to prevent merging until the integration tests pass.

* Firestore COUNT API (#1174) * Firestore COUNT API * Exempt from go/allstar check (#1173) * Token-changed listeners for iOS AppCheck (#1172) * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Setup Desktop test workflow that detects C++ SDK breakage caused by iOS SDK changes (#1162) * [Bugfx] Fix Nightly Test Report template (#1181) * Reduce number of RewardedAd loads on iOS in CI (#1180) The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id. This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> * Tomandersen/count test (#1206) * Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Add self move assignment test * Format * Simplify * PR Followup (#1237) * Firestore COUNT API (#1174) * Firestore COUNT API * Exempt from go/allstar check (#1173) * Token-changed listeners for iOS AppCheck (#1172) * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Setup Desktop test workflow that detects C++ SDK breakage caused by iOS SDK changes (#1162) * [Bugfx] Fix Nightly Test Report template (#1181) * Reduce number of RewardedAd loads on iOS in CI (#1180) The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id. This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> * Tomandersen/count test (#1206) * Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Add self move assignment test * Format * Simplify * Pretty --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> * Tomandersen/count api (#1236) * Firestore COUNT API (#1174) * Firestore COUNT API * Exempt from go/allstar check (#1173) * Token-changed listeners for iOS AppCheck (#1172) * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Setup Desktop test workflow that detects C++ SDK breakage caused by iOS SDK changes (#1162) * [Bugfx] Fix Nightly Test Report template (#1181) * Reduce number of RewardedAd loads on iOS in CI (#1180) The GMA backend doesn't whitelist iOS devices running in CI which means number of ads that can be served to iOS in CI is restricted. This is true even when using the prescribed Demo Ad Unit Id. This PR reduces the number of ads we load on iOS in an attempt to minimize the chance of encountering NoFillErrors and push our CI to green. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> * Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Firestore COUNT API * [macOS sandbox mode] Application group name mangling for semaphores (#1167) When macOS Sandbox mode is enabled, macOS requires that semaphores have a name that is prefixed by the App's Group Name. If the semaphore's name doesn't match this convention then its creation fails. Unfortunately there's no official way for the SDK to query the app's group name at runtime, so we can't automatically mangle the semaphore names. Instead I've updated the SDK to use an Info.plist property named FBAppGroupEntitlementName on macOS. If that property is present then the SDK will use it's value to prefix the semaphore names. As an additional issue, the SDK attempted to detect semaphore creation errors by comparing the semaphore handle to nil. But in the case of macOS, a semaphore creation error returns SEM_FAILED which is 0xFFFFFFFFFFFFFFFF, not nil. And on Linux, sem_init returns -1. I've updated the corresponding platform implementations to detect the correct values for these errors. * Firestore COUNT implementation (WIP) * Firestore COUNT implementation (WIP) * Fix linting * Fix linting * Fix linking * Disable getSessionId test on emulators. (#1193) * Disable getSessionId test on Android emulator. * Also skip on iOS simulator. * Cleanup * Fix release notes * Format * Fixes from review * Formatting * Responding to PR comments. * Add Hash * Add Hash * Fix copyright year * Rename * Format * Rename * Fixup constructor/assignment parameter naming. * Format * Stub Android Impl * Tomandersen/count test (#1206) * Fix linking * Cleanup * Responding to PR comments. * Rename * Rename * Fixup constructor/assignment parameter naming. * Add Test * Format * Add test * Add test * Format * Recognize NaN filter probelm * Add tests * Add tests * Add tests * Test is_valid() * Add constructor and assignment tests. * Rename variable * Format * Remove extra semicolon * Add self move assignment test * Format * Simplify * JNI Count Implementation * Pretty * Fix according PR comments * Pretty --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> Co-authored-by: Jon Simantov <jsimantov@google.com> * Fix * Revert Query Test changes. * Add separate file for count tests. * Changes from PR comments * Pretty --------- Co-authored-by: chkuang-g <31869252+chkuang-g@users.noreply.github.com> Co-authored-by: Matthew Hyndman <almostmatt@google.com> Co-authored-by: DellaBitta <DellaBitta@users.noreply.github.com> Co-authored-by: Mou Sun <69009538+sunmou99@users.noreply.github.com> Co-authored-by: Jon Simantov <jsimantov@google.com>

tom-andersen and others added 2 commits March 13, 2023 12:27

tom-andersen force-pushed the count branch from d403ded to cf18ec7 Compare March 13, 2023 20:42

tom-andersen and others added 3 commits March 16, 2023 16:28

Merge branch 'main' into count

ae48d8c

tom-andersen marked this pull request as ready for review March 20, 2023 19:21

tom-andersen requested a review from ehsannas March 20, 2023 19:22

tom-andersen assigned ehsannas Mar 20, 2023

tom-andersen requested a review from dconeybe March 20, 2023 19:22

ehsannas assigned dconeybe Mar 22, 2023

cherylEnkidu reviewed Mar 23, 2023

View reviewed changes

firestore/integration_test_internal/src/query_test.cc Outdated Show resolved Hide resolved

Fix

fa81ff9

tom-andersen added the tests-requested: quick Trigger a quick set of integration tests. label Mar 23, 2023

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Mar 23, 2023

github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Mar 23, 2023

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Mar 23, 2023

tom-andersen requested a review from cherylEnkidu March 24, 2023 17:09

dconeybe requested changes Mar 29, 2023

View reviewed changes

dconeybe assigned tom-andersen and unassigned dconeybe Mar 29, 2023

tom-andersen added 5 commits March 31, 2023 09:26

Merge remote-tracking branch 'origin/main' into count

01f9e31

# Conflicts: # release_build_files/readme.md

Revert Query Test changes.

9d88ffe

Add separate file for count tests.

e6c018c

Changes from PR comments

9a747b5

Pretty

378a898

tom-andersen requested a review from dconeybe April 4, 2023 16:43

tom-andersen assigned dconeybe Apr 4, 2023

tom-andersen removed their assignment Apr 4, 2023

dconeybe added tests-requested: quick Trigger a quick set of integration tests. and removed tests: succeeded This PR's integration tests succeeded. labels Apr 4, 2023

github-actions bot added tests: in-progress This PR's integration tests are in progress. and removed tests-requested: quick Trigger a quick set of integration tests. labels Apr 4, 2023

dconeybe approved these changes Apr 4, 2023

View reviewed changes

dconeybe assigned tom-andersen and unassigned dconeybe Apr 4, 2023

github-actions bot added the tests: succeeded This PR's integration tests succeeded. label Apr 4, 2023

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 4, 2023

tom-andersen merged commit 9afe041 into main Apr 5, 2023

github-actions bot added tests: in-progress This PR's integration tests are in progress. tests: succeeded This PR's integration tests succeeded. and removed tests: succeeded This PR's integration tests succeeded. labels Apr 5, 2023

firebase-workflow-trigger bot removed the tests: in-progress This PR's integration tests are in progress. label Apr 5, 2023

firebase locked and limited conversation to collaborators May 5, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Firestore COUNT #1207

Firestore COUNT #1207

tom-andersen commented Feb 13, 2023

github-actions bot commented Mar 23, 2023 •

edited

Loading

dconeybe Mar 29, 2023

tom-andersen Mar 31, 2023

dconeybe Apr 1, 2023

tom-andersen Apr 3, 2023

dconeybe Apr 3, 2023

tom-andersen Apr 3, 2023

dconeybe Mar 29, 2023

tom-andersen Mar 29, 2023 •

edited

Loading

dconeybe left a comment

dconeybe commented Apr 4, 2023

Firestore COUNT #1207

Firestore COUNT #1207

Conversation

tom-andersen commented Feb 13, 2023

github-actions bot commented Mar 23, 2023 • edited Loading

Integration test with FLAKINESS (succeeded after retry)

dconeybe Mar 29, 2023

Choose a reason for hiding this comment

tom-andersen Mar 31, 2023

Choose a reason for hiding this comment

dconeybe Apr 1, 2023

Choose a reason for hiding this comment

tom-andersen Apr 3, 2023

Choose a reason for hiding this comment

dconeybe Apr 3, 2023

Choose a reason for hiding this comment

tom-andersen Apr 3, 2023

Choose a reason for hiding this comment

dconeybe Mar 29, 2023

Choose a reason for hiding this comment

tom-andersen Mar 29, 2023 • edited Loading

Choose a reason for hiding this comment

dconeybe left a comment

Choose a reason for hiding this comment

dconeybe commented Apr 4, 2023

github-actions bot commented Mar 23, 2023 •

edited

Loading

tom-andersen Mar 29, 2023 •

edited

Loading