Load CDK: avro tests have temporal types #51546
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
.../load-avro/src/testFixtures/kotlin/io/airbyte/cdk/load/data/avro/AvroExpectedRecordMapper.kt
Outdated
Show resolved
Hide resolved
| return when (avroValue) { | ||
| null -> NullValue | ||
| is GenericRecord -> | ||
| ObjectValue( | ||
| avroValue.schema.fields.associateTo(linkedMapOf()) { field -> | ||
| field.name() to convert(avroValue.get(field.name())) | ||
| field.name() to convert(avroValue.get(field.name()), field.schema()) |
There was a problem hiding this comment.
Why are we using the field schema vs using the passed in schema here?
There was a problem hiding this comment.
goal is to avoid accidentally masking real problems. I.e. if we wrote an avro file with the wrong schema, we should read back that schema, rather than forcibly converting it to the schema we expect
…/airbyte/cdk/load/data/avro/AvroExpectedRecordMapper.kt Co-authored-by: Francis Genet <[email protected]>
| @@ -15,22 +15,15 @@ import io.airbyte.protocol.models.v0.AirbyteRecordMessageMetaChange | |||
| import java.time.Instant | |||
| import java.util.UUID | |||
|
|
|||
| class AirbyteValueWithMetaToOutputRecord { | |||
| class AirbyteValueWithMetaToOutputRecord(val extractedAtType: ExtractedAtType) { | |||
There was a problem hiding this comment.
It feels wrong to make the extractedAtType a first class citizen. The avro schema tests in the avro toolkit validate that the schema is correct. Why should this be a feature of the ITs?
There was a problem hiding this comment.
this class gets used for all the output formats (json/csv/avro/parquet), so it needs to know who the caller is
maybe this is actually a sign that tests shouldn't treat the airbyte_X fields specially? I.e. if RecordDiffer just operated on a plain AirbyteValue, we could skip this step? (or at least not extracted_at, since we do want RecordDiffer to ignore loaded_at / raw_id)
(probably not a change I want to make right now, but if we ever want to come back and improve the core test framework, maybe it's on that list)
(pr will fail CI until the actual bug is fixed)
test now fails on our current behavior