Copilot commented on code in PR #16917:
URL: https://github.com/apache/iotdb/pull/16917#discussion_r2625962940
##########
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, the columnDataType INT64 or
INT32 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);
+ }
+ }
+
+ if (literal instanceof LongLiteral && columnDataType == INT32) {
+ return constructValueFilter(operator, literal, LongType.INT64,
measurementIndex);
+ }
+
+ return constructValueFilter(operator, literal, type, measurementIndex);
+ }
+
+ private static Filter constructFilterFromDouble(
+ ComparisonExpression.Operator operator, double doubleValue, int
measurementIndex, Type type) {
+
+ switch (operator) {
+ case GREATER_THAN_OR_EQUAL:
+ case LESS_THAN:
+ double ceil = Math.ceil(doubleValue);
+ // for targeted type is INT32 or INT64, transformed the double value
to LongLiteral
+ Literal ceilLiteral = new LongLiteral(String.valueOf((long) ceil));
+ return constructValueFilter(operator, ceilLiteral, type,
measurementIndex);
+
+ case LESS_THAN_OR_EQUAL:
+ case GREATER_THAN:
+ double floor = Math.floor(doubleValue);
+ Literal floorLiteral = new LongLiteral(String.valueOf((long) floor));
+ return constructValueFilter(operator, floorLiteral, type,
measurementIndex);
Review Comment:
Inconsistent rounding approach between table and tree model implementations.
The table model uses Math.ceil/Math.floor with direct double casting, while the
tree model uses BigDecimal.setScale with RoundingMode. This could potentially
lead to subtle differences in edge cases due to floating-point precision.
Consider using a consistent approach (preferably BigDecimal for precision)
across both implementations to ensure identical behavior.
##########
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, the columnDataType INT64 or
INT32 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);
+ }
+ }
+
+ if (literal instanceof LongLiteral && columnDataType == INT32) {
+ return constructValueFilter(operator, literal, LongType.INT64,
measurementIndex);
+ }
Review Comment:
Missing overflow check for LongLiteral values when comparing with INT32
columns. When a LongLiteral exceeds Integer.MAX_VALUE or is less than
Integer.MIN_VALUE, it should return FalseLiteralFilter for EQUAL or
ValueIsNotNullOperator/FalseLiteralFilter for comparison operators (similar to
the double overflow handling). Currently, it unconditionally uses INT64 type
promotion which may produce incorrect filter behavior. Add overflow validation
similar to lines 184-190 before type promotion.
##########
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, the columnDataType INT64 or
INT32 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);
Review Comment:
Missing test coverage for special double values (NaN, Infinity, -Infinity)
when comparing with INT32/INT64 columns. These edge cases should be handled
explicitly, as they have undefined behavior with the current overflow checks
and Math.ceil/floor operations. Add test cases to verify proper handling of
these special values.
##########
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, the columnDataType INT64 or
INT32 has to be
+ // converted.
Review Comment:
Comment is slightly misleading. It states that "columnDataType INT64 or
INT32 has to be converted" when actually it's the double literal that gets
converted (rounded) to match the integer column type. Consider rephrasing to:
"when literal is DoubleLiteral and column is INT64 or INT32, convert the double
literal to an appropriate integer value".
##########
iotdb-core/datanode/src/main/java/org/apache/iotdb/db/queryengine/plan/expression/visitor/predicate/ConvertPredicateToFilterVisitor.java:
##########
@@ -265,7 +272,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();
+ if (constantDataType == DOUBLE) {
+ String constantString = constantLiteral.getValueString();
+ 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);
+ }
+ }
+
+ if (constantDataType == INT64 && columnDataType == INT32) {
+ return constructValueFilter(expressionType, constantOperand, INT64,
measurementIndex);
+ }
Review Comment:
Missing overflow check for INT64 literal values when comparing with INT32
columns. When an INT64 literal exceeds Integer.MAX_VALUE or is less than
Integer.MIN_VALUE, it should handle overflow similar to the double overflow
handling at lines 294-299. Currently, it unconditionally uses INT64 type
promotion which may produce incorrect filter behavior for out-of-range values.
Add overflow validation before type promotion.
--
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]
