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)
