rahil-c commented on PR #6637:
URL: https://github.com/apache/hudi/pull/6637#issuecomment-1242384390
> Fix LGTM. However, we should not be adding this whole end to end test in
`TestCOWDataSource` and `TestMORDataSource`. These tests are there to test
overall datasource related functionality, and should not really be used to test
something so specific as DMS payload. There should be no need to run an entire
end to end test to discover this issue.
>
> There is a `TestAWSDmsAvroPayload` class. We should understand why tests
in that class did not catch the issue, and just modify them or add a new test
as needed to be able to catch this issue.
Sounds good I agree that adding this unittest in this class might be
unnecessary. As for modifying or adding a new test within
`TestAWSDmsAvroPayload` this also might be not needed as the tests actually are
referring to the correct api and do have coverage. For example in one of the
unit tests of that class that handles delete case
```
@Test
public void testDelete() {
Schema avroSchema = new Schema.Parser().parse(AVRO_SCHEMA_STRING);
GenericRecord deleteRecord = new GenericData.Record(avroSchema);
deleteRecord.put("field1", 2);
deleteRecord.put("Op", "D");
GenericRecord oldRecord = new GenericData.Record(avroSchema);
oldRecord.put("field1", 2);
oldRecord.put("Op", "U");
AWSDmsAvroPayload payload = new
AWSDmsAvroPayload(Option.of(deleteRecord));
try {
Option<IndexedRecord> outputPayload =
payload.combineAndGetUpdateValue(oldRecord, avroSchema);
// expect nothing to be committed to table
assertFalse(outputPayload.isPresent());
} catch (Exception e) {
fail("Unexpected exception");
}
}
```
it uses the correct `combineAndGetUpdateValue(oldRecord, avroSchema);`
method signature which is not requiring us to take in account the `properties`
object.
If we want to reproduce the same exception as what was reported we can add
the `properties` object to the call ` Option<IndexedRecord> outputPayload =
payload.combineAndGetUpdateValue(oldRecord, avroSchema, properties);` and get
the same exception
```
ava.util.NoSuchElementException: No value present in Option
at org.apache.hudi.common.util.Option.get(Option.java:89)
at
org.apache.hudi.common.model.AWSDmsAvroPayload.combineAndGetUpdateValue(AWSDmsAvroPayload.java:85)
at
org.apache.hudi.common.model.TestAWSDmsAvroPayload.testDeleteWithProperties(TestAWSDmsAvroPayload.java:123)
at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
```
But this however would not make sense as this code change is making sure to
use the signature without the `properties` for both `return
getInsertValue(schema)` and `
return combineAndGetUpdateValue(currentValue, schema)` similar to what the
tests have been doing for some time.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
