[CALCITE-7780] Avoid redundant map lookups - #5262
Conversation
22f7113 to
c98bfc1
Compare
| // 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); |
There was a problem hiding this comment.
Some Java maps allow null keys. Have you checked that this one doesn't?
If it does, the semantics is not the same.
There was a problem hiding this 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?
There was a problem hiding this comment.
Fortunately we have @Nullable annotations, so if the ksy is not nullable, hopefully the invariant is maintained.
| continue; | ||
| } | ||
| for (Object key : map.keySet()) { | ||
| for (Map.Entry<?, ?> entry : map.entrySet()) { |
There was a problem hiding this comment.
Map.forEach(BiConsumer) may be more efficient here
There was a problem hiding this 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
|
Please squash the commits so we can merge |
mihaibudiu
added
the
LGTM-will-merge-soon
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 removes a digest only if it still maps to the vertex being updated, which is Map#remove(key, value). The same shape occurs in a handful of other classes, and is replaced with the corresponding single-lookup idiom: a get plus null check for containsKey followed by get; putIfAbsent or computeIfAbsent for containsKey followed by put; iteration over entrySet, with Map.Entry#setValue for in-place updates, for iterating keySet and calling get per key. Some sites are on per-row paths (AggregateNode#send, UncollectNode and the expression caches in RexToLixTranslator); the rest is planner and validator code. No lambda introduced captures enclosing state, so the computeIfAbsent call sites allocate nothing. All values involved are non-null, so replacing containsKey with a null check on get preserves behaviour. There is no functional change. MongoFilter and QueryBuilders were reverted from this change: in MongoFilter the map is declared Map<String, @nullable Object>, and in QueryBuilders the map is supplied by the caller, so in neither case is a non-null value guaranteed by the type. Treating "absent" and "mapped to null" alike is safe for the keys these two sites use, but it rests on an invariant rather than on the declaration, so leave both as they were. Strong#createPolicyMap is reverted for a different reason: the map is an EnumMap, which does not override putIfAbsent, so the default Map implementation still performs a get followed by a put. No lookup is saved. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
ae327f3 to
f6d1750
Compare
I believe it should be good to go now |
|
3 participants
Jira Link
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
Mapmethod does the same job. For example,HepPlanner#updateVertex:is
Map#remove(key, value):The same shape occurs in a handful of other classes, each replaced with the
corresponding single-lookup idiom: a
getplus null check forcontainsKeythen
get;putIfAbsentorcomputeIfAbsentforcontainsKeythenput;iterating
entrySet, withMap.Entry#setValuefor in-place updates, foriterating
keySetand callinggetper key.There is no functional change, so no tests are added; the existing suites cover
these paths. Every map involved holds non-null values, so replacing
containsKeywith a null check ongetpreserves behaviour, andHepRelVertexdoes not overrideequals, so the two-argumentMap#removeperforms the identity comparison the old code did explicitly.
The single lambda introduced, in
Match, captures nothing, so thatcomputeIfAbsentcall site allocates nothing per call.AggregateNode#sendkeeps an explicit
getplus null check for the same reason: a mapping functionthere would have to read
accumulatorFactories, allocating a capturing lambdaon every row.
Some sites are on per-row paths (
AggregateNode#send,UncollectNode, and theexpression caches in
RexToLixTranslator); the rest is planner and validatorcode. No benchmark is claimed, only strictly less work.
./gradlew buildpasses.🤖 Generated with Claude Code