[
https://issues.apache.org/jira/browse/CALCITE-6420?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17850888#comment-17850888
]
Julian Hyde commented on CALCITE-6420:
--------------------------------------
[~cat_shark], Thanks for logging this. I wrote the {{Mapping}} class, many
years ago, and I always knew there were inconsistencies. You seem to have
deduced the purpose of the class, and how it is supposed to work. I would be
delighted if you cleaned it up.
Of course you must do this without introducing bugs in Calcite code that uses
it. And if you change APIs (e.g. rename methods) you must do so in a backwards
compatible way (which generally means leaving the old method in place,
deprecated). If you are unable to do the cleanup without breaking changes,
let's discuss.
> Fix confusing MappingType enum
> ------------------------------
>
> Key: CALCITE-6420
> URL: https://issues.apache.org/jira/browse/CALCITE-6420
> Project: Calcite
> Issue Type: Improvement
> Components: core
> Affects Versions: 1.37.0
> Reporter: ZiJie Song
> Assignee: ZiJie Song
> Priority: Major
>
> I've noticed that the MappingType enumeration is quite confusing and
> contradicts its own definition. After some research, I found that actually,
> there are many bugs in the code and implementation of the entire Mapping
> part. Next I'll describe the error I observed.
> 1. Enumeration error
> According to the four fields OPTIONAL_SOURCE, MULTIPLE_SOURCE,
> OPTIONAL_TARGET, MULTIPLE_TARGET in MappingType.java and the function design
> in the class, I realized that the original design intention of this class is
> as follows:
> Mapping is divided into 16 types according to the corresponding relationship
> between source and target. For a given source, it can correspond to 1, <=1,
> >=1 or any target, so the mapping is divided into four categories. On the
> contrary, the target corresponding to the source is also divided into four
> categories, for a total of 16 types of mapping.
> But for some reason, the ordinal of MappingType is not set correctly.
> According to the above definition, the ordinal of the surjection should be 2,
> but it is set to 1. The injective ordinal should be 1, but is set to 2. Many
> other types also have errors, such as InverseSurjection and so on.
> The above error led to a funny phenomenon, the result of
> MappingType.Surjection.isSurjection() turned out to be false. This also
> proves the correctness of my observation 1.
> 2. Implementation errors
> Due to the wrong MappingType setting, the implementation in Mapping is also
> very confusing. Partial_Surjection and Surjection are implemented as
> PartialMapping, which is completely inconsistent with their literal meaning.
> InverseSurjection should be the inverse function of Surjection, but it is
> interpreted as SurjectionWithInverse.
> This is not just an error in the meaning of variable naming, because
> functions such as MappingType.inverse() are also called in the code, which
> will lead to actual logical errors.
> TODO:
> 1. Set the ordinal correctly according to the literal meaning of each
> enumeration of MappingType.
> 2. Correspond the implementation of Mapping to the correct MappingType
> 3. Check the places where Mapping is used in the entire project and set them
> to the correct MappingType (about a dozen places). This requires examining
> the logical meaning they really need (for example, when using Surjection,
> does the code want to use the correct Surjection or the original wrong one?
> ). After modify the logic, I need to check whether it will cause other
> changes.
> I'd love to fix these issues myself. I'd be very happy if someone could help
> me check if the above ideas are correct.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
