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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
