Copilot commented on code in PR #16917:
URL: https://github.com/apache/iotdb/pull/16917#discussion_r2634077513
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/predicate/ConvertPredicateToFilterVisitor.java:
##########
@@ -77,6 +81,10 @@
import static
org.apache.iotdb.db.queryengine.plan.relational.analyzer.predicate.PredicatePushIntoScanChecker.isSymbolReference;
import static
org.apache.iotdb.db.queryengine.plan.relational.planner.ir.GlobalTimePredicateExtractVisitor.isExtractTimeColumn;
import static
org.apache.iotdb.db.queryengine.plan.relational.planner.ir.GlobalTimePredicateExtractVisitor.isTimeColumn;
+import static org.apache.tsfile.enums.TSDataType.FLOAT;
Review Comment:
The FLOAT import is unused in this file. While FLOAT is referenced in the
getValue method at line 350, it doesn't need to be statically imported since
it's accessed via dataType.getTypeEnum(). Consider removing this unused static
import to keep the imports clean.
```suggestion
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/expression/visitor/predicate/ConvertPredicateToFilterVisitor.java:
##########
@@ -265,7 +273,131 @@ private <T extends Comparable<T>> Filter
constructCompareFilter(
Context context) {
PartialPath path = ((TimeSeriesOperand) timeseriesOperand).getPath();
int measurementIndex = context.getMeasurementIndex(path.getMeasurement());
- TSDataType dataType = context.getType(path);
+ TSDataType columnDataType = context.getType(path);
+
+ ConstantOperand constantLiteral = (ConstantOperand) constantOperand;
+ TSDataType constantDataType = constantLiteral.getDataType();
+ String constantString = constantLiteral.getValueString();
+ if (constantDataType == DOUBLE && NumberUtils.isCreatable(constantString))
{
+ BigDecimal constantDecimal = new BigDecimal(constantString);
+
+ if (columnDataType == INT64) {
+ if (constantDecimal.compareTo(new BigDecimal(Long.MAX_VALUE)) > 0) {
+ return constructFilterForGreaterThanMax(expressionType,
measurementIndex);
+ }
+ if (constantDecimal.compareTo(new BigDecimal(Long.MIN_VALUE)) < 0) {
+ return constructFilterForLessThanMin(expressionType,
measurementIndex);
+ }
+ return constructFilterFromDouble(
+ expressionType, constantOperand, measurementIndex, columnDataType);
+
+ } else if (columnDataType == INT32) {
+ if (constantDecimal.compareTo(new BigDecimal(Integer.MAX_VALUE)) > 0) {
+ return constructFilterForGreaterThanMax(expressionType,
measurementIndex);
+ }
+ if (constantDecimal.compareTo(new BigDecimal(Integer.MIN_VALUE)) < 0) {
+ return constructFilterForLessThanMin(expressionType,
measurementIndex);
+ }
+ return constructFilterFromDouble(
+ expressionType, constantOperand, measurementIndex, columnDataType);
+ }
+ }
Review Comment:
The code uses NumberUtils.isCreatable() to validate the constant string, but
this check is insufficient for handling special double values.
NumberUtils.isCreatable() returns true for strings like "Infinity",
"-Infinity", and "NaN", which would then be parsed into BigDecimal and cause
issues. BigDecimal constructor throws NumberFormatException for these special
values, but they should be handled explicitly. Consider adding specific checks
for these cases or handling the NumberFormatException properly to avoid
unexpected behavior.
##########
iotdb-core/datanode/src/test/java/org/apache/iotdb/db/queryengine/expression/predicate/TreePredicateConversionTest.java:
##########
@@ -0,0 +1,572 @@
+/*
+ * 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.expression.predicate;
+
+import org.apache.iotdb.commons.exception.IllegalPathException;
+import org.apache.iotdb.commons.path.PartialPath;
+import org.apache.iotdb.db.queryengine.plan.analyze.PredicateUtils;
+import org.apache.iotdb.db.queryengine.plan.analyze.TypeProvider;
+import org.apache.iotdb.db.queryengine.plan.expression.Expression;
+import
org.apache.iotdb.db.queryengine.plan.expression.binary.EqualToExpression;
+import
org.apache.iotdb.db.queryengine.plan.expression.binary.GreaterEqualExpression;
+import
org.apache.iotdb.db.queryengine.plan.expression.binary.GreaterThanExpression;
+import
org.apache.iotdb.db.queryengine.plan.expression.binary.LessEqualExpression;
+import
org.apache.iotdb.db.queryengine.plan.expression.binary.LessThanExpression;
+import
org.apache.iotdb.db.queryengine.plan.expression.binary.NonEqualExpression;
+import org.apache.iotdb.db.queryengine.plan.expression.leaf.ConstantOperand;
+import org.apache.iotdb.db.queryengine.plan.expression.leaf.TimeSeriesOperand;
+
+import org.apache.tsfile.enums.TSDataType;
+import org.apache.tsfile.read.filter.basic.Filter;
+import org.apache.tsfile.read.filter.operator.FalseLiteralFilter;
+import org.apache.tsfile.read.filter.operator.LongFilterOperators;
+import org.apache.tsfile.read.filter.operator.ValueIsNotNullOperator;
+import org.junit.Assert;
+import org.junit.Test;
+import org.mockito.Mockito;
+
+import java.util.Collections;
+
+public class TreePredicateConversionTest {
+
+ Expression int64Column = new TimeSeriesOperand(new
PartialPath("root.sg.d1.Int64Column"));
+ Expression int32Column = new TimeSeriesOperand(new
PartialPath("root.sg.d1.Int32Column"));
+
+ public TreePredicateConversionTest() throws IllegalPathException {}
+
+ private TypeProvider getMockTypeProvider(TSDataType dataType) {
+ TypeProvider mockTypeProvider = Mockito.mock(TypeProvider.class);
+
Mockito.when(mockTypeProvider.getTreeModelType(Mockito.anyString())).thenReturn(dataType);
+ return mockTypeProvider;
+ }
+
+ @Test
+ public void testDoubleLiteralAndInt64Column() {
+ Expression doubleConstant = new ConstantOperand(TSDataType.DOUBLE,
"123.3");
+
+ // 1. GreaterEqual (>=): int64 >= 123.3 -> int64 >= 124 (ceil)
+ // Result: ValueGtEq(124)
+ Expression greaterEqualExpression = new
GreaterEqualExpression(int64Column, doubleConstant);
+ Filter actualFilter1 =
+ PredicateUtils.convertPredicateToFilter(
+ greaterEqualExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueGtEq(0, 124),
actualFilter1);
+
+ // 2. LessEqual (<=): int64 <= 123.3 -> int64 <= 123 (floor)
+ // Result: ValueLtEq(123)
+ LessEqualExpression lessEqualExpression = new
LessEqualExpression(int64Column, doubleConstant);
+ Filter actualFilter2 =
+ PredicateUtils.convertPredicateToFilter(
+ lessEqualExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueLtEq(0, 123),
actualFilter2);
+
+ // 3. GreaterThan (>): int64 > 123.3 -> int64 > 123 (floor)
+ // Result: ValueGt(123)
+ GreaterThanExpression greaterThanExpression =
+ new GreaterThanExpression(int64Column, doubleConstant);
+ Filter actualFilter3 =
+ PredicateUtils.convertPredicateToFilter(
+ greaterThanExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueGt(0, 123),
actualFilter3);
+
+ // 4. LessThan (<): int64 < 123.3 -> int64 < 124 (ceil)
+ // Result: ValueLt(124)
+ LessThanExpression lessThanExpression = new
LessThanExpression(int64Column, doubleConstant);
+ Filter actualFilter4 =
+ PredicateUtils.convertPredicateToFilter(
+ lessThanExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueLt(0, 124),
actualFilter4);
+
+ // 5. EqualTo (=): int64 = 123.3 -> Impossible for int64
+ // Result: FalseLiteralFilter (Always False)
+ EqualToExpression equalToExpression = new EqualToExpression(int64Column,
doubleConstant);
+ Filter actualFilter5 =
+ PredicateUtils.convertPredicateToFilter(
+ equalToExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new FalseLiteralFilter(), actualFilter5);
+
+ // 6. NonEqual (!=): int64 != 123.3
+ // Result: ValueIsNotNullOperator (Always True, as long as value exists)
+ NonEqualExpression nonEqualExpression = new
NonEqualExpression(int64Column, doubleConstant);
+ Filter actualFilter6 =
+ PredicateUtils.convertPredicateToFilter(
+ nonEqualExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new ValueIsNotNullOperator(0), actualFilter6);
+ }
+
+ @Test
+ public void testNegativeDoubleLiteralAndInt64Column() {
+ Expression doubleConstant = new ConstantOperand(TSDataType.DOUBLE,
"-123.3");
+
+ // 1. GreaterEqual (>=): int64 >= -123.3 -> int64 >= -123 (ceil)
+ // Result: ValueGtEq(-123)
+ Expression greaterEqualExpression = new
GreaterEqualExpression(int64Column, doubleConstant);
+ Filter actualFilter1 =
+ PredicateUtils.convertPredicateToFilter(
+ greaterEqualExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueGtEq(0, -123),
actualFilter1);
+
+ // 2. LessEqual (<=): int64 <= -123.3 -> int64 <= -124 (floor)
+ // Result: ValueLtEq(-124)
+ LessEqualExpression lessEqualExpression = new
LessEqualExpression(int64Column, doubleConstant);
+ Filter actualFilter2 =
+ PredicateUtils.convertPredicateToFilter(
+ lessEqualExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueLtEq(0, -124),
actualFilter2);
+
+ // 3. GreaterThan (>): int64 > -123.3 -> int64 > -124 (floor)
+ // Result: ValueGt(-124)
+ GreaterThanExpression greaterThanExpression =
+ new GreaterThanExpression(int64Column, doubleConstant);
+ Filter actualFilter3 =
+ PredicateUtils.convertPredicateToFilter(
+ greaterThanExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueGt(0, -124),
actualFilter3);
+
+ // 4. LessThan (<): int64 < -123.3 -> int64 < -123 (ceil)
+ // Result: ValueLt(-123)
+ LessThanExpression lessThanExpression = new
LessThanExpression(int64Column, doubleConstant);
+ Filter actualFilter4 =
+ PredicateUtils.convertPredicateToFilter(
+ lessThanExpression,
+ Collections.singletonList("Int64Column"),
+ false,
+ getMockTypeProvider(TSDataType.INT64),
+ null);
+ Assert.assertEquals(new LongFilterOperators.ValueLt(0, -123),
actualFilter4);
+
+ // 5. EqualTo (=): int64 = -123.3 -> Impossible for int64
+ // Result:FalseLiteralFilter(Always False)
Review Comment:
There's a spacing issue in the comment. "Result:FalseLiteralFilter(Always
False)" should have a space after the colon for consistency with other comments
in the file (e.g., line 180).
```suggestion
// Result: FalseLiteralFilter(Always False)
```
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/predicate/ConvertPredicateToFilterVisitor.java:
##########
@@ -155,9 +163,117 @@ public static <T extends Comparable<T>> Filter
constructCompareFilter(
int measurementIndex =
context.getMeasurementIndex(symbolReference.getName());
Type type = context.getType(Symbol.from(symbolReference));
+ TSDataType columnDataType = InternalTypeManager.getTSDataType(type);
+
+ // when literal is the doubleLiteral type and the columnDataType is INT64
or INT32,
+ // the doubleLiteral has to be converted.
+ if (literal instanceof DoubleLiteral) {
+ DoubleLiteral doubleLiteral = (DoubleLiteral) literal;
+ double doubleLiteralValue = doubleLiteral.getValue();
+
+ if (columnDataType == INT64) {
+ if (doubleLiteralValue > Long.MAX_VALUE) {
+ return constructFilterForGreaterThanMax(operator, measurementIndex);
+ }
+ if (doubleLiteralValue < Long.MIN_VALUE) {
+ return constructFilterForLessThanMin(operator, measurementIndex);
+ }
+ return constructFilterFromDouble(operator, doubleLiteralValue,
measurementIndex, type);
+
+ } else if (columnDataType == INT32) {
+ if (doubleLiteralValue > Integer.MAX_VALUE) {
+ return constructFilterForGreaterThanMax(operator, measurementIndex);
+ }
+
+ if (doubleLiteralValue < Integer.MIN_VALUE) {
+ return constructFilterForLessThanMin(operator, measurementIndex);
+ }
+ return constructFilterFromDouble(operator, doubleLiteralValue,
measurementIndex, type);
+ }
+ }
Review Comment:
The code doesn't handle special double values like NaN and Infinity. When
doubleLiteralValue is NaN, comparisons with Long.MAX_VALUE/Long.MIN_VALUE at
lines 175, 178, 184, 188 will always return false, and NaN will be passed to
Math.ceil/floor which returns NaN, leading to incorrect filter construction.
Similarly, positive/negative infinity should be explicitly checked. Consider
adding checks at the beginning: if (Double.isNaN(doubleLiteralValue)) throw new
IllegalArgumentException() or return appropriate filter, and if
(Double.isInfinite(doubleLiteralValue)) handle overflow cases directly.
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/relational/analyzer/predicate/ConvertPredicateToFilterVisitor.java:
##########
@@ -155,9 +163,117 @@ public static <T extends Comparable<T>> Filter
constructCompareFilter(
int measurementIndex =
context.getMeasurementIndex(symbolReference.getName());
Type type = context.getType(Symbol.from(symbolReference));
+ TSDataType columnDataType = InternalTypeManager.getTSDataType(type);
+
+ // when literal is the doubleLiteral type and the columnDataType is INT64
or INT32,
+ // the doubleLiteral has to be converted.
+ if (literal instanceof DoubleLiteral) {
+ DoubleLiteral doubleLiteral = (DoubleLiteral) literal;
+ double doubleLiteralValue = doubleLiteral.getValue();
+
+ if (columnDataType == INT64) {
+ if (doubleLiteralValue > Long.MAX_VALUE) {
+ return constructFilterForGreaterThanMax(operator, measurementIndex);
+ }
+ if (doubleLiteralValue < Long.MIN_VALUE) {
+ return constructFilterForLessThanMin(operator, measurementIndex);
+ }
+ return constructFilterFromDouble(operator, doubleLiteralValue,
measurementIndex, type);
+
+ } else if (columnDataType == INT32) {
+ if (doubleLiteralValue > Integer.MAX_VALUE) {
+ return constructFilterForGreaterThanMax(operator, measurementIndex);
+ }
+
+ if (doubleLiteralValue < Integer.MIN_VALUE) {
+ return constructFilterForLessThanMin(operator, measurementIndex);
+ }
+ return constructFilterFromDouble(operator, doubleLiteralValue,
measurementIndex, type);
+ }
+ }
Review Comment:
The implementation uses DoubleMath.isMathematicalInteger() to check if a
double is an integer (line 221), while the tree model implementation
(ConvertPredicateToFilterVisitor in the expression/visitor/predicate package)
uses BigDecimal.remainder(BigDecimal.ONE).signum() == 0 for the same purpose
(line 342). This inconsistency makes the codebase harder to maintain. Consider
using the same approach in both places, preferably
DoubleMath.isMathematicalInteger() as it's more concise and handles edge cases
like NaN correctly.
--
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]
