You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "alex-plekhanov (via GitHub)" <gi...@apache.org> on 2026/06/08 11:42:58 UTC
[PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
alex-plekhanov opened a new pull request, #13219:
URL: https://github.com/apache/ignite/pull/13219
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see tab `PR Check` at [TC.Bot - Instance 1](https://tcbot2.sbt-ignite-dev.ru/prs.html) or [TC.Bot - Instance 2](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3414419735
##########
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:
Accepted flag means that mapping is accpeted by cluster. Assertion here is to keep invariant implemented in method resolveMissedMapping, (looks like this method should be called only for accepted mapping), but for new check I don't think it's always true, so, just in case, removed this assert
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3418486468
##########
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:
Assert enabled only for resolveMissedMapping (ensureAccepted = true). For resolveClassName assert is disabled (ensureAccepted = false)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3414391326
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -498,4 +516,35 @@ else if (!F.isEmpty(cmd.primaryKeyColumns()) && cmd.primaryKeyColumns().size() =
return res;
}
+
+ /** */
+ private void validateTypeName(String typeName) {
Review Comment:
User should retry with another type name, Added message to the user.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3393686234
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -498,4 +516,35 @@ else if (!F.isEmpty(cmd.primaryKeyColumns()) && cmd.primaryKeyColumns().size() =
return res;
}
+
+ /** */
+ private void validateTypeName(String typeName) {
Review Comment:
What action points do user need to implement if exception will raised ? Seems we need to give a some clue here ?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -498,4 +516,35 @@ else if (!F.isEmpty(cmd.primaryKeyColumns()) && cmd.primaryKeyColumns().size() =
return res;
}
+
+ /** */
+ private void validateTypeName(String typeName) {
+ String duplicatedTypeName = duplicatedTypeName(typeName);
+ if (duplicatedTypeName != null) {
+ throw new IgniteSQLException(
+ "Duplicate ID [typeId=" + qryProc.objectContext().binaryContext().typeId(typeName) +
+ ", oldCls=" + duplicatedTypeName +
+ ", newCls=" + typeName + ']');
+ }
+ }
+
+ /** */
+ private boolean hasTypeIdCollisions(String typeName) {
+ return duplicatedTypeName(typeName) != null;
+ }
+
+ /** */
+ private @Nullable String duplicatedTypeName(String typeName) {
+ BinaryContext binCtx = qryProc.objectContext().binaryContext();
+
+ int typeId = binCtx.typeId(typeName);
+
+ String anotherTypeName = ((MarshallerContextImpl)binCtx.marshaller().getContext()).resolveClassName(JAVA_ID, typeId);
+
+ if (anotherTypeName == null || anotherTypeName.equals(typeName)
+ || binCtx.userTypeName(anotherTypeName).equals(binCtx.userTypeName(typeName)))
Review Comment:
binCtx.userTypeName(anotherTypeName).equals(binCtx.userTypeName(typeName)) - this branch is not covered by tests ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3418296041
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -498,4 +516,35 @@ else if (!F.isEmpty(cmd.primaryKeyColumns()) && cmd.primaryKeyColumns().size() =
return res;
}
+
+ /** */
+ private void validateTypeName(String typeName) {
+ String duplicatedTypeName = duplicatedTypeName(typeName);
+ if (duplicatedTypeName != null) {
+ throw new IgniteSQLException(
+ "Duplicate ID [typeId=" + qryProc.objectContext().binaryContext().typeId(typeName) +
+ ", oldCls=" + duplicatedTypeName +
+ ", newCls=" + typeName + ']');
+ }
+ }
+
+ /** */
+ private boolean hasTypeIdCollisions(String typeName) {
+ return duplicatedTypeName(typeName) != null;
+ }
+
+ /** */
+ private @Nullable String duplicatedTypeName(String typeName) {
+ BinaryContext binCtx = qryProc.objectContext().binaryContext();
+
+ int typeId = binCtx.typeId(typeName);
+
+ String anotherTypeName = ((MarshallerContextImpl)binCtx.marshaller().getContext()).resolveClassName(JAVA_ID, typeId);
+
+ if (anotherTypeName == null || anotherTypeName.equals(typeName)
+ || binCtx.userTypeName(anotherTypeName).equals(binCtx.userTypeName(typeName)))
Review Comment:
I think this still need to be covered by test, wdyt ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3418912208
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java:
##########
@@ -498,4 +516,35 @@ else if (!F.isEmpty(cmd.primaryKeyColumns()) && cmd.primaryKeyColumns().size() =
return res;
}
+
+ /** */
+ private void validateTypeName(String typeName) {
+ String duplicatedTypeName = duplicatedTypeName(typeName);
+ if (duplicatedTypeName != null) {
+ throw new IgniteSQLException(
+ "Duplicate ID [typeId=" + qryProc.objectContext().binaryContext().typeId(typeName) +
+ ", oldCls=" + duplicatedTypeName +
+ ", newCls=" + typeName + ']');
+ }
+ }
+
+ /** */
+ private boolean hasTypeIdCollisions(String typeName) {
+ return duplicatedTypeName(typeName) != null;
+ }
+
+ /** */
+ private @Nullable String duplicatedTypeName(String typeName) {
+ BinaryContext binCtx = qryProc.objectContext().binaryContext();
+
+ int typeId = binCtx.typeId(typeName);
+
+ String anotherTypeName = ((MarshallerContextImpl)binCtx.marshaller().getContext()).resolveClassName(JAVA_ID, typeId);
+
+ if (anotherTypeName == null || anotherTypeName.equals(typeName)
+ || binCtx.userTypeName(anotherTypeName).equals(binCtx.userTypeName(typeName)))
Review Comment:
Test added
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "asf-gitbox-commits (via GitHub)" <gi...@apache.org>.
asf-gitbox-commits closed pull request #13219: IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types
URL: https://github.com/apache/ignite/pull/13219
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3394050456
##########
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:
there is no definition\documentation what ".accepted()" flag mean - thus it`s hard to understand this assertion - can you append some kind of explanation here ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
alex-plekhanov commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3414419735
##########
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:
Accepted flag means that mapping is accpeted by cluster. Assertion here is to keep invariant implemented in method resolveMissedMapping, (looks like this method should be called only for accepted mapping), but for new check I don't think it's always true, so, just in case, removed this assert)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types [ignite]
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #13219:
URL: https://github.com/apache/ignite/pull/13219#discussion_r3418294334
##########
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:
I see that assert still in place, did i miss smth from explanation ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org