xtern commented on code in PR #2671:
URL: https://github.com/apache/ignite-3/pull/2671#discussion_r1363556769
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/exp/ExpressionFactoryImpl.java:
##########
@@ -487,18 +534,13 @@ private Scalar compile(List<RexNode> nodes, RelDataType
type, boolean biInParams
RexProgramBuilder programBuilder = new RexProgramBuilder(type,
rexBuilder);
- BitSet unspecifiedValues = new BitSet(nodes.size());
-
for (int i = 0; i < nodes.size(); i++) {
RexNode node = nodes.get(i);
if (node != null) {
programBuilder.addProject(node, null);
} else {
- unspecifiedValues.set(i);
-
- programBuilder.addProject(rexBuilder.makeNullLiteral(type ==
emptyType
- ? nullType : type.getFieldList().get(i).getType()),
null);
+ assert false : "unexpected nullable node";
Review Comment:
This conditional branching is redundant now. I suggest to remove it
```
assert node != null : "unexpected nullable node";
programBuilder.addProject(node, null);
```
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/exp/ExpressionFactoryImplTest.java:
##########
@@ -176,6 +189,60 @@ public void testRangeConditionWithNullIsIgnored() {
assertEquals(List.of(new TestRange(new Object[]{1})), list);
}
+ @ParameterizedTest(name = "condition satisfies: [{0}] the index")
Review Comment:
`condition satisfies: [true] the index`
:thinking: Is this an intentional message format?
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/exp/ExpressionFactoryImplTest.java:
##########
@@ -176,6 +189,60 @@ public void testRangeConditionWithNullIsIgnored() {
assertEquals(List.of(new TestRange(new Object[]{1})), list);
}
+ @ParameterizedTest(name = "condition satisfies: [{0}] the index")
+ @ValueSource(booleans = {true, false})
Review Comment:
I am not fully understand the idea behind `conditionSatisfyIdx = false` test
case.
AFAIK the handling of this case was not changed by this patch. But maybe you
know better why this test case is necessary.
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/exec/exp/ExpressionFactoryImplTest.java:
##########
@@ -176,6 +189,60 @@ public void testRangeConditionWithNullIsIgnored() {
assertEquals(List.of(new TestRange(new Object[]{1})), list);
}
+ @ParameterizedTest(name = "condition satisfies: [{0}] the index")
+ @ValueSource(booleans = {true, false})
+ public void testConditionsNotContainsNulls(boolean conditionSatisfyIdx) {
+ RexBuilder rexBuilder = Commons.rexBuilder();
+
+ RexNode val1 = rexBuilder.makeExactLiteral(new BigDecimal("1"));
+ RexNode val2 = rexBuilder.makeExactLiteral(new BigDecimal("2"));
+
+ RelDataTypeSystem typeSystem =
Commons.cluster().getTypeFactory().getTypeSystem();
+
+ RexLocalRef ref1 = rexBuilder.makeLocalRef(new
BasicSqlType(typeSystem, SqlTypeName.INTEGER), conditionSatisfyIdx ? 1 : 3);
+ RexLocalRef ref2 = rexBuilder.makeLocalRef(new
BasicSqlType(typeSystem, SqlTypeName.INTEGER), 2);
+
+ RexNode pred1 = rexBuilder.makeCall(SqlStdOperatorTable.GREATER_THAN,
ref1, val1);
+ RexNode pred2 = rexBuilder.makeCall(SqlStdOperatorTable.GREATER_THAN,
ref2, val2);
+
+ RexNode andCondition = rexBuilder.makeCall(SqlStdOperatorTable.AND,
pred1, pred2);
+
+ RelDataType rowType = new Builder(typeFactory)
+ .add("C1", SqlTypeName.INTEGER)
+ .add("C2", SqlTypeName.INTEGER)
+ .add("C3", SqlTypeName.INTEGER)
+ .build();
+
+ // build bounds for two sequential columns also belongs to index
+ List<SearchBounds> bounds =
RexUtils.buildSortedSearchBounds(Commons.cluster(),
+ RelCollations.of(ImmutableIntList.of(1, 2)), andCondition,
rowType, ImmutableBitSet.of(0, 1, 2));
+
+ if (!conditionSatisfyIdx) {
+ assertNull(bounds);
+ return;
+ } else {
+ assertNotNull(bounds);
+ }
+
+ RangeIterable<Object[]> ranges = expFactory.ranges(bounds, rowType,
null);
+ // TODO: https://issues.apache.org/jira/browse/IGNITE-13568 seems
length predicate bounds
+ // for sequential columns also belong to index need to be 2
+ assertEquals(1, ranges.iterator().next().lower().length);
+
+ // condition expression is not used
+ RexLiteral condition = rexBuilder.makeLiteral(true);
+
+ // assembly bounds
+ List<SearchBounds> boundsList = List.of(
+ new RangeBounds(condition, val1, val2, true, true)
+ );
+
+ ranges = expFactory.ranges(boundsList, rowType, null);
+ assertEquals(1, ranges.iterator().next().lower().length);
+ assertEquals(1, ranges.iterator().next().upper().length);
+
Review Comment:
```suggestion
```
--
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]
