From f6d1750b71153f920d6e4bb3edb1cc7c15f3b291 Mon Sep 17 00:00:00 2001 From: Michael Braun Date: Mon, 14 Sep 2026 23:36:35 -0400 Subject: [PATCH] [CALCITE-7780] Avoid redundant map lookups 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, 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 --- .../enumerable/RexToLixTranslator.java | 25 +++++++++++------- .../calcite/interpreter/AggregateNode.java | 7 ++--- .../calcite/interpreter/UncollectNode.java | 8 +++--- .../org/apache/calcite/plan/RelOptUtil.java | 5 ++-- .../apache/calcite/plan/hep/HepPlanner.java | 4 +-- .../org/apache/calcite/rel/core/Match.java | 9 ++----- .../calcite/rel/externalize/RelDotWriter.java | 8 +++--- .../rules/CombineSimpleEquivalenceRule.java | 5 ++-- .../rel/rules/LoptOptimizeJoinRule.java | 5 ++-- .../java/org/apache/calcite/rex/RexUtil.java | 26 +++++++------------ .../sql/validate/SqlValidatorImpl.java | 3 +-- .../sql2rel/RelStructuredTypeFlattener.java | 17 ++++++------ 12 files changed, 58 insertions(+), 64 deletions(-) diff --git a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java index 57ac8495c819..90437caa75a9 100644 --- a/core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java +++ b/core/src/main/java/org/apache/calcite/adapter/enumerable/RexToLixTranslator.java @@ -1454,8 +1454,9 @@ private static Expression scaleValue( final Pair 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); + if (cached != null) { + return cached; } // Generate one line of code to get the input, e.g., // "final Employee current =(Employee) inputEnumerator.current();" @@ -1519,8 +1520,9 @@ private static Expression scaleValue( */ @Override public Result visitLiteral(RexLiteral literal) { // If the RexLiteral has been visited already, just return the result - if (rexResultMap.containsKey(literal)) { - return rexResultMap.get(literal); + final Result cached = rexResultMap.get(literal); + if (cached != null) { + return cached; } // Generate one line of code for the value of RexLiteral, e.g., // "final int literal_value = 10;" @@ -1613,8 +1615,9 @@ private ConstantExpression getTypedNullLiteral(RexLiteral literal) { * need to be implemented separately. */ @Override public Result visitCall(RexCall call) { - if (rexResultMap.containsKey(call)) { - return rexResultMap.get(call); + final Result cached = rexResultMap.get(call); + if (cached != null) { + return cached; } final SqlOperator operator = call.getOperator(); if (operator == PREV) { @@ -1812,8 +1815,9 @@ private Result toInnerStorageType(Result result, Type storageType) { @Override public Result visitDynamicParam(RexDynamicParam dynamicParam) { final Pair key = Pair.of(dynamicParam, currentStorageType); - if (rexWithStorageTypeResultMap.containsKey(key)) { - return rexWithStorageTypeResultMap.get(key); + final Result cached = rexWithStorageTypeResultMap.get(key); + if (cached != null) { + return cached; } final Type valueType = typeFactory.getJavaClass(dynamicParam.getType()); final Type storageType = currentStorageType != null ? currentStorageType : valueType; @@ -1857,8 +1861,9 @@ private Result toInnerStorageType(Result result, Type storageType) { @Override public Result visitFieldAccess(RexFieldAccess fieldAccess) { final Pair key = Pair.of(fieldAccess, currentStorageType); - if (rexWithStorageTypeResultMap.containsKey(key)) { - return rexWithStorageTypeResultMap.get(key); + final Result cached = rexWithStorageTypeResultMap.get(key); + if (cached != null) { + return cached; } final RexNode target = deref(fieldAccess.getReferenceExpr()); int fieldIndex = fieldAccess.getField().getIndex(); diff --git a/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java b/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java index 684baad8d493..1e4a97040cd0 100644 --- a/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java +++ b/core/src/main/java/org/apache/calcite/interpreter/AggregateNode.java @@ -382,15 +382,16 @@ public void send(Row row) { } Row key = builder.build(); - if (!accumulators.containsKey(key)) { - AccumulatorList list = new AccumulatorList(); + AccumulatorList list = accumulators.get(key); + if (list == null) { + list = new AccumulatorList(); for (AccumulatorFactory factory : accumulatorFactories) { list.add(factory.get()); } accumulators.put(key, list); } - accumulators.get(key).send(row); + list.send(row); } public void end(Sink sink) throws InterruptedException { diff --git a/core/src/main/java/org/apache/calcite/interpreter/UncollectNode.java b/core/src/main/java/org/apache/calcite/interpreter/UncollectNode.java index 725f577c4a79..38372df6304c 100644 --- a/core/src/main/java/org/apache/calcite/interpreter/UncollectNode.java +++ b/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()) { if (rel.withOrdinality) { - sink.send(Row.of(key, map.get(key), i++)); + sink.send(Row.of(entry.getKey(), entry.getValue(), i++)); } else { - sink.send(Row.of(key, map.get(key))); + sink.send(Row.of(entry.getKey(), entry.getValue())); } } } else { diff --git a/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java b/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java index 42e86b5c4be5..d92a43659526 100644 --- a/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java +++ b/core/src/main/java/org/apache/calcite/plan/RelOptUtil.java @@ -2773,8 +2773,9 @@ public static void inferViewPredicates(Map projectMap, } if (o0 instanceof RexInputRef && o1 instanceof RexLiteral) { final int index = ((RexInputRef) o0).getIndex(); - if (projectMap.get(index) == null) { - projectMap.put(index, o1); + // The first constraint on a column populates projectMap; a later + // constraint on the same column remains in filters. + if (projectMap.putIfAbsent(index, o1) == null) { continue; } } diff --git a/core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java b/core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java index d2a39d4a43c9..6952fedb0ae8 100644 --- a/core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java +++ b/core/src/main/java/org/apache/calcite/plan/hep/HepPlanner.java @@ -1080,9 +1080,7 @@ private void updateVertex(HepRelVertex vertex, RelNode rel) { notifyDiscard(vertex.getCurrentRel()); } RelDigest oldKey = vertex.getCurrentRel().getRelDigest(); - if (mapDigestToVertex.get(oldKey) == vertex) { - mapDigestToVertex.remove(oldKey); - } + mapDigestToVertex.remove(oldKey, vertex); // When a transformation happened in one rule apply, support // vertex2 replace vertex1, but the current relNode of // vertex1 and vertex2 is same, diff --git a/core/src/main/java/org/apache/calcite/rel/core/Match.java b/core/src/main/java/org/apache/calcite/rel/core/Match.java index a0d106b2096b..1fd61e72279c 100644 --- a/core/src/main/java/org/apache/calcite/rel/core/Match.java +++ b/core/src/main/java/org/apache/calcite/rel/core/Match.java @@ -260,13 +260,8 @@ private static class AggregateFinder extends RexVisitorImpl { pv.add(STAR); } for (String alpha : pv) { - final NavigableSet set; - if (aggregateCallsPerVar.containsKey(alpha)) { - set = aggregateCallsPerVar.get(alpha); - } else { - set = new TreeSet<>(); - aggregateCallsPerVar.put(alpha, set); - } + final NavigableSet set = + aggregateCallsPerVar.computeIfAbsent(alpha, k -> new TreeSet<>()); boolean update = true; for (RexMRAggCall rex : set) { if (rex.equals(aggCall)) { diff --git a/core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java b/core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java index f354bda24637..46dee53efa69 100644 --- a/core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java +++ b/core/src/main/java/org/apache/calcite/rel/externalize/RelDotWriter.java @@ -31,6 +31,7 @@ import java.io.PrintWriter; import java.util.ArrayList; +import java.util.Collection; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.List; @@ -187,9 +188,10 @@ private void explainInputs(List inputs) { pw.println("digraph {"); // print nodes with styles - for (RelNode rel : nodeStyles.keySet()) { - String style = String.join(",", nodeStyles.get(rel)); - pw.println(nodeLabels.get(rel) + " [style=\"" + style + "\"]"); + for (Map.Entry> entry + : nodeStyles.asMap().entrySet()) { + String style = String.join(",", entry.getValue()); + pw.println(nodeLabels.get(entry.getKey()) + " [style=\"" + style + "\"]"); } // ordinary arcs diff --git a/core/src/main/java/org/apache/calcite/rel/rules/CombineSimpleEquivalenceRule.java b/core/src/main/java/org/apache/calcite/rel/rules/CombineSimpleEquivalenceRule.java index eba15bcddc24..86166c9639cc 100644 --- a/core/src/main/java/org/apache/calcite/rel/rules/CombineSimpleEquivalenceRule.java +++ b/core/src/main/java/org/apache/calcite/rel/rules/CombineSimpleEquivalenceRule.java @@ -170,9 +170,8 @@ private static RelHomogeneousShuttle getReplacer( @Override public RelNode visit(RelNode node) { // Check if this node's digest matches any of our shared components RelDigest nodeDigest = node.getRelDigest(); - if (digestToSpool.containsKey(nodeDigest)) { - LogicalTableSpool spool = digestToSpool.get(nodeDigest); - + LogicalTableSpool spool = digestToSpool.get(nodeDigest); + if (spool != null) { if (producers.contains(nodeDigest)) { // Subsequent occurrence - replace with table scan (consumer) return LogicalTableScan.create( diff --git a/core/src/main/java/org/apache/calcite/rel/rules/LoptOptimizeJoinRule.java b/core/src/main/java/org/apache/calcite/rel/rules/LoptOptimizeJoinRule.java index 16cb367ac2fe..44a7ff830939 100644 --- a/core/src/main/java/org/apache/calcite/rel/rules/LoptOptimizeJoinRule.java +++ b/core/src/main/java/org/apache/calcite/rel/rules/LoptOptimizeJoinRule.java @@ -329,8 +329,9 @@ private static void findRemovableSelfJoins(RelMetadataQuery mq, LoptMultiJoin mu // From the candidate self-join pairs, determine if there is // the appropriate join condition between the two factors that will // allow the join to be removed. - for (Integer factor1 : selfJoinPairs.keySet()) { - final int factor2 = selfJoinPairs.get(factor1); + for (Map.Entry selfJoinPair : selfJoinPairs.entrySet()) { + final int factor1 = selfJoinPair.getKey(); + final int factor2 = selfJoinPair.getValue(); final List selfJoinFilters = new ArrayList<>(); for (RexNode filter : multiJoin.getJoinFilters()) { ImmutableBitSet joinFactors = diff --git a/core/src/main/java/org/apache/calcite/rex/RexUtil.java b/core/src/main/java/org/apache/calcite/rex/RexUtil.java index 99045f0a3cb6..29b63409ad6c 100644 --- a/core/src/main/java/org/apache/calcite/rex/RexUtil.java +++ b/core/src/main/java/org/apache/calcite/rex/RexUtil.java @@ -3197,14 +3197,12 @@ private void combinePredicatesUsingAnd( Map baseMap, Map forMergeMap) { for (Map.Entry entry : forMergeMap.entrySet()) { + final RexNode baseRex = baseMap.get(entry.getKey()); RexNode mergedRex = - relBuilder.and( - entry.getValue(), - baseMap.getOrDefault(entry.getKey(), relBuilder.literal(true))); + relBuilder.and(entry.getValue(), + baseRex == null ? relBuilder.literal(true) : baseRex); int originalCount = entry.getValue().nodeCount() - + (baseMap.containsKey(entry.getKey()) - ? baseMap.get(entry.getKey()).nodeCount() - : 0); + + (baseRex == null ? 0 : baseRex.nodeCount()); checkExpandCount(mergedRex.nodeCount() - originalCount); baseMap.put(entry.getKey(), mergedRex); } @@ -3233,23 +3231,17 @@ private void combinePredicatesUsingOr( Iterator> iterator = baseMap.entrySet().iterator(); while (iterator.hasNext()) { - int forMergeNodeCount = 0; - Map.Entry entry = iterator.next(); - if (!forMergeMap.containsKey(entry.getKey())) { + final RexNode forMergeRex = forMergeMap.get(entry.getKey()); + if (forMergeRex == null) { checkExpandCount(-entry.getValue().nodeCount()); iterator.remove(); continue; - } else { - forMergeNodeCount = forMergeMap.get(entry.getKey()).nodeCount(); } - RexNode mergedRex = - relBuilder.or( - entry.getValue(), - forMergeMap.get(entry.getKey())); - int originalCount = entry.getValue().nodeCount() + forMergeNodeCount; + RexNode mergedRex = relBuilder.or(entry.getValue(), forMergeRex); + int originalCount = entry.getValue().nodeCount() + forMergeRex.nodeCount(); checkExpandCount(mergedRex.nodeCount() - originalCount); - baseMap.put(entry.getKey(), mergedRex); + entry.setValue(mergedRex); } } diff --git a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java index 85a0f535a2ec..78c9d0d5224a 100644 --- a/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java +++ b/core/src/main/java/org/apache/calcite/sql/validate/SqlValidatorImpl.java @@ -8793,8 +8793,7 @@ private SqlNode expandAliases(SqlIdentifier id, CalciteContextException ex) { if (expr instanceof SqlIdentifier) { expr = getScope().fullyQualify((SqlIdentifier) expr).identifier; } - if (!expansions.containsKey(name)) { - expansions.put(name, expr); + if (expansions.putIfAbsent(name, expr) == null) { validator.setOriginal(expr, id); } return expr; diff --git a/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java b/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java index ae095af32dc8..713aeb9be2ac 100644 --- a/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java +++ b/core/src/main/java/org/apache/calcite/sql2rel/RelStructuredTypeFlattener.java @@ -197,23 +197,24 @@ private RelNode getCurrentRelOrThrow() { public void updateRelInMap( SortedSetMultimap mapRefRelToCorVar) { for (RelNode rel : Lists.newArrayList(mapRefRelToCorVar.keySet())) { - if (oldToNewRelMap.containsKey(rel)) { + RelNode newRel = oldToNewRelMap.get(rel); + if (newRel != null) { SortedSet corVarSet = mapRefRelToCorVar.removeAll(rel); - mapRefRelToCorVar.putAll(oldToNewRelMap.get(rel), corVarSet); + mapRefRelToCorVar.putAll(newRel, corVarSet); } } } - @SuppressWarnings({"JdkObsolete", "ModifyCollectionInEnhancedForLoop"}) + @SuppressWarnings("JdkObsolete") public void updateRelInMap( SortedMap mapCorVarToCorRel) { - for (CorrelationId corVar : mapCorVarToCorRel.keySet()) { - LogicalCorrelate oldRel = mapCorVarToCorRel.get(corVar); - if (oldToNewRelMap.containsKey(oldRel)) { - RelNode newRel = oldToNewRelMap.get(oldRel); + for (Map.Entry entry + : mapCorVarToCorRel.entrySet()) { + RelNode newRel = oldToNewRelMap.get(entry.getValue()); + if (newRel != null) { assert newRel instanceof LogicalCorrelate; - mapCorVarToCorRel.put(corVar, (LogicalCorrelate) newRel); + entry.setValue((LogicalCorrelate) newRel); } } }