You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by GitBox <gi...@apache.org> on 2020/05/03 15:50:19 UTC
[GitHub] [calcite] rkondakov opened a new pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
rkondakov opened a new pull request #1959:
URL: https://github.com/apache/calcite/pull/1959
Currently method `RelTrait.apply(Mappings.Mapping)` might throw an exception when collation/distribution keys are not covered by mapping. For example when we apply project
`SELECT name`
on input
`id, name ORDER BY id`
Method `RelTrait.apply(Mappings.Mapping)` throws NPE because of internals of `RexUtil.apply` where assumed that all sort keys `[0]` should be covered by project mapping keys `name=[1]`. And when this assumption is wrong, the exception is thrown.
In this patch the graceful handling for such situations added for both collation and distribution traits. When collation or distribution cannot be derived from the mapping, `RelDistributions#RANDOM_DISTRIBUTED` and `RelCollations#EMPTY` are returned accordingly.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422448391
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +141,14 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ for (int key : keys) {
+ if (mapping.getTargetOpt(key) == -1) {
+ return RANDOM_DISTRIBUTED; // Some distribution keys are not mapped => random.
Review comment:
I would recommend `ANY`, since it is the default.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422698299
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +141,14 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ for (int key : keys) {
+ if (mapping.getTargetOpt(key) == -1) {
+ return RANDOM_DISTRIBUTED; // Some distribution keys are not mapped => random.
Review comment:
Fixed
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422712608
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
I don't mind throwing `NoElementException`, if the desc in the javadoc is misleading. @julianhyde Can you confirm?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422697773
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
According to the doc, NoElementException should be when a mapping is expected to return one element but returns none. -1 means it is not mapped. Maybe we can just leave it as it is (and other places), let it throw ArrayIndexOutOfBoundsException.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422712366
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
yes.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422731546
##########
File path: core/src/main/java/org/apache/calcite/plan/RelTrait.java
##########
@@ -91,6 +93,17 @@
/**
* Applies a mapping to this trait.
*
+ * <p>Some traits may be changed if the columns order is changed by a mapping of the
+ * {@link Project} operator. </p>
+ *
+ * <p>For example, if relation {@code ORDER a, b} is sorted by columns [0, 1], then the
Review comment:
Is it a typo? `ORDER a, b`
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r419406480
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -81,6 +77,15 @@ public static RelDistribution of(RelDistribution.Type type, ImmutableIntList key
return RelDistributionTraitDef.INSTANCE.canonize(distribution);
}
+ public static ImmutableIntList normalizeKeys(Collection<? extends Number> numbers) {
Review comment:
Can you change it to private and add a comment?
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +140,16 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ List<Integer> mapList = Mappings.asList(mapping);
+ for (Integer key : keys) {
Review comment:
Is `int` OK here?
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +140,16 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ List<Integer> mapList = Mappings.asList(mapping);
+ for (Integer key : keys) {
+ if (mapList.get(key) == null) {
+ return RANDOM_DISTRIBUTED; // Some distribution keys are not mapped => random.
+ }
+ }
+ List<Integer> mappedKeys0 = Mappings.apply2((Mapping) mapping, keys);
+ ImmutableIntList mappedKeys = normalizeKeys(mappedKeys0);
+ RelDistribution newDistribution = new RelDistributionImpl(type, mappedKeys);
Review comment:
Use `of(type, keys)`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422697773
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
According to the doc, NoElementException should be thrown when a mapping is expected to return one element but returns none. -1 means it is not mapped. Maybe we can just leave it as it is (and other places), let it throw ArrayIndexOutOfBoundsException.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r419519409
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +141,14 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ for (int key : keys) {
+ if (mapping.getTargetOpt(key) == -1) {
Review comment:
I agree. Added fix for mappings.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422701078
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
Checking `AbstractMapping`, NoElementException is thrown when `getTargetOpt` or `getSourceOpt` returns -1, which is aligning with the javadoc.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422959689
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
I changed exception to `IndexOutOfBoundsException` when source/target is out of domain. Please, take a look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r419464886
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +141,14 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ for (int key : keys) {
+ if (mapping.getTargetOpt(key) == -1) {
Review comment:
Key might be greater than / equal with mapping.sourceCount (out of range). Haven't try to reproduce here in Calcite. Perhaps we can also add a check in TargetMapping implementation class, if the key >= sourceCount, also return -1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422959099
##########
File path: core/src/main/java/org/apache/calcite/plan/RelTrait.java
##########
@@ -91,6 +93,17 @@
/**
* Applies a mapping to this trait.
*
+ * <p>Some traits may be changed if the columns order is changed by a mapping of the
+ * {@link Project} operator. </p>
+ *
+ * <p>For example, if relation {@code ORDER a, b} is sorted by columns [0, 1], then the
Review comment:
Yes, this is a type. Fixed. Thank you.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] julianhyde commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
julianhyde commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422804623
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
`NoElementException` is for the case where the source is within the domain (0 .. sourceCount - 1) but no mapping is present.
If the source is outside the domain - less than 0 or greater than or equal to sourceCount - then `IndexOutOfBoundsException` would be appropriate.
I don't think `getSourceOpt` or `getTargetOpt` should ever throw `NoElementException`. They have a different way to communicate no element - namely by returning -1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r419456722
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +140,16 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ List<Integer> mapList = Mappings.asList(mapping);
+ for (Integer key : keys) {
+ if (mapList.get(key) == null) {
+ return RANDOM_DISTRIBUTED; // Some distribution keys are not mapped => random.
+ }
+ }
+ List<Integer> mappedKeys0 = Mappings.apply2((Mapping) mapping, keys);
+ ImmutableIntList mappedKeys = normalizeKeys(mappedKeys0);
+ RelDistribution newDistribution = new RelDistributionImpl(type, mappedKeys);
Review comment:
Good point! Thank you. Fixed.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] hsyuan commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
hsyuan commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r419464886
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +141,14 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ for (int key : keys) {
+ if (mapping.getTargetOpt(key) == -1) {
Review comment:
Key might be greater than / equal with `mapping.sourceCount` (out of range). Haven't try to reproduce here in Calcite. Perhaps we can also add a check in `TargetMapping.getTargetOpt()` of implementation class, if the `key >= sourceCount`, also return -1.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422709614
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
So, what do you propose? Just throw `ArrayIndexOutOfBoundsException`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r419455340
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -81,6 +77,15 @@ public static RelDistribution of(RelDistribution.Type type, ImmutableIntList key
return RelDistributionTraitDef.INSTANCE.canonize(distribution);
}
+ public static ImmutableIntList normalizeKeys(Collection<? extends Number> numbers) {
Review comment:
Done.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r422699962
##########
File path: core/src/main/java/org/apache/calcite/util/mapping/Mappings.java
##########
@@ -1245,12 +1271,30 @@ public void set(int source, int target) {
assert isValid();
}
+ /**
+ * Returns the source that a target maps to, or -1 if it is not mapped.
+ *
+ * @throws NoElementException if target is out of sources range.
+ */
public int getSourceOpt(int target) {
- return sources[target];
+ try {
+ return sources[target];
+ } catch (ArrayIndexOutOfBoundsException e) {
+ throw new Mappings.NoElementException("invalid target " + target);
Review comment:
Hmm.. it is also said in javadoc that `FunctionMapping#getTarget` throws `NoElementException if source is not mapped`. Which is closer to the truth. In this case `NoElementException` is valid move.
I also noticed that for some mappings it was an original behavior, i.e. `Permutation#getTarget()`. I just made the fix by analogy with this legacy behavior.
I agree that it is far from perfect. May be `IllegalArgumentException` is more suitable here. But I think that `ArrayIndexOutOfBoundsException` is not the best choice.
What if I just change the javadoc of `NoElementException` to `Should be thrown when source or target is not mapped`?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [calcite] rkondakov commented on a change in pull request #1959: [CALCITE-3969] Method RelTrait.apply(Mappings.Mapping) throws exception when mapping doesn't cover collation or distribution keys
Posted by GitBox <gi...@apache.org>.
rkondakov commented on a change in pull request #1959:
URL: https://github.com/apache/calcite/pull/1959#discussion_r419456514
##########
File path: core/src/main/java/org/apache/calcite/rel/RelDistributions.java
##########
@@ -135,10 +140,16 @@ public RelDistribution apply(Mappings.TargetMapping mapping) {
if (keys.isEmpty()) {
return this;
}
- return getTraitDef().canonize(
- new RelDistributionImpl(type,
- ImmutableIntList.copyOf(
- Mappings.apply2((Mapping) mapping, keys))));
+ List<Integer> mapList = Mappings.asList(mapping);
+ for (Integer key : keys) {
Review comment:
I replaced `Mappings.asList(mapping)` with `mapping.getTargetOpt(key)`, please take a look.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org