[
https://issues.apache.org/jira/browse/CALCITE-6420?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
]
ZiJie Song updated CALCITE-6420:
--------------------------------
Description:
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.
was:
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.
> 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)
