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
Port IndexValueWriter #5826
Port IndexValueWriter #5826
Conversation
|
3576df8
to
42fa149
Compare
Binary Size ReportAffected SDKs
Test Logs
|
Size Analysis ReportAffected Products
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm with some questions and optional suggestions
indexValue: Value, | ||
encoder: DirectionalIndexByteEncoder | ||
): void { | ||
if ('nullValue' in indexValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: use switch
here, since every if-condition is a single statement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, switch does not work here since we are not doing an equals comparison on a value.
): void { | ||
this.writeValueTypeLabel(encoder, INDEX_TYPE_REFERENCE); | ||
const path = DocumentKey.fromName(referenceValue).path; | ||
for (let i = 0; i < path.length; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional:
DocumentKey.fromName(referenceValue).path.forEach(segment => {
this.writeValueTypeLabel(encoder, INDEX_TYPE_REFERENCE_SEGMENT);
this.writeUnlabeledIndexString(segment, encoder);
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept "path" but used forEach.
import { fail } from '../util/assert'; | ||
import { ByteString } from '../util/byte_string'; | ||
|
||
export class OrderedCodeWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: copy comment over and add a TODO that this section will be implemented in the future, rather than intentionally not implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is already in review: #5821
@@ -73,6 +81,10 @@ export function typeOrder(value: Value): TypeOrder { | |||
|
|||
/** Tests `left` and `right` for equality based on the backend semantics. */ | |||
export function valueEquals(left: Value, right: Value): boolean { | |||
if (left === right) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't fully follow here, were we always able to do this direct comparison, or does this PR now allow us to do this direct comparison?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were always able to do this. I added this now to make it more performant to do an equals check on MAX_VALUE.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
indexValue: Value, | ||
encoder: DirectionalIndexByteEncoder | ||
): void { | ||
if ('nullValue' in indexValue) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, switch does not work here since we are not doing an equals comparison on a value.
): void { | ||
this.writeValueTypeLabel(encoder, INDEX_TYPE_REFERENCE); | ||
const path = DocumentKey.fromName(referenceValue).path; | ||
for (let i = 0; i < path.length; ++i) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Kept "path" but used forEach.
import { fail } from '../util/assert'; | ||
import { ByteString } from '../util/byte_string'; | ||
|
||
export class OrderedCodeWriter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is already in review: #5821
@@ -73,6 +81,10 @@ export function typeOrder(value: Value): TypeOrder { | |||
|
|||
/** Tests `left` and `right` for equality based on the backend semantics. */ | |||
export function valueEquals(left: Value, right: Value): boolean { | |||
if (left === right) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We were always able to do this. I added this now to make it more performant to do an equals check on MAX_VALUE.
This ports:
https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/index/DirectionalIndexByteEncoder.java
https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/index/FirestoreIndexValueWriter.java
https://github.com/firebase/firebase-android-sdk/blob/master/firebase-firestore/src/main/java/com/google/firebase/firestore/index/IndexByteEncoder.java