[GitHub] [beam] kennknowles commented on a diff in pull request #28624: [Fix Python Bigtable dataloss bug] Stop unsetting timestamps of -1

["kennknowles (via GitHub)" <gi...@apache.org>]

kennknowles commented on code in PR #28624:
URL: https://github.com/apache/beam/pull/28624#discussion_r1335225747


##########
sdks/java/io/google-cloud-platform/src/main/java/org/apache/beam/sdk/io/gcp/bigtable/BigtableWriteSchemaTransformProvider.java:
##########
@@ -179,12 +179,13 @@ public KV<ByteString, Iterable<Mutation>> apply(Row row) {
                     .setColumnQualifier(
                         ByteString.copyFrom(ofNullable(mutation.get("column_qualifier")).get()))
                     .setFamilyNameBytes(
-                        ByteString.copyFrom(ofNullable(mutation.get("family_name")).get()));
-            if (mutation.containsKey("timestamp_micros")) {
-              setMutation =
-                  setMutation.setTimestampMicros(
-                      Longs.fromByteArray(ofNullable(mutation.get("timestamp_micros")).get()));
-            }
+                        ByteString.copyFrom(ofNullable(mutation.get("family_name")).get()))
+                    // Use timestamp if provided, else default to -1 (current Bigtable server time)
+                    .setTimestampMicros(

Review Comment:
   nit: would the most appropriate logic be more like this?
   
   ```
   if (mutation.containsKey("timestamp_micros")) { 
     builder.setTimestampMicros(mutation.get("timestamp_micros"));
   }
   ```
   
   and so on probably for all the fields, since proto handling of optional fields is not usable



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@beam.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org