sv2000 commented on a change in pull request #3458:
URL: https://github.com/apache/gobblin/pull/3458#discussion_r804879857
##########
File path:
gobblin-utility/src/test/java/org/apache/gobblin/util/AvroUtilsTest.java
##########
@@ -101,7 +101,7 @@ public void testNullifyFieldForNonUnionSchemaMerge() {
Schema expectedOutputSchema1 =
new Schema.Parser().parse("{\"type\":\"record\", \"name\":\"test\", "
+ "\"fields\":["
- + "{\"name\": \"name\", \"type\": \"string\"}, " + "{\"name\":
\"number\", \"type\": [\"null\", \"int\"]}"
+ + "{\"name\": \"name\", \"type\": \"string\"}, " + "{\"name\":
\"number\", \"type\": [\"null\", \"int\"],\"default\":null}]}"
Review comment:
Can you add a comment why the default is needed here?
##########
File path:
gobblin-restli/gobblin-throttling-service/gobblin-throttling-service-server/src/test/java/org/apache/gobblin/restli/throttling/PoliciesResourceTest.java
##########
@@ -38,7 +38,7 @@ public void test() {
ThrottlingPolicyFactory factory = new ThrottlingPolicyFactory();
SharedLimiterKey res1key = new SharedLimiterKey("res1");
- Map<String, String> configMap =
avro.shaded.com.google.common.collect.ImmutableMap.<String, String>builder()
+ Map<String, String> configMap =
com.google.common.collect.ImmutableMap.<String, String>builder()
Review comment:
Do we no longer need to shade the guava dependency brought in by Avro
1.9?
##########
File path:
gobblin-core-base/src/test/java/org/apache/gobblin/converter/filter/AvroSchemaFieldRemoverTest.java
##########
@@ -35,18 +35,30 @@
@Test
public void testRemoveFields() throws IllegalArgumentException, IOException {
+ // Avro 1.9.2 uses Object as default value and uses internal
JacksonUtils.toJson to convert the Object default value
+ // to JsonNode. Enum default value is not supported and hence the
conversion fails. Updated the input schema and
+ // removed the non null default value for enum.
Schema convertedSchema1 =
convertSchema("/converter/recursive_schema_1.avsc", "YwchQiH.OjuzrLOtmqLW");
Schema expectedSchema1 =
parseSchema("/converter/recursive_schema_1_converted.avsc");
- Assert.assertEquals(convertedSchema1, expectedSchema1);
+ Assert.assertEquals(convertedSchema1.toString(),
expectedSchema1.toString());
Review comment:
Looks like we are restricting the condition for equality of two Avro
schemas. Why is this change needed after your changes to .avsc files? Also,
what does this mean in production pipelines that uses the Schema#equals()
checks?
##########
File path:
gobblin-core/src/test/java/org/apache/gobblin/recordaccess/AvroGenericRecordAccessorTest.java
##########
@@ -139,6 +139,11 @@ public void testGetStringArrayUtf8() throws IOException {
@Test
public void testGetMultiConvertsStrings() throws IOException {
+ // The below error is due to invalid avro data. Type with "null" union
must have "null" first and then
Review comment:
What would this mean for production deployment with existing Avro
schemas containing nullable unions with null not being the first member of the
union?
--
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]
