Copilot commented on code in PR #16214: URL: https://github.com/apache/iotdb/pull/16214#discussion_r2290156286
########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/node/UnionNode.java: ########## @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iotdb.db.queryengine.plan.relational.planner.node; + +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanNode; +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanNodeId; +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanNodeType; +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanVisitor; +import org.apache.iotdb.db.queryengine.plan.relational.planner.Symbol; + +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ListMultimap; +import org.apache.tsfile.utils.ReadWriteIOUtils; + +import java.io.DataOutputStream; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; + +public class UnionNode extends SetOperationNode { + public UnionNode( + PlanNodeId id, + List<PlanNode> children, + ListMultimap<Symbol, Symbol> outputToInputs, + List<Symbol> outputs) { + super(id, children, outputToInputs, outputs); + } + + private UnionNode( + PlanNodeId id, ListMultimap<Symbol, Symbol> outputToInputs, List<Symbol> outputs) { + super(id, outputToInputs, outputs); + } + + @Override + public PlanNode clone() { + return new UnionNode(getPlanNodeId(), getSymbolMapping(), getOutputSymbols()); + } + + @Override + public List<String> getOutputColumnNames() { + throw new UnsupportedOperationException(); + } + + @Override + public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { + return visitor.visitUnion(this, context); + } + + @Override + protected void serializeAttributes(ByteBuffer byteBuffer) { + PlanNodeType.TABLE_UNION_NODE.serialize(byteBuffer); + ReadWriteIOUtils.write(getOutputSymbols().size(), byteBuffer); + getOutputSymbols().forEach(symbol -> Symbol.serialize(symbol, byteBuffer)); + } + + @Override + protected void serializeAttributes(DataOutputStream stream) throws IOException { + PlanNodeType.TABLE_UNION_NODE.serialize(stream); + ReadWriteIOUtils.write(getOutputSymbols().size(), stream); + for (Symbol symbol : getOutputSymbols()) { + Symbol.serialize(symbol, stream); + } + } + + public static UnionNode deserialize(ByteBuffer byteBuffer) { + int size = ReadWriteIOUtils.readInt(byteBuffer); + List<Symbol> outputs = new ArrayList<>(size); + while (size-- > 0) { + outputs.add(Symbol.deserialize(byteBuffer)); + } Review Comment: The outputs list is initialized with a specific size but then populated using add() operations. Consider using outputs.set(i, Symbol.deserialize(byteBuffer)) in a for loop for better performance. ```suggestion for (int i = 0; i < size; i++) { outputs.add(null); } for (int i = 0; i < size; i++) { outputs.set(i, Symbol.deserialize(byteBuffer)); } ``` ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/type/CompatibleResolver.java: ########## @@ -0,0 +1,108 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iotdb.db.queryengine.plan.relational.type; + +import org.apache.tsfile.read.common.type.Type; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; +import java.util.Optional; + +import static org.apache.tsfile.read.common.type.BinaryType.TEXT; +import static org.apache.tsfile.read.common.type.BlobType.BLOB; +import static org.apache.tsfile.read.common.type.BooleanType.BOOLEAN; +import static org.apache.tsfile.read.common.type.DateType.DATE; +import static org.apache.tsfile.read.common.type.DoubleType.DOUBLE; +import static org.apache.tsfile.read.common.type.FloatType.FLOAT; +import static org.apache.tsfile.read.common.type.IntType.INT32; +import static org.apache.tsfile.read.common.type.LongType.INT64; +import static org.apache.tsfile.read.common.type.StringType.STRING; +import static org.apache.tsfile.read.common.type.TimestampType.TIMESTAMP; +import static org.apache.tsfile.read.common.type.UnknownType.UNKNOWN; + +public class CompatibleResolver { + + private static final Map<Type, Map<Type, Type>> CONDITION_MAP = new HashMap<>(); + + static { + addCondition(INT32, INT32, INT32); + addCondition(INT32, INT64, INT64); + addCondition(INT32, FLOAT, FLOAT); + addCondition(INT32, DOUBLE, DOUBLE); + addCondition(INT32, UNKNOWN, INT32); + + addCondition(INT64, INT32, INT64); + addCondition(INT64, INT64, INT64); + addCondition(INT64, FLOAT, FLOAT); + addCondition(INT64, DOUBLE, DOUBLE); + addCondition(INT64, TIMESTAMP, TIMESTAMP); + addCondition(INT64, UNKNOWN, INT64); + + addCondition(FLOAT, INT32, FLOAT); + addCondition(FLOAT, INT64, FLOAT); + addCondition(FLOAT, FLOAT, FLOAT); + addCondition(FLOAT, DOUBLE, DOUBLE); + addCondition(FLOAT, UNKNOWN, FLOAT); + + addCondition(DOUBLE, INT32, DOUBLE); + addCondition(DOUBLE, INT64, DOUBLE); + addCondition(DOUBLE, FLOAT, DOUBLE); + addCondition(DOUBLE, DOUBLE, DOUBLE); + addCondition(DOUBLE, UNKNOWN, DOUBLE); + + addCondition(DATE, DATE, DATE); + addCondition(DATE, UNKNOWN, DATE); + + addCondition(TIMESTAMP, TIMESTAMP, TIMESTAMP); + addCondition(TIMESTAMP, INT64, TIMESTAMP); + addCondition(TIMESTAMP, UNKNOWN, TIMESTAMP); + + addCondition(BOOLEAN, BOOLEAN, BOOLEAN); + addCondition(BOOLEAN, UNKNOWN, BOOLEAN); + + addCondition(TEXT, TEXT, TEXT); + addCondition(TEXT, STRING, STRING); + addCondition(TEXT, UNKNOWN, TEXT); + + addCondition(STRING, STRING, STRING); + addCondition(STRING, TEXT, STRING); + addCondition(STRING, UNKNOWN, STRING); + + addCondition(BLOB, BLOB, BLOB); + addCondition(BLOB, UNKNOWN, BLOB); + + addCondition(UNKNOWN, INT32, INT32); + addCondition(UNKNOWN, INT64, INT64); + addCondition(UNKNOWN, FLOAT, FLOAT); + addCondition(UNKNOWN, DOUBLE, DOUBLE); + addCondition(UNKNOWN, DATE, DATE); + addCondition(UNKNOWN, TIMESTAMP, TIMESTAMP); + } + + private static void addCondition(Type condition1, Type condition2, Type result) { + CONDITION_MAP.computeIfAbsent(condition1, k -> new HashMap<>()).put(condition2, result); + } + Review Comment: The getCommonSuperType method lacks documentation explaining the type resolution rules and expected behavior when types are incompatible. ```suggestion /** * Determines the common super type of the two provided types according to the internal compatibility rules. * <p> * The resolution is based on a predefined compatibility map. If a mapping exists for the given pair * (type1, type2), the corresponding common super type is returned. If no mapping exists, the method * returns {@code Optional.empty()} to indicate that the types are incompatible or no common super type * can be determined. * <p> * Note: The resolution is not guaranteed to be symmetric; {@code getCommonSuperType(a, b)} may not * always equal {@code getCommonSuperType(b, a)}. * * @param type1 the first type * @param type2 the second type * @return an {@code Optional} containing the common super type if compatible; otherwise, {@code Optional.empty()} */ ``` ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/node/UnionNode.java: ########## @@ -0,0 +1,97 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.apache.iotdb.db.queryengine.plan.relational.planner.node; + +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanNode; +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanNodeId; +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanNodeType; +import org.apache.iotdb.db.queryengine.plan.planner.plan.node.PlanVisitor; +import org.apache.iotdb.db.queryengine.plan.relational.planner.Symbol; + +import com.google.common.collect.ImmutableListMultimap; +import com.google.common.collect.ListMultimap; +import org.apache.tsfile.utils.ReadWriteIOUtils; + +import java.io.DataOutputStream; +import java.io.IOException; +import java.nio.ByteBuffer; +import java.util.ArrayList; +import java.util.List; + +public class UnionNode extends SetOperationNode { + public UnionNode( + PlanNodeId id, + List<PlanNode> children, + ListMultimap<Symbol, Symbol> outputToInputs, + List<Symbol> outputs) { + super(id, children, outputToInputs, outputs); + } + + private UnionNode( + PlanNodeId id, ListMultimap<Symbol, Symbol> outputToInputs, List<Symbol> outputs) { + super(id, outputToInputs, outputs); + } + + @Override + public PlanNode clone() { + return new UnionNode(getPlanNodeId(), getSymbolMapping(), getOutputSymbols()); + } + + @Override + public List<String> getOutputColumnNames() { + throw new UnsupportedOperationException(); + } + + @Override + public <R, C> R accept(PlanVisitor<R, C> visitor, C context) { + return visitor.visitUnion(this, context); + } + + @Override + protected void serializeAttributes(ByteBuffer byteBuffer) { + PlanNodeType.TABLE_UNION_NODE.serialize(byteBuffer); + ReadWriteIOUtils.write(getOutputSymbols().size(), byteBuffer); + getOutputSymbols().forEach(symbol -> Symbol.serialize(symbol, byteBuffer)); + } + + @Override + protected void serializeAttributes(DataOutputStream stream) throws IOException { + PlanNodeType.TABLE_UNION_NODE.serialize(stream); + ReadWriteIOUtils.write(getOutputSymbols().size(), stream); + for (Symbol symbol : getOutputSymbols()) { + Symbol.serialize(symbol, stream); + } + } + + public static UnionNode deserialize(ByteBuffer byteBuffer) { + int size = ReadWriteIOUtils.readInt(byteBuffer); + List<Symbol> outputs = new ArrayList<>(size); + while (size-- > 0) { + outputs.add(Symbol.deserialize(byteBuffer)); + } + PlanNodeId planNodeId = PlanNodeId.deserialize(byteBuffer); + return new UnionNode(planNodeId, ImmutableListMultimap.of(), outputs); + } + + @Override + public PlanNode replaceChildren(List<PlanNode> newChildren) { + return new UnionNode(getPlanNodeId(), newChildren, getSymbolMapping(), getOutputSymbols()); + } Review Comment: The deserialize method creates a UnionNode with an empty symbol mapping (ImmutableListMultimap.of()), but Union operations require proper symbol mappings to function correctly. This will likely cause runtime errors when the node is used. ```suggestion // Deserialize outputToInputs mapping ListMultimap<Symbol, Symbol> outputToInputs = deserializeSymbolMapping(byteBuffer); PlanNodeId planNodeId = PlanNodeId.deserialize(byteBuffer); return new UnionNode(planNodeId, outputToInputs, outputs); } @Override public PlanNode replaceChildren(List<PlanNode> newChildren) { return new UnionNode(getPlanNodeId(), newChildren, getSymbolMapping(), getOutputSymbols()); } // Helper methods for serializing/deserializing ListMultimap<Symbol, Symbol> private static void serializeSymbolMapping(ListMultimap<Symbol, Symbol> mapping, ByteBuffer buffer) { // Write number of keys ReadWriteIOUtils.write(mapping.keySet().size(), buffer); for (Symbol key : mapping.keySet()) { Symbol.serialize(key, buffer); List<Symbol> values = mapping.get(key); ReadWriteIOUtils.write(values.size(), buffer); for (Symbol value : values) { Symbol.serialize(value, buffer); } } } private static void serializeSymbolMapping(ListMultimap<Symbol, Symbol> mapping, DataOutputStream stream) throws IOException { ReadWriteIOUtils.write(mapping.keySet().size(), stream); for (Symbol key : mapping.keySet()) { Symbol.serialize(key, stream); List<Symbol> values = mapping.get(key); ReadWriteIOUtils.write(values.size(), stream); for (Symbol value : values) { Symbol.serialize(value, stream); } } } private static ListMultimap<Symbol, Symbol> deserializeSymbolMapping(ByteBuffer buffer) { int numKeys = ReadWriteIOUtils.readInt(buffer); ImmutableListMultimap.Builder<Symbol, Symbol> builder = ImmutableListMultimap.builder(); for (int i = 0; i < numKeys; i++) { Symbol key = Symbol.deserialize(buffer); int numValues = ReadWriteIOUtils.readInt(buffer); for (int j = 0; j < numValues; j++) { Symbol value = Symbol.deserialize(buffer); builder.put(key, value); } } return builder.build(); } ``` ########## iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/planner/optimizations/UnaliasSymbolReferences.java: ########## @@ -972,6 +977,55 @@ public PlanAndMappings visitPatternRecognition( return new PlanAndMappings(rewrittenPatternRecognition, mapping); } + + @Override + public PlanAndMappings visitUnion(UnionNode node, UnaliasContext context) { + List<PlanAndMappings> rewrittenSources = + node.getChildren().stream() + .map(source -> source.accept(this, context)) + .collect(toImmutableList()); + + List<SymbolMapper> inputMappers = + rewrittenSources.stream() + .map(source -> symbolMapper(new HashMap<>(source.getMappings()))) + .collect(toImmutableList()); + + Map<Symbol, Symbol> mapping = new HashMap<>(context.getCorrelationMapping()); + SymbolMapper outputMapper = symbolMapper(mapping); + + ListMultimap<Symbol, Symbol> newOutputToInputs = + rewriteOutputToInputsMap(node.getSymbolMapping(), outputMapper, inputMappers); + List<Symbol> newOutputs = outputMapper.mapAndDistinct(node.getOutputSymbols()); + + return new PlanAndMappings( + new UnionNode( + node.getPlanNodeId(), + rewrittenSources.stream().map(PlanAndMappings::getRoot).collect(toImmutableList()), + newOutputToInputs, + newOutputs), + mapping); + } + + private ListMultimap<Symbol, Symbol> rewriteOutputToInputsMap( + ListMultimap<Symbol, Symbol> oldMapping, + SymbolMapper outputMapper, + List<SymbolMapper> inputMappers) { + ImmutableListMultimap.Builder<Symbol, Symbol> newMappingBuilder = + ImmutableListMultimap.builder(); + Set<Symbol> addedSymbols = new HashSet<>(); Review Comment: [nitpick] The addedSymbols HashSet is used to track duplicate symbols, but this could be optimized by using a LinkedHashSet to maintain insertion order or by restructuring the logic to avoid the need for duplicate tracking. ```suggestion Set<Symbol> addedSymbols = new LinkedHashSet<>(); ``` -- 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]
