Copilot commented on code in PR #7912:
URL: https://github.com/apache/ignite-3/pull/7912#discussion_r3063825578
##########
migration-tools/modules/migration-tools-commons/src/main/java/org/apache/ignite/migrationtools/sql/SqlDdlGenerator.java:
##########
@@ -430,6 +433,28 @@ private QueryEntityEvaluation
populateQueryEntity(QueryEntity qe, boolean allowE
// Mark keyFields as not nullable.
qe.getNotNullFields().addAll(qe.getKeyFields());
+
+ // We also want to remove fields non-compliant with AI3 so that
they are handled in the extra fields.
+ for (var e : qe.getFields().entrySet()) {
+ String fieldType = ClassnameUtils.ensureWrapper(e.getValue());
+
+ boolean isNativelySupportedType;
+ try {
+ Class<?> type =
ClassUtils.getClass(this.clientClassLoader, fieldType);
+ isNativelySupportedType =
TypeInspector.isPrimitiveType(type);
+ } catch (ClassNotFoundException ignored) {
+ // All natively supported types should be in the classpath.
+ // Some enums might still slip throught.
Review Comment:
Typo in comment: "slip throught" should be "slip through".
```suggestion
// Some enums might still slip through.
```
##########
migration-tools/modules/migration-tools-commons/src/main/java/org/apache/ignite/migrationtools/sql/SqlDdlGenerator.java:
##########
@@ -430,6 +433,28 @@ private QueryEntityEvaluation
populateQueryEntity(QueryEntity qe, boolean allowE
// Mark keyFields as not nullable.
qe.getNotNullFields().addAll(qe.getKeyFields());
+
+ // We also want to remove fields non-compliant with AI3 so that
they are handled in the extra fields.
+ for (var e : qe.getFields().entrySet()) {
+ String fieldType = ClassnameUtils.ensureWrapper(e.getValue());
+
+ boolean isNativelySupportedType;
+ try {
+ Class<?> type =
ClassUtils.getClass(this.clientClassLoader, fieldType);
+ isNativelySupportedType =
TypeInspector.isPrimitiveType(type);
+ } catch (ClassNotFoundException ignored) {
+ // All natively supported types should be in the classpath.
+ // Some enums might still slip throught.
+ isNativelySupportedType = false;
+ }
+
+ // Remove from the field list, we will need to map it in the
extra fields.
+ if (!isNativelySupportedType) {
+ mapsPojo = true;
+ qe.getFields().remove(e.getKey());
+ qe.getKeyFields().remove(e.getKey());
+ }
Review Comment:
Modifying `qe.getFields()` while iterating `qe.getFields().entrySet()`
(calling `qe.getFields().remove(...)` inside the enhanced for-loop) will throw
`ConcurrentModificationException` for typical `Map` implementations (e.g.,
`HashMap`/`LinkedHashMap`). Collect keys to remove first (or use an explicit
iterator and `iterator.remove()`), then update `keyFields` accordingly.
##########
migration-tools/modules/migration-tools-commons/src/main/java/org/apache/ignite/migrationtools/types/TypeInspector.java:
##########
@@ -133,7 +133,13 @@ public static List<InspectedField> inspectType(Class<?>
type) {
}
}
- private static boolean isPrimitiveType(Class<?> type) {
+ /**
+ * Whether the provided class, or it's derivatives, is natively supported.
+ *
+ * @param type Type.
+ * @return True or false.
+ */
Review Comment:
Grammar in Javadoc: "it's derivatives" should be "its derivatives"
(possessive, not contraction).
--
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]
