add update time to FieldIndex #2984

thebrianchen · 2021-09-17T20:39:58Z

Incremental PR to add storing and retrieving last update time from FieldIndex. Also renamed collectionId to collectionGroup on fieldIndex.

google-oss-bot · 2021-09-17T20:50:13Z

Coverage Report

Affected SDKs

firebase-firestore

SDK overall coverage changed from 43.10% (e445aca) to 43.16% (38d628d2) by +0.06%.

Filename Base (e445aca) Head (38d628d2) Diff

FieldIndex.java 76.47% 75.00% -1.47%

LocalSerializer.java 97.95% 97.97% +0.03%

SQLiteIndexManager.java 96.36% 95.74% -0.62%

Test Logs

Notes

HTML coverage reports can be produced locally with ./gradlew <product>:checkCoverage.
Report files are located at <product-build-dir>/reports/jacoco/.

Head commit (38d628d2) is created by Prow via merging commits: e445aca ceb3121.

google-oss-bot · 2021-09-17T20:52:24Z

Binary Size Report

Affected SDKs

firebase-firestore

Type Base (e445aca) Head (38d628d2) Diff

aar 1.20 MB 1.20 MB +792 B (+0.1%)

apk (release) 3.24 MB 3.24 MB +456 B (+0.0%)

Test Logs

Notes

Head commit (38d628d2) is created by Prow via merging commits: e445aca ceb3121.

schmidt-sebastian

Some feedback. Please send to @wu-hui if you have updates by Monday.

schmidt-sebastian · 2021-09-17T20:58:33Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java

-  public FieldIndex decodeFieldIndex(String collection_group, int indexId, Index index) {
-    FieldIndex fieldIndex = new FieldIndex(collection_group, indexId);
+  public FieldIndex decodeFieldIndex(
+      String collectionGroup, int indexId, Index index, int updateSeconds, int updateNanos) {


Please use SnapshotVersion for seconds+nanos.

schmidt-sebastian · 2021-09-17T20:59:25Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

    result = 31 * result + segments.hashCode();
    return result;
  }

  @Override
  public String toString() {
-    return String.format("FieldIndex{collectionId='%s', segments=%s}", collectionId, segments);
+    return String.format(
+        "FieldIndex{collectionGroup='%s', segments=%s}", collectionGroup, segments);


updateTime?

schmidt-sebastian · 2021-09-17T20:59:28Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

  }

  @Override
  public int hashCode() {
-    int result = collectionId.hashCode();
+    int result = collectionGroup.hashCode();


updateTime?

schmidt-sebastian · 2021-09-17T20:59:31Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

@@ -118,18 +131,19 @@ public boolean equals(Object o) {
    FieldIndex fieldIndex = (FieldIndex) o;

    if (!segments.equals(fieldIndex.segments)) return false;
-    return collectionId.equals(fieldIndex.collectionId);
+    return collectionGroup.equals(fieldIndex.collectionGroup);


updateTime?

schmidt-sebastian · 2021-09-17T21:00:17Z

...base-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java

+    assertEquals(indexes.size(), 1);
+    FieldIndex index = indexes.get(0);
+    assertEquals(index.getUpdateTime(), new Timestamp(10, 20));
+  }


This test seems better suited for our serialization tests.

The test here checks that the persistence queries in addFieldIndex and getFieldIndex are correct.

thebrianchen

thanks for the speedy review!

thebrianchen · 2021-09-17T21:14:13Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/LocalSerializer.java

-  public FieldIndex decodeFieldIndex(String collection_group, int indexId, Index index) {
-    FieldIndex fieldIndex = new FieldIndex(collection_group, indexId);
+  public FieldIndex decodeFieldIndex(
+      String collectionGroup, int indexId, Index index, int updateSeconds, int updateNanos) {


thebrianchen · 2021-09-17T21:14:18Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

@@ -118,18 +131,19 @@ public boolean equals(Object o) {
    FieldIndex fieldIndex = (FieldIndex) o;

    if (!segments.equals(fieldIndex.segments)) return false;
-    return collectionId.equals(fieldIndex.collectionId);
+    return collectionGroup.equals(fieldIndex.collectionGroup);


thebrianchen · 2021-09-17T21:14:23Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

  }

  @Override
  public int hashCode() {
-    int result = collectionId.hashCode();
+    int result = collectionGroup.hashCode();


thebrianchen · 2021-09-17T21:14:27Z

firebase-firestore/src/main/java/com/google/firebase/firestore/model/FieldIndex.java

    result = 31 * result + segments.hashCode();
    return result;
  }

  @Override
  public String toString() {
-    return String.format("FieldIndex{collectionId='%s', segments=%s}", collectionId, segments);
+    return String.format(
+        "FieldIndex{collectionGroup='%s', segments=%s}", collectionGroup, segments);


thebrianchen · 2021-09-17T21:15:08Z

...base-firestore/src/test/java/com/google/firebase/firestore/local/SQLiteIndexManagerTest.java

+    assertEquals(indexes.size(), 1);
+    FieldIndex index = indexes.get(0);
+    assertEquals(index.getUpdateTime(), new Timestamp(10, 20));
+  }


The test here checks that the persistence queries in addFieldIndex and getFieldIndex are correct.

wu-hui

@thebrianchen You replied some comments, but the actual changes are not applied it seems. Did you forget to push a new commit?

wu-hui · 2021-09-20T15:55:38Z

firebase-firestore/src/main/java/com/google/firebase/firestore/local/SQLiteIndexManager.java

+    List<FieldIndex> allIndexes = new ArrayList<>();
+    db.query(
+            "SELECT index_id, collection_group, index_proto, update_time_seconds, update_time_nanos FROM index_configuration "
+                + "WHERE active")


Does WHERE active actually work? And even if it works, we should still be using active = 1.

Thanks for catching. SQL query still works without adding = 1, but it's best to follow best practices.

google-oss-bot · 2021-09-20T17:09:06Z

@thebrianchen: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
device-check-changed	`ceb3121`	link	`/test device-check-changed`

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

add update time to FieldIndex

2cdef90

thebrianchen added the api: firestore label Sep 17, 2021

thebrianchen requested a review from schmidt-sebastian September 17, 2021 20:39

thebrianchen assigned schmidt-sebastian Sep 17, 2021

googlebot added the cla: yes Override cla label Sep 17, 2021

google-oss-bot added the size/L label Sep 17, 2021

schmidt-sebastian reviewed Sep 17, 2021

View reviewed changes

schmidt-sebastian assigned thebrianchen and unassigned schmidt-sebastian Sep 17, 2021

thebrianchen commented Sep 17, 2021

View reviewed changes

resolve comments

e5fdd37

thebrianchen requested a review from wu-hui September 17, 2021 21:17

thebrianchen assigned wu-hui and unassigned thebrianchen Sep 17, 2021

wu-hui reviewed Sep 20, 2021

View reviewed changes

wu-hui assigned thebrianchen and wu-hui and unassigned wu-hui and thebrianchen Sep 20, 2021

wu-hui requested changes Sep 20, 2021

View reviewed changes

wu-hui assigned thebrianchen and unassigned wu-hui Sep 20, 2021

active = 1

ceb3121

thebrianchen requested a review from wu-hui September 20, 2021 16:40

thebrianchen assigned wu-hui and unassigned thebrianchen Sep 20, 2021

wu-hui approved these changes Sep 20, 2021

View reviewed changes

wu-hui removed their assignment Sep 20, 2021

wu-hui assigned thebrianchen Sep 20, 2021

thebrianchen merged commit 0e40391 into master Sep 20, 2021

thebrianchen deleted the bc/field-index-update-times branch September 20, 2021 19:29

firebase locked and limited conversation to collaborators Oct 21, 2021

add update time to FieldIndex #2984

add update time to FieldIndex #2984

Uh oh!

Conversation

thebrianchen commented Sep 17, 2021

Uh oh!

google-oss-bot commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

google-oss-bot commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Binary Size Report

Affected SDKs

firebase-firestore

Test Logs

Notes

Uh oh!

schmidt-sebastian left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thebrianchen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

wu-hui left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

google-oss-bot commented Sep 20, 2021

Uh oh!

Uh oh!

google-oss-bot commented Sep 17, 2021 •

edited

Loading

`firebase-firestore`

google-oss-bot commented Sep 17, 2021 •

edited

Loading

`firebase-firestore`