mihaibudiu commented on code in PR #4350:
URL: https://github.com/apache/calcite/pull/4350#discussion_r2069165248
##########
core/src/main/java/org/apache/calcite/rex/RexPermuteInputsShuttle.java:
##########
@@ -54,14 +55,41 @@ public class RexPermuteInputsShuttle extends RexShuttle {
public RexPermuteInputsShuttle(
Mappings.TargetMapping mapping,
RelNode... inputs) {
- this(mapping, fields(inputs));
+ this(mapping, fields(inputs), false);
+ }
+
+ /**
+ * Creates a RexPermuteInputsShuttle with target type matching control.
+ *
+ * <p>The mapping provides at most one target for every source. If a source
Review Comment:
I think this paragraph belongs in the documentation for the mapping
parameter.
I don't think you should discuss here what TargetMapping does, that should
belong to the documentation of TargetMapping itself. What you should say is
what this function assumes about the mapping. Perhaps you mean to say that
every RexInputRef in the expression that will be analyzed by this shuttle needs
to be a key in this mapping?
##########
core/src/main/java/org/apache/calcite/rex/RexPermuteInputsShuttle.java:
##########
@@ -54,14 +55,41 @@ public class RexPermuteInputsShuttle extends RexShuttle {
public RexPermuteInputsShuttle(
Mappings.TargetMapping mapping,
RelNode... inputs) {
- this(mapping, fields(inputs));
+ this(mapping, fields(inputs), false);
+ }
+
+ /**
+ * Creates a RexPermuteInputsShuttle with target type matching control.
Review Comment:
what is 'control'? It's not mentioned anywhere else.
##########
core/src/main/java/org/apache/calcite/rex/RexPermuteInputsShuttle.java:
##########
@@ -54,14 +55,41 @@ public class RexPermuteInputsShuttle extends RexShuttle {
public RexPermuteInputsShuttle(
Mappings.TargetMapping mapping,
RelNode... inputs) {
- this(mapping, fields(inputs));
+ this(mapping, fields(inputs), false);
+ }
+
+ /**
+ * Creates a RexPermuteInputsShuttle with target type matching control.
+ *
+ * <p>The mapping provides at most one target for every source. If a source
+ * has no targets and is referenced in the expression,
+ * {@link
org.apache.calcite.util.mapping.Mappings.TargetMapping#getTarget(int)}
+ * will give an error. Otherwise the mapping gives a unique target.
+ *
+ * @param mapping Mapping that defines source-to-target relation
+ * @param shouldMatchTargetType If true, enforces use target type when
remapping inputs
Review Comment:
Based on this description, this should be called 'matchTargetType'. There is
no need for 'should'.
I would describe what this does for both 'true' and 'false' cases.
In the 'false' case each input ref is substituted with a reference with the
same type as the original input.
In the 'true' case each input ref is substituted with a reference with the
same type as the target type.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
