IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types#13219
IGNITE-28755 SQL Calcite: Fix typeId collisions for autogenerated types#13219alex-plekhanov wants to merge 3 commits into
Conversation
alex-plekhanov
force-pushed
the
ignite-28755
branch
from
bbde33b to
c9672f6
Compare
There was a problem hiding this comment.
Pull request overview
This PR addresses binary typeId collisions in the Calcite SQL CREATE TABLE flow by detecting collisions against existing marshaller mappings and either (a) regenerating auto-generated type names or (b) rejecting explicitly provided type names that would collide.
Changes:
- Add collision avoidance loop for auto-generated key/value type names in Calcite DDL handling.
- Add validation that explicitly provided
KEY_TYPE/VALUE_TYPEdo not collide with an already-mapped type ID. - Add integration tests intended to cover both auto-generated collision resolution and explicit-type collision rejection, and expose a marshaller-context lookup helper.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| modules/core/src/main/java/org/apache/ignite/internal/MarshallerContextImpl.java | Adds resolveClassName helper used by Calcite to detect existing mappings (but currently contains a broken assert condition). |
| modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ddl/DdlCommandHandler.java | Regenerates auto-generated type names until no typeId collision is found; validates explicit type names to fail fast on collisions. |
| modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/TableDdlIntegrationTest.java | Adds integration tests for auto-generated collision handling and explicit collision rejection (but the collision setup is currently extremely heavy and probabilistic). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
| String anotherTypeName = ((MarshallerContextImpl)binCtx.marshaller().getContext()).resolveClassName(JAVA_ID, typeId); | ||
|
|
||
| if (anotherTypeName == null || anotherTypeName.equals(typeName) | ||
| || binCtx.userTypeName(anotherTypeName).equals(binCtx.userTypeName(typeName))) |
There was a problem hiding this comment.
binCtx.userTypeName(anotherTypeName).equals(binCtx.userTypeName(typeName)) - this branch is not covered by tests ?
| } | ||
|
|
||
| /** */ | ||
| private void validateTypeName(String typeName) { |
There was a problem hiding this comment.
What action points do user need to implement if exception will raised ? Seems we need to give a some clue here ?
|
|
||
| if (mappedName != null) { | ||
| assert mappedName.accepted() : mappedName; | ||
| assert !ensureAccepted || mappedName.accepted() : mappedName; |
There was a problem hiding this 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 ?
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
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.