Copilot commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3393376629
##########
modules/core/src/main/java/org/apache/ignite/internal/MarshallerContextImpl.java:
##########
@@ -541,21 +541,38 @@ else if (clientNode) {
/**
* @param platformId Platform id.
* @param typeId Type id.
+ * @param ensureAccepted Ensure that mapping is accepted.
*/
- public String resolveMissedMapping(byte platformId, int typeId) {
+ private @Nullable String resolveClassName(byte platformId, int typeId,
boolean ensureAccepted) {
ConcurrentMap<Integer, MappedName> cache = getCacheFor(platformId);
MappedName mappedName = cache.get(typeId);
if (mappedName != null) {
- assert mappedName.accepted() : mappedName;
+ assert !ensureAccepted && mappedName.accepted() : mappedName;
Review Comment:
The assertion condition is inverted: with ensureAccepted=true this will
always fail (because `!ensureAccepted` is false), and with ensureAccepted=false
it still asserts `accepted()` even though callers explicitly want to read
unaccepted mappings. This breaks resolveMissedMapping and makes
resolveClassName unusable for collision checks when assertions are enabled.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java:
##########
@@ -1240,6 +1245,63 @@ public void testMulitlineWithCreateTable() {
}
}
+ /**
+ * Tests that type id collisions for autogenerated types are automatically
resolved.
+ */
+ @Test
+ public void testAutogeneratedTypeIdCollision() {
+ BinaryContext binCtx = client.context().cacheObjects().binaryContext();
+
+ binCtx.registerUserClassName(binCtx.typeId("TestTypeName"),
"TestTypeName", false, true, JAVA_ID);
+
+ MarshallerContextImpl marshCtx =
(MarshallerContextImpl)binCtx.marshaller().getContext();
+
+ Map<Integer, MappedName> mappings =
marshCtx.getCachedMappings().get(JAVA_ID);
+
+ MappedName dummyName = new MappedName("Dummy", true);
+
+ // Maximize probability of collision.
+ for (int i = 0; i < 20_000_000; i++)
+ mappings.putIfAbsent(i, dummyName);
Review Comment:
This test populates the marshaller mappings map with 20,000,000 entries and
never cleans it up. This is very likely to cause excessive runtime, high memory
usage (Integer boxing + CHM node overhead), and can destabilize the rest of the
test class/JVM since the same node is reused across tests. It is also still
probabilistic (collisions may not happen), so it may pass even if the
collision-handling logic regresses.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java:
##########
@@ -1240,6 +1245,63 @@ public void testMulitlineWithCreateTable() {
}
}
+ /**
+ * Tests that type id collisions for autogenerated types are automatically
resolved.
+ */
+ @Test
+ public void testAutogeneratedTypeIdCollision() {
+ BinaryContext binCtx = client.context().cacheObjects().binaryContext();
+
+ binCtx.registerUserClassName(binCtx.typeId("TestTypeName"),
"TestTypeName", false, true, JAVA_ID);
+
+ MarshallerContextImpl marshCtx =
(MarshallerContextImpl)binCtx.marshaller().getContext();
+
+ Map<Integer, MappedName> mappings =
marshCtx.getCachedMappings().get(JAVA_ID);
+
+ MappedName dummyName = new MappedName("Dummy", true);
+
+ // Maximize probability of collision.
+ for (int i = 0; i < 20_000_000; i++)
+ mappings.putIfAbsent(i, dummyName);
+
+ for (int i = 0; i < 100; i++) {
+ sql("CREATE TABLE test (id1 int, id2 int, val int, primary key
(id1, id2))");
+ sql("INSERT INTO test VALUES (0, 0, 0)");
+ sql("DROP TABLE test");
+ }
+ }
+
+ /**
+ * Tests that table with excplicetly provided types with type id
collisions cannot be created.
Review Comment:
Typo in Javadoc: "excplicetly" -> "explicitly".
--
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]
