-
Notifications
You must be signed in to change notification settings - Fork 409
fix(firestore-bigquery-change-tracker): keep partition value on delete using old data #2424
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
Conversation
96b6af6
to
17fb4e9
Compare
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.
Pull Request Overview
This PR updates the partitioning logic to retain the partition value on delete events by sourcing it from the old data rather than the new event data.
- Uses the new ChangeType import to differentiate between DELETE and non-DELETE events.
- Applies a conditional to select the appropriate field value for partitioning.
Comments suppressed due to low confidence (1)
firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts:202
- [nitpick] Consider adding an inline comment explaining why event.oldData is used for DELETE events. This will improve clarity and maintainability for future developers.
event.operation === ChangeType.DELETE ? event.oldData[firestoreFieldName] : event.data[firestoreFieldName];
687952e
to
ee64adf
Compare
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 as a fix, we will need to bump the change tracker package version?
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.
Pull Request Overview
This PR ensures that when a Firestore delete event occurs without new data, the BigQuery partition value is derived from the old data.
- Imports
ChangeType
and updatesgetPartitionValue
to useoldData
for delete operations. - Adds a guard to early return when both
data
andoldData
are null. - Adjusts the
fieldValue
assignment based on the event’s operation.
Comments suppressed due to low confidence (2)
firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts:209
- Using
!fieldValue
treats any falsy value (e.g.,0
) as missing. If the partition field might legitimately be falsy, use a nullish check (fieldValue == null
) instead.
if (!fieldName || !fieldValue) {
firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts:196
- Add a unit test for the delete path to verify that the partition value is correctly pulled from
oldData
whenevent.data
is null.
getPartitionValue(event: FirestoreDocumentChangeEvent) {
firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts
Outdated
Show resolved
Hide resolved
firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts
Show resolved
Hide resolved
ee64adf
to
71229e3
Compare
firestore-bigquery-export/firestore-bigquery-change-tracker/src/bigquery/partitioning.ts
Show resolved
Hide resolved
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!
Resolves #2303