You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@calcite.apache.org by "michaelbraun (via GitHub)" <gi...@apache.org> on 2026/09/13 21:58:23 UTC
[PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
michaelbraun opened a new pull request, #5262:
URL: https://github.com/apache/calcite/pull/5262
## Jira Link
[CALCITE-7780](https://issues.apache.org/jira/browse/CALCITE-7780)
## Changes Proposed
Several places look up the same map key two or three times in a row, where one
call to an existing `Map` method does the same job. For example,
`HepPlanner#updateVertex`:
```java
if (mapDigestToVertex.get(oldKey) == vertex) {
mapDigestToVertex.remove(oldKey);
}
```
is `Map#remove(key, value)`:
```java
mapDigestToVertex.remove(oldKey, vertex);
```
The same shape occurs in a handful of other classes, each replaced with the
corresponding single-lookup idiom:
| Idiom | Sites |
| --- | --- |
| `Map#remove(key, value)` for get, compare, remove | `HepPlanner` |
| A single `get` plus null check for `containsKey` then `get` | `RexToLixTranslator` (5), `AggregateNode`, `CombineSimpleEquivalenceRule`, `RexUtil` (2), `RelStructuredTypeFlattener`, `MongoFilter`, `QueryBuilders` |
| `putIfAbsent` for `containsKey`/`get` then `put` | `RelOptUtil`, `SqlValidatorImpl`, `Strong` |
| `computeIfAbsent` for `containsKey`, `get` and `put` | `Match` |
| Iterating `entrySet`, and `Map.Entry#setValue` for in-place updates, for iterating `keySet` and calling `get`/`put` per key | `UncollectNode`, `LoptOptimizeJoinRule`, `RelStructuredTypeFlattener`, `RelDotWriter` |
| `containsKey`/`getOrDefault` for `keySet().contains` then `get` | `QueryBuilders` |
There is no functional change.
### Notes for review
* Every map involved holds non-null values, so replacing `containsKey` with a
null check on `get` preserves behaviour. In `MongoFilter` the `"$nin"` key is
written only by the three lines below the change, always with a non-null
`List`.
* `HepRelVertex` does not override `equals`, so the two-argument
`Map#remove` performs the identity comparison the old code did explicitly.
* Neither lambda introduced captures enclosing state, so both
`computeIfAbsent` call sites link a single instance and allocate nothing per
call. `AggregateNode#send` is left as an explicit `get`/null check rather than
`computeIfAbsent` precisely because the mapping function there would have to
read `accumulatorFactories` and so would allocate a capturing lambda on every
row.
* `RexUtil#combinePredicatesUsingOr` now updates through `Map.Entry#setValue`.
The method is private with a single call site that passes a `HashMap`, and the
`iterator.remove()` branch `continue`s, so `setValue` is never called on a
removed entry.
* In `RelStructuredTypeFlattener#updateRelInMap`, switching to `setValue` makes
the `ModifyCollectionInEnhancedForLoop` suppression unnecessary, so it is
removed.
* Some sites are on per-row paths (`AggregateNode#send`, `UncollectNode`, and
the expression caches in `RexToLixTranslator`); the rest is planner and
validator code. No benchmark is claimed here, only strictly less work.
* Left alone deliberately: `ReflectUtil#lookupVisitMethod` and
`CompositeMap#get`, which store or permit null values and so need
`containsKey`; and the several `if (!list.contains(x)) list.add(x)` sites,
where the collection is a `List` and the scan is doing real work.
`./gradlew build` passes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5262:
URL: https://github.com/apache/calcite/pull/5262#discussion_r4010797301
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -1454,8 +1454,9 @@ private static Expression scaleValue(
final Pair<RexNode, @Nullable Type> key = Pair.of(inputRef, currentStorageType);
// If the RexInputRef has been visited under current storage type already,
// it is not necessary to visit it again, just return the result.
- if (rexWithStorageTypeResultMap.containsKey(key)) {
- return rexWithStorageTypeResultMap.get(key);
+ final Result cached = rexWithStorageTypeResultMap.get(key);
Review Comment:
Fortunately we have `@Nullable` annotations, so if the ksy is not nullable, hopefully the invariant is maintained.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5262:
URL: https://github.com/apache/calcite/pull/5262#issuecomment-5656527176
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "michaelbraun (via GitHub)" <gi...@apache.org>.
michaelbraun commented on PR #5262:
URL: https://github.com/apache/calcite/pull/5262#issuecomment-5674356081
> Please squash the commits so we can merge
I believe it should be good to go now
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "michaelbraun (via GitHub)" <gi...@apache.org>.
michaelbraun commented on code in PR #5262:
URL: https://github.com/apache/calcite/pull/5262#discussion_r4001864060
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -1454,8 +1454,9 @@ private static Expression scaleValue(
final Pair<RexNode, @Nullable Type> key = Pair.of(inputRef, currentStorageType);
// If the RexInputRef has been visited under current storage type already,
// it is not necessary to visit it again, just return the result.
- if (rexWithStorageTypeResultMap.containsKey(key)) {
- return rexWithStorageTypeResultMap.get(key);
+ final Result cached = rexWithStorageTypeResultMap.get(key);
Review Comment:
In this case the values will always be non-null. Should I comment in these cases to make it more clear this is safe?
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu merged PR #5262:
URL: https://github.com/apache/calcite/pull/5262
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on code in PR #5262:
URL: https://github.com/apache/calcite/pull/5262#discussion_r4001156726
##########
core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java:
##########
@@ -1454,8 +1454,9 @@ private static Expression scaleValue(
final Pair<RexNode, @Nullable Type> key = Pair.of(inputRef, currentStorageType);
// If the RexInputRef has been visited under current storage type already,
// it is not necessary to visit it again, just return the result.
- if (rexWithStorageTypeResultMap.containsKey(key)) {
- return rexWithStorageTypeResultMap.get(key);
+ final Result cached = rexWithStorageTypeResultMap.get(key);
Review Comment:
Some Java maps allow null keys. Have you checked that this one doesn't?
If it does, the semantics is not the same.
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "julianhyde (via GitHub)" <gi...@apache.org>.
julianhyde commented on code in PR #5262:
URL: https://github.com/apache/calcite/pull/5262#discussion_r4007986089
##########
core/src/main/java/org/apache/calcite/interpreter/UncollectNode.java:
##########
@@ -61,16 +61,16 @@ public UncollectNode(Compiler compiler, Uncollect uncollect) {
}
}
} else if (value instanceof Map) {
- Map map = (Map) value;
+ Map<?, ?> map = (Map<?, ?>) value;
if (map.isEmpty() && rel.isOuter) {
sink.send(Row.of(new Object[width]));
continue;
}
- for (Object key : map.keySet()) {
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
Review Comment:
`Map.forEach(BiConsumer)` may be more efficient here
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5262:
URL: https://github.com/apache/calcite/pull/5262#issuecomment-5656514409
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.9% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5262:
URL: https://github.com/apache/calcite/pull/5262#issuecomment-5674587386
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "michaelbraun (via GitHub)" <gi...@apache.org>.
michaelbraun commented on code in PR #5262:
URL: https://github.com/apache/calcite/pull/5262#discussion_r4008776503
##########
core/src/main/java/org/apache/calcite/interpreter/UncollectNode.java:
##########
@@ -61,16 +61,16 @@ public UncollectNode(Compiler compiler, Uncollect uncollect) {
}
}
} else if (value instanceof Map) {
- Map map = (Map) value;
+ Map<?, ?> map = (Map<?, ?>) value;
if (map.isEmpty() && rel.isOuter) {
sink.send(Row.of(new Object[width]));
continue;
}
- for (Object key : map.keySet()) {
+ for (Map.Entry<?, ?> entry : map.entrySet()) {
Review Comment:
The int i is being modified within this loop, so that would need to be wrapped in order to modify it, if we went that route. entrySet seems like a better fit here
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "sonarqubecloud[bot] (via GitHub)" <gi...@apache.org>.
sonarqubecloud[bot] commented on PR #5262:
URL: https://github.com/apache/calcite/pull/5262#issuecomment-5665801082
## [](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262) **Quality Gate passed**
Issues
 [0 New issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [0 Accepted issues](https://sonarcloud.io/project/issues?id=apache_calcite&pullRequest=5262&issueStatuses=ACCEPTED)
Measures
 [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_calcite&pullRequest=5262&issueStatuses=OPEN,CONFIRMED&sinceLeakPeriod=true)
 [81.6% Coverage on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_coverage&view=list)
 [0.0% Duplication on New Code](https://sonarcloud.io/component_measures?id=apache_calcite&pullRequest=5262&metric=new_duplicated_lines_density&view=list)
<!-- sqra-placement-anchor -->
[See analysis details on SonarQube Cloud](https://sonarcloud.io/dashboard?id=apache_calcite&pullRequest=5262)
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CALCITE-7780] Avoid redundant map lookups [calcite]
Posted by "mihaibudiu (via GitHub)" <gi...@apache.org>.
mihaibudiu commented on PR #5262:
URL: https://github.com/apache/calcite/pull/5262#issuecomment-5672827748
Please squash the commits so we can merge
--
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: commits-unsubscribe@calcite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org