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.

Sign up for GitHub

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

Jump to bottom

Add setIndexConfiguration for client side indexing #10090

Merged
merged 3 commits into from
Aug 18, 2022
Merged

Add setIndexConfiguration for client side indexing #10090

merged 3 commits into from
Aug 18, 2022

Conversation

cherylEnkidu
Copy link
Contributor

@cherylEnkidu cherylEnkidu commented Aug 10, 2022 •
edited
Loading

Expose client side indexing feature with FIRFirestore.setIndexConfigurationFromJSON and FIRFirestore.setIndexConfigurationFromStream
@google-oss-bot
Copy link

google-oss-bot commented Aug 10, 2022 •
edited
Loading

Size Report 1

Affected Products

  • FirebaseFirestore

    TypeBase (160accf)Merge (0006d15)Diff
    CocoaPods6.94 MB6.96 MB+17.0 kB (+0.2%)

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/GmbcjYJXnM.html
@google-oss-bot
Copy link

google-oss-bot commented Aug 10, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • FirebaseFirestore-iOS-FirebaseFirestore.framework

    Overall coverage changed from 88.58% (160accf) to 88.55% (0006d15) by -0.03%.

    11 individual files with coverage change

    FilenameBase (160accf)Merge (0006d15)Diff
    bundle_serializer.cc90.92%91.42%+0.50%
    exception.cc84.21%23.68%-60.53%
    exception_apple.mm65.52%96.55%+31.03%
    firestore.cc95.03%92.13%-2.90%
    firestore_client.cc98.81%98.83%+0.02%
    FIRFirestore.mm91.43%87.87%-3.56%
    json_reader.cc?87.50%?
    leveldb_index_manager.cc96.17%97.57%+1.40%
    leveldb_key.cc98.14%98.82%+0.69%
    local_serializer.cc87.19%87.74%+0.56%
    ordered_code.cc94.39%93.90%-0.49%

Test Logs

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/ajAOD0SKTe.html
@cherylEnkidu cherylEnkidu force-pushed the cheryllin/indexAPI branch 11 times, most recently from cb80f34 to 3a17222 Compare August 12, 2022 18:58
@cherylEnkidu cherylEnkidu requested a review from wu-hui August 12, 2022 23:18
wu-hui
wu-hui reviewed Aug 15, 2022
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not finished with review yet, but thought it'd good to give some early feedback.
Firestore/Example/Tests/Integration/API/FIRIndexingTests.mm Outdated Show resolved Hide resolved
Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h Outdated Show resolved Hide resolved
Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h Outdated Show resolved Hide resolved
Firestore/Source/Public/FirebaseFirestore/FIRFirestore.h Show resolved Hide resolved
Firestore/core/src/api/firestore.cc Outdated Show resolved Hide resolved
Firestore/core/src/api/firestore.cc Outdated Show resolved Hide resolved
Firestore/Source/API/FIRFirestore.mm Show resolved Hide resolved
Firestore/core/src/local/local_store.cc Show resolved Hide resolved
wu-hui
wu-hui reviewed Aug 16, 2022
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished review, overall good work, we are close to the finish line!
Firestore/core/src/api/firestore.cc Outdated Show resolved Hide resolved
Firestore/core/src/api/firestore.cc Outdated Show resolved Hide resolved
Firestore/core/src/api/firestore.cc Show resolved Hide resolved
Firestore/core/test/unit/local/local_store_test.cc Show resolved Hide resolved
Firestore/core/test/unit/local/leveldb_local_store_test.cc Outdated Show resolved Hide resolved
@wu-hui wu-hui removed their assignment Aug 16, 2022
@cherylEnkidu cherylEnkidu removed their assignment Aug 17, 2022
wu-hui
wu-hui approved these changes Aug 17, 2022
View reviewed changes
Copy link
Contributor

@wu-hui wu-hui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you forgot a comment about using std::move, but LGTM!

Congrats to making it to the finish line!
@wu-hui wu-hui assigned cherylEnkidu and unassigned wu-hui Aug 17, 2022
@cherylEnkidu cherylEnkidu merged commit cc9c626 into master Aug 18, 2022
@cherylEnkidu cherylEnkidu deleted the cheryllin/indexAPI branch August 18, 2022 14:51
charlotteliang pushed a commit that referenced this pull request Aug 31, 2022
@cherylEnkidu @wu-hui @charlotteliang
Add setIndexConfiguration for client side indexing (#10090)
88ac246 
* Add setIndexConfiguration for client side indexing

* Address feedback

* Templates in headers

Co-authored-by: Wu-Hui <wu.hui.github@gmail.com>
@firebase firebase locked and limited conversation to collaborators Sep 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api: firestore public-api-change
3 participants
@cherylEnkidu @google-oss-bot @wu-hui