Dobiasd commented on pull request #15087: URL: https://github.com/apache/beam/pull/15087#issuecomment-869953354
@iemejia Thanks for the review. :+1: I tried to add tests (see the latest commit), but it actually is quite hard to do correctly (they currently fail), because in the Beam schema (and thus row), the Avro microsecond timestamp is also represented as `FieldType.DATETIME` (same as the millisecond timestamp). So the back conversion, i.e., `AvroUtils.toAvroSchema(beamSchema)` in `testFromBeamSchema` and `AvroUtils.toGenericRecord(getBeamRow() ...);` in `testBeamRowToGenericRecordInferSchema`/`testBeamRowToGenericRecord`, returns an Avro schema/record with milliseconds instead of microseconds, which makes the equality check with the original schema/record (`assertEquals(getAvroSchema(), avroSchema);`/`assertEquals(getGenericRecord(), genericRecord);`) fail. Since we probably don't want to introduce a new `FieldType.DATETIMEMICROS`, we'd need to somewhat hack around this "source resolution amnesia" in the test, which might create more harm than help. Another way would be to add an additional, separate test (without back conversion check) just for the microsecond timestamp, but regarding the triviality of the implementation, this too currently seems to me like too much overhead. What do you think? -- 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-unsubscr...@beam.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
