markap14 commented on code in PR #11817:
URL: https://github.com/apache/nifi/pull/11817#discussion_r4318180496
##########
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java:
##########
@@ -1922,17 +1922,33 @@ public static boolean isRecordWider(final RecordSchema
potentiallyWider, final R
continue;
}
- final Optional<DataType> widerType =
getWiderType(widerField.get().getDataType(), narrowerField.getDataType());
- if (widerType.isEmpty()) {
+ if (!isWiderOrEqual(widerField.get().getDataType(),
narrowerField.getDataType())) {
return false;
}
+ }
- if (!widerType.get().equals(widerField.get().getDataType())) {
- return false;
+ return true;
+ }
+
+ private static boolean isWiderOrEqual(final DataType potentiallyWider,
final DataType potentiallyNarrower) {
+ final RecordFieldType widerFieldType = potentiallyWider.getFieldType();
+ final RecordFieldType narrowerFieldType =
potentiallyNarrower.getFieldType();
+
+ if (widerFieldType == RecordFieldType.RECORD && narrowerFieldType ==
RecordFieldType.RECORD) {
+ final RecordSchema widerSchema = ((RecordDataType)
potentiallyWider).getChildSchema();
+ final RecordSchema narrowerSchema = ((RecordDataType)
potentiallyNarrower).getChildSchema();
+ if (widerSchema != null && narrowerSchema != null) {
+ return isRecordWider(widerSchema, narrowerSchema);
+ }
+ } else if (widerFieldType == RecordFieldType.ARRAY &&
narrowerFieldType == RecordFieldType.ARRAY) {
+ final DataType widerElementType = ((ArrayDataType)
potentiallyWider).getElementType();
+ final DataType narrowerElementType = ((ArrayDataType)
potentiallyNarrower).getElementType();
+ if (widerElementType != null && narrowerElementType != null) {
+ return isWiderOrEqual(widerElementType, narrowerElementType)
&&
RecordFieldType.ARRAY.getArrayDataType(widerElementType).equals(potentiallyWider);
Review Comment:
[Claude Opus 5.5]
At first glance
`RecordFieldType.ARRAY.getArrayDataType(widerElementType).equals(potentiallyWider)`
looks like it is always true. But the element types are already known to
match, so what it really checks is that the wider array does not allow null
elements, because `ArrayDataType.DEFAULT_NULLABLE` is `false`. That copies how
`getWiderType` rebuilds arrays with the default nullability. Could this be
written as `!((ArrayDataType) potentiallyWider).isElementsNullable()`, or have
a short comment saying why it's there? Otherwise it's easy to mistake it for
redundant code and remove it.
##########
nifi-commons/nifi-record/src/main/java/org/apache/nifi/serialization/record/util/DataTypeUtils.java:
##########
@@ -1922,17 +1922,33 @@ public static boolean isRecordWider(final RecordSchema
potentiallyWider, final R
continue;
}
- final Optional<DataType> widerType =
getWiderType(widerField.get().getDataType(), narrowerField.getDataType());
- if (widerType.isEmpty()) {
+ if (!isWiderOrEqual(widerField.get().getDataType(),
narrowerField.getDataType())) {
return false;
}
+ }
- if (!widerType.get().equals(widerField.get().getDataType())) {
- return false;
+ return true;
+ }
+
+ private static boolean isWiderOrEqual(final DataType potentiallyWider,
final DataType potentiallyNarrower) {
Review Comment:
[Claude Opus 5.5]
Could you add a one-line comment saying that nested records and arrays
should not be passed to `getWiderType` here, because `getWiderType` checks both
directions and the cost doubles at each nesting level? Nothing in the code
itself shows that, and it would stop someone from "simplifying" this back to a
plain `getWiderType` call later.
##########
nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java:
##########
@@ -1688,4 +1690,88 @@ public void
testMergeSchemasSingleSideNestedArrayFieldBecomesNullable() {
assertTrue(merged.getField("tags").orElseThrow().isNullable());
assertFalse(((ArrayDataType)
merged.getField("tags").orElseThrow().getDataType()).isElementsNullable());
}
+
+ @Test
+ public void
testMergeDataTypesWithDeeplyNestedNonWiderRecordsCompletesQuickly() {
+ final DataType merged =
assertTimeoutPreemptively(Duration.ofSeconds(10),
+ () -> DataTypeUtils.mergeDataTypes(createNestedRecordType(40,
"a"), createNestedRecordType(40, "b")));
+
+ final RecordSchema mergedSchema = ((RecordDataType)
merged).getChildSchema();
+ assertEquals(List.of("content", "a", "b"),
mergedSchema.getFieldNames());
+ }
+
+ @Test
+ public void
testMergeDataTypesWithChoiceOfDeeplyNestedNonWiderRecordsCompletesQuickly() {
Review Comment:
[Claude Opus 5.5]
This test and the ones at lines 1717 and 1730 all end up in the same call:
`getWiderType` on the two 40-level records, which then calls `isRecordWider`.
The test at line 1695 already covers both the record and the array branches of
`isWiderOrEqual`. Since the fix is in that one method, I don't think the CHOICE
and array-of-arrays variants add coverage, and we try to keep the number of
tests down. Would you be OK removing these three? The one at 1730 also only
checks that the result is a `CHOICE`.
##########
nifi-extension-bundles/nifi-standard-services/nifi-record-serialization-services-bundle/nifi-record-serialization-services/src/test/java/org/apache/nifi/json/TestJsonSchemaInference.java:
##########
@@ -178,17 +182,43 @@ public void testNestedChoiceOfEmptyOrStringArray() throws
IOException {
assertSame(RecordFieldType.STRING, arrayElementType.getFieldType());
}
+ @Test
+ void testDeeplyNestedNonWiderRecordsCompleteQuickly() {
+ final String json = "[{\"doc\":[" + createNestedJson("a", 40) + "," +
createNestedJson("b", 40) + "]}]";
+
+ final RecordSchema schema =
assertTimeoutPreemptively(Duration.ofSeconds(10),
+ () -> inferSchema(new
ByteArrayInputStream(json.getBytes(StandardCharsets.UTF_8))));
+
+ final DataType elementType = ((ArrayDataType)
schema.getDataType("doc").get()).getElementType();
+ assertEquals(List.of("content", "a", "b"), ((RecordDataType)
elementType).getChildSchema().getFieldNames());
+ }
+
+ @Test
+ void testNestedRecordWithSupersetOfFieldsKeepsItsFieldOrder() throws
IOException {
Review Comment:
[Claude Opus 5.5]
I don't think this test reaches the changed code. When one record's fields
include all of the other's, `getWiderRecordType` returns it through the
`containsAll` check before `isRecordWider` is called. The test also passes
against the original code. If it's meant to protect field order against a
different kind of fix in the future, it might belong in a separate PR.
Otherwise I'd suggest removing it.
##########
nifi-commons/nifi-record/src/test/java/org/apache/nifi/serialization/record/TestDataTypeUtils.java:
##########
@@ -1688,4 +1690,88 @@ public void
testMergeSchemasSingleSideNestedArrayFieldBecomesNullable() {
assertTrue(merged.getField("tags").orElseThrow().isNullable());
assertFalse(((ArrayDataType)
merged.getField("tags").orElseThrow().getDataType()).isElementsNullable());
}
+
+ @Test
+ public void
testMergeDataTypesWithDeeplyNestedNonWiderRecordsCompletesQuickly() {
+ final DataType merged =
assertTimeoutPreemptively(Duration.ofSeconds(10),
+ () -> DataTypeUtils.mergeDataTypes(createNestedRecordType(40,
"a"), createNestedRecordType(40, "b")));
+
+ final RecordSchema mergedSchema = ((RecordDataType)
merged).getChildSchema();
+ assertEquals(List.of("content", "a", "b"),
mergedSchema.getFieldNames());
+ }
+
+ @Test
+ public void
testMergeDataTypesWithChoiceOfDeeplyNestedNonWiderRecordsCompletesQuickly() {
+ final DataType choice =
RecordFieldType.CHOICE.getChoiceDataType(RecordFieldType.STRING.getDataType(),
createNestedRecordType(40, "a"));
+ final DataType merged =
assertTimeoutPreemptively(Duration.ofSeconds(10),
+ () -> DataTypeUtils.mergeDataTypes(choice,
createNestedRecordType(40, "b")));
+
+ final List<DataType> subTypes = ((ChoiceDataType)
merged).getPossibleSubTypes();
+ assertEquals(2, subTypes.size());
+ assertEquals(RecordFieldType.STRING.getDataType(), subTypes.get(0));
+ final RecordSchema mergedSchema = ((RecordDataType)
subTypes.get(1)).getChildSchema();
+ assertEquals(List.of("content", "a", "b"),
mergedSchema.getFieldNames());
+ }
+
+ @Test
+ public void
testMergeDataTypesWithChoiceOfDeeplyNestedNonWiderRecordArraysCompletesQuickly()
{
+ final DataType choice =
RecordFieldType.CHOICE.getChoiceDataType(RecordFieldType.STRING.getDataType(),
+ RecordFieldType.ARRAY.getArrayDataType(createNestedRecordType(40,
"a")));
+ final DataType merged =
assertTimeoutPreemptively(Duration.ofSeconds(10),
+ () -> DataTypeUtils.mergeDataTypes(choice,
RecordFieldType.ARRAY.getArrayDataType(createNestedRecordType(40, "b"))));
+
+ assertEquals(List.of(RecordFieldType.STRING.getDataType(),
+
RecordFieldType.ARRAY.getArrayDataType(createNestedRecordType(40, "a")),
+
RecordFieldType.ARRAY.getArrayDataType(createNestedRecordType(40, "b"))),
+ ((ChoiceDataType) merged).getPossibleSubTypes());
+ }
+
+ @Test
+ public void
testMergeDataTypesWithNestedArraysOfDeeplyNestedNonWiderRecordsCompletesQuickly()
{
+ final DataType merged =
assertTimeoutPreemptively(Duration.ofSeconds(10), () ->
DataTypeUtils.mergeDataTypes(
+
RecordFieldType.ARRAY.getArrayDataType(RecordFieldType.ARRAY.getArrayDataType(createNestedRecordType(40,
"a"))),
+
RecordFieldType.ARRAY.getArrayDataType(RecordFieldType.ARRAY.getArrayDataType(createNestedRecordType(40,
"b")))));
+
+ assertEquals(RecordFieldType.CHOICE, merged.getFieldType());
+ }
+
+ @ParameterizedTest
+ @MethodSource("nestedArrayWiderRecordData")
+ public void testGetWiderTypeWithNestedArrayFields(final DataType
widerArrayType, final DataType narrowerArrayType, final boolean expectedWider) {
Review Comment:
[Claude Opus 5.5]
All four cases here pass against the original `DataTypeUtils`, so this test
only checks behavior that existed before the PR rather than anything the PR
changes. I'd suggest removing it, along with `nestedArrayWiderRecordData`. That
also avoids locking in the "non-nullable elements over nullable elements" case,
which looks backwards to me: the merged result drops the "elements may be null"
setting. If that behavior needs another look, we can open a separate Jira for
it.
--
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]
