[jira] [Created] (CALCITE-6420) Fix confusing MappingType enum

["ZiJie Song (Jira)" <ji...@apache.org>]

ZiJie Song created CALCITE-6420:
-----------------------------------

             Summary: 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


Fix confusing MappingType enum
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)