Copilot commented on code in PR #18729:
URL: https://github.com/apache/pinot/pull/18729#discussion_r3390516373
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -350,20 +350,22 @@ private static RexExpression handleSearch(RexCall
rexCall) {
if (leftOperand instanceof RexLiteral) {
return evaluateLiteralIn((RexLiteral) leftOperand,
sarg.rangeSet.asRanges());
}
- return new RexExpression.FunctionCall(ColumnDataType.BOOLEAN,
SqlKind.IN.name(),
- toSearchFunctionOperands(leftOperand, sarg.rangeSet.asRanges(),
dataType));
+ RexExpression inExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.IN.name(),
+ toSearchFunctionOperands(leftOperand, sarg.rangeSet.asRanges(),
dataType));
+ return addNullCheckIfRequired(leftOperand, sarg.nullAs, inExpr);
} else if (sarg.isComplementedPoints()) {
if (leftOperand instanceof RexLiteral) {
return evaluateLiteralNotIn((RexLiteral) leftOperand,
sarg.rangeSet.complement().asRanges());
}
- return new RexExpression.FunctionCall(ColumnDataType.BOOLEAN,
SqlKind.NOT_IN.name(),
- toSearchFunctionOperands(leftOperand,
sarg.rangeSet.complement().asRanges(), dataType));
+ RexExpression notInExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.NOT_IN.name(),
+ toSearchFunctionOperands(leftOperand,
sarg.rangeSet.complement().asRanges(), dataType));
+ return addNullCheckIfRequired(leftOperand, sarg.nullAs, notInExpr);
} else {
if (leftOperand instanceof RexLiteral) {
return evaluateLiteralOrRanges((RexLiteral) leftOperand,
sarg.rangeSet.asRanges());
}
- Set<Range> ranges = sarg.rangeSet.asRanges();
- return convertRangesToOr(dataType, leftOperand, ranges);
+ RexExpression orExpr = convertRangesToOr(dataType, leftOperand, ranges);
Review Comment:
`ranges` is referenced but never defined in this branch, which will not
compile. It should use `sarg.rangeSet.asRanges()` (or declare a local
`Set<Range>` first) before calling `convertRangesToOr`.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -350,20 +350,22 @@ private static RexExpression handleSearch(RexCall
rexCall) {
if (leftOperand instanceof RexLiteral) {
return evaluateLiteralIn((RexLiteral) leftOperand,
sarg.rangeSet.asRanges());
}
- return new RexExpression.FunctionCall(ColumnDataType.BOOLEAN,
SqlKind.IN.name(),
- toSearchFunctionOperands(leftOperand, sarg.rangeSet.asRanges(),
dataType));
+ RexExpression inExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.IN.name(),
+ toSearchFunctionOperands(leftOperand, sarg.rangeSet.asRanges(),
dataType));
Review Comment:
The continuation indentation here is inconsistent with the rest of the file
and violates the repository Checkstyle indentation rules (basic offset 2,
wrapped indent +4). This is likely to fail CI formatting checks.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -350,20 +350,22 @@ private static RexExpression handleSearch(RexCall
rexCall) {
if (leftOperand instanceof RexLiteral) {
return evaluateLiteralIn((RexLiteral) leftOperand,
sarg.rangeSet.asRanges());
}
- return new RexExpression.FunctionCall(ColumnDataType.BOOLEAN,
SqlKind.IN.name(),
- toSearchFunctionOperands(leftOperand, sarg.rangeSet.asRanges(),
dataType));
+ RexExpression inExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.IN.name(),
+ toSearchFunctionOperands(leftOperand, sarg.rangeSet.asRanges(),
dataType));
+ return addNullCheckIfRequired(leftOperand, sarg.nullAs, inExpr);
} else if (sarg.isComplementedPoints()) {
if (leftOperand instanceof RexLiteral) {
return evaluateLiteralNotIn((RexLiteral) leftOperand,
sarg.rangeSet.complement().asRanges());
}
- return new RexExpression.FunctionCall(ColumnDataType.BOOLEAN,
SqlKind.NOT_IN.name(),
- toSearchFunctionOperands(leftOperand,
sarg.rangeSet.complement().asRanges(), dataType));
+ RexExpression notInExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.NOT_IN.name(),
+ toSearchFunctionOperands(leftOperand,
sarg.rangeSet.complement().asRanges(), dataType));
Review Comment:
The continuation indentation here is inconsistent with the rest of the file
and violates the repository Checkstyle indentation rules (basic offset 2,
wrapped indent +4). This is likely to fail CI formatting checks.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -397,6 +399,23 @@ private static RexExpression
evaluateLiteralOrRanges(RexLiteral leftOperand, Set
return RexExpression.Literal.FALSE;
}
+ private static RexExpression addNullCheckIfRequired(RexNode leftOperand,
RexUnknownAs nullAs, RexExpression expr) {
+ switch (nullAs) {
+ case TRUE:
+ RexExpression isNullExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.IS_NULL.name(),
+ List.of(fromRexNode(leftOperand)));
Review Comment:
The wrapped line indentation here (and in the `FALSE` branch below) doesn't
follow the repository's Checkstyle indentation rules (basic offset 2, wrapped
indent +4), which is likely to fail CI style checks.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -397,6 +399,23 @@ private static RexExpression
evaluateLiteralOrRanges(RexLiteral leftOperand, Set
return RexExpression.Literal.FALSE;
}
+ private static RexExpression addNullCheckIfRequired(RexNode leftOperand,
RexUnknownAs nullAs, RexExpression expr) {
Review Comment:
This method uses `RexUnknownAs` but the type isn't imported in this file,
causing a compilation error. Add the Calcite import for `RexUnknownAs`
(preferred) or fully-qualify the type in the signature.
##########
pinot-query-planner/src/test/java/org/apache/pinot/query/planner/logical/RexExpressionUtilsTest.java:
##########
@@ -0,0 +1,384 @@
+/**
+ * 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.pinot.query.planner.logical;
+
+import com.google.common.collect.ImmutableRangeSet;
+import com.google.common.collect.Range;
+import java.math.BigDecimal;
+import org.apache.calcite.rel.type.RelDataTypeFactory;
+import org.apache.calcite.rex.RexBuilder;
+import org.apache.calcite.rex.RexCall;
+import org.apache.calcite.rex.RexInputRef;
+import org.apache.calcite.rex.RexLiteral;
+import org.apache.calcite.rex.RexUnknownAs;
+import org.apache.calcite.sql.SqlKind;
+import org.apache.calcite.sql.fun.SqlStdOperatorTable;
+import org.apache.calcite.sql.type.SqlTypeName;
+import org.apache.calcite.util.Sarg;
+import org.apache.pinot.query.type.TypeFactory;
+import org.testng.Assert;
+import org.testng.annotations.BeforeClass;
+import org.testng.annotations.Test;
+
+
+/**
+ * Tests for RexExpressionUtils, focusing on the handleSearch method and null
handling.
+ */
+public class RexExpressionUtilsTest {
+ private RexBuilder _rexBuilder;
+ private RelDataTypeFactory _typeFactory;
+
+ @BeforeClass
Review Comment:
This new test file uses 4-space indentation throughout, but the repo
enforces 2-space indentation via Checkstyle (and runs it on test sources). As
written, this is expected to fail CI style checks; please reformat the file to
match the project's indentation rules.
##########
pinot-query-planner/src/main/java/org/apache/pinot/query/planner/logical/RexExpressionUtils.java:
##########
@@ -397,6 +399,23 @@ private static RexExpression
evaluateLiteralOrRanges(RexLiteral leftOperand, Set
return RexExpression.Literal.FALSE;
}
+ private static RexExpression addNullCheckIfRequired(RexNode leftOperand,
RexUnknownAs nullAs, RexExpression expr) {
+ switch (nullAs) {
+ case TRUE:
+ RexExpression isNullExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.IS_NULL.name(),
+ List.of(fromRexNode(leftOperand)));
+ return new RexExpression.FunctionCall(ColumnDataType.BOOLEAN,
SqlKind.OR.name(), List.of(expr, isNullExpr));
+
+ case FALSE:
+ RexExpression isNotNullExpr = new
RexExpression.FunctionCall(ColumnDataType.BOOLEAN, SqlKind.IS_NOT_NULL.name(),
+ List.of(fromRexNode(leftOperand)));
Review Comment:
The wrapped line indentation here doesn't follow the repository's Checkstyle
indentation rules (basic offset 2, wrapped indent +4), which is likely to fail
CI style checks.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
