ericpai commented on a change in pull request #5273: URL: https://github.com/apache/iotdb/pull/5273#discussion_r830722121
########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/CompareGreaterEqualTransformer.java ########## @@ -0,0 +1,35 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class CompareGreaterEqualTransformer extends CompareOperatorTransformer { + + public CompareGreaterEqualTransformer( + LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { + return leftOperand >= rightOperand ? 1.0 : 0.0; Review comment: It's not suggested that use `>=` to compare double directly, as there may be precision loss. Please reference the review comment of `CompareOperatorTransformer` and change like this ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return Double.compare(leftOperand, rightOperand) >= 0; } ########## File path: antlr/src/main/antlr4/org/apache/iotdb/db/qp/sql/SqlLexer.g4 ########## @@ -827,7 +827,8 @@ MOD : '%'; // Operators. Comparation -OPERATOR_EQ : '=' | '=='; +OPERATOR_DEQ : '=='; Review comment: What's the real difference between `==` and `=` ? Are they alias mutually? ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/LogicNotTransformer.java ########## @@ -0,0 +1,78 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.exception.query.QueryProcessException; +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; +import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType; + +import java.io.IOException; + +public class LogicNotTransformer extends Transformer { + private final LayerPointReader layerPointReader; + + public LogicNotTransformer(LayerPointReader layerPointReader) { + this.layerPointReader = layerPointReader; + } + + @Override + public boolean isConstantPointReader() { + return layerPointReader.isConstantPointReader(); + } + + @Override + protected boolean cacheValue() throws QueryProcessException, IOException { + if (!layerPointReader.next()) { + return false; + } + cachedTime = layerPointReader.currentTime(); + if (layerPointReader.isCurrentNull()) { + currentNull = true; + } else { + switch (layerPointReader.getDataType()) { + case INT32: + cachedBoolean = layerPointReader.currentInt() == 0; + break; + case INT64: + cachedBoolean = layerPointReader.currentLong() == 0L; + break; + case FLOAT: + cachedBoolean = layerPointReader.currentFloat() == 0.0f; + break; + case DOUBLE: + cachedBoolean = layerPointReader.currentDouble() == 0.0; Review comment: Use `Double.compare` instead. ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/LogicNotTransformer.java ########## @@ -0,0 +1,78 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.exception.query.QueryProcessException; +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; +import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType; + +import java.io.IOException; + +public class LogicNotTransformer extends Transformer { + private final LayerPointReader layerPointReader; + + public LogicNotTransformer(LayerPointReader layerPointReader) { + this.layerPointReader = layerPointReader; + } + + @Override + public boolean isConstantPointReader() { + return layerPointReader.isConstantPointReader(); + } + + @Override + protected boolean cacheValue() throws QueryProcessException, IOException { + if (!layerPointReader.next()) { + return false; + } + cachedTime = layerPointReader.currentTime(); + if (layerPointReader.isCurrentNull()) { + currentNull = true; + } else { + switch (layerPointReader.getDataType()) { + case INT32: + cachedBoolean = layerPointReader.currentInt() == 0; + break; + case INT64: + cachedBoolean = layerPointReader.currentLong() == 0L; + break; + case FLOAT: + cachedBoolean = layerPointReader.currentFloat() == 0.0f; Review comment: Use `Float.compare` instead. ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/CompareEqualToTransformer.java ########## @@ -0,0 +1,35 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class CompareEqualToTransformer extends CompareOperatorTransformer { + + public CompareEqualToTransformer( + LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { + return leftOperand == rightOperand ? 1.0 : 0.0; Review comment: It's not suggested that use `==` to compare double directly, as there may be precision loss. Please reference the review comment of `CompareOperatorTransformer` and change like this ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return Double.compare(leftOperand, rightOperand) == 0; } ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/CompareGreaterThanTransformer.java ########## @@ -0,0 +1,35 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class CompareGreaterThanTransformer extends CompareOperatorTransformer { + + public CompareGreaterThanTransformer( + LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { + return leftOperand > rightOperand ? 1.0 : 0.0; Review comment: Same as above. ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return Double.compare(leftOperand, rightOperand) == 1; } ``` ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/CompareNonEqualTransformer.java ########## @@ -0,0 +1,35 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class CompareNonEqualTransformer extends CompareOperatorTransformer { + + public CompareNonEqualTransformer( + LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { + return leftOperand != rightOperand ? 1.0 : 0.0; Review comment: Same as above. ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return Double.compare(leftOperand, rightOperand) != 0; } ``` ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/CompareLessEqualTransformer.java ########## @@ -0,0 +1,35 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class CompareLessEqualTransformer extends CompareOperatorTransformer { + + public CompareLessEqualTransformer( + LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { + return leftOperand <= rightOperand ? 1.0 : 0.0; Review comment: Same as above. ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return Double.compare(leftOperand, rightOperand) <= 0; } ``` ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/CompareOperatorTransformer.java ########## @@ -0,0 +1,37 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; +import org.apache.iotdb.tsfile.file.metadata.enums.TSDataType; + +public abstract class CompareOperatorTransformer extends ArithmeticBinaryTransformer { Review comment: As `CompareOperatorTransformer` always accepts two inputs and output a boolean result. It's not good to inherit `ArithmeticBinaryTransformer` directly as it's return double. Maybe we can define a new base class called `LogicBinaryTransformer` which returns boolean directly? ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/CompareLessThanTransformer.java ########## @@ -0,0 +1,35 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class CompareLessThanTransformer extends CompareOperatorTransformer { + + public CompareLessThanTransformer( + LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { + return leftOperand < rightOperand ? 1.0 : 0.0; Review comment: Same as above. ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return Double.compare(leftOperand, rightOperand) < 0; } ``` ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/LogicAndTransformer.java ########## @@ -0,0 +1,34 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class LogicAndTransformer extends CompareOperatorTransformer { + + public LogicAndTransformer(LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { + return (leftOperand == 1.0) && (rightOperand == 1.0) ? 1.0 : 0.0; Review comment: Please reference the review comment of `CompareOperatorTransformer` and change like this ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return leftOperand && rightOperand; } ``` ########## File path: server/src/main/java/org/apache/iotdb/db/query/udf/core/transformer/LogicOrTransformer.java ########## @@ -0,0 +1,34 @@ +/* + * 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.query.udf.core.transformer; + +import org.apache.iotdb.db.query.udf.core.reader.LayerPointReader; + +public class LogicOrTransformer extends CompareOperatorTransformer { + + public LogicOrTransformer(LayerPointReader leftPointReader, LayerPointReader rightPointReader) { + super(leftPointReader, rightPointReader); + } + + @Override + protected double evaluate(double leftOperand, double rightOperand) { Review comment: Please reference the review comment of `CompareOperatorTransformer` and change like this ```suggestion protected boolean evaluate(boolean leftOperand, boolean rightOperand) { return leftOperand || rightOperand; } -- 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]
