Copilot commented on code in PR #13225:
URL: https://github.com/apache/ignite/pull/13225#discussion_r3427462866
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +352,135 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithTableScan() {
+ checkEmptyResultsWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema. */
+ private void checkEmptyResultsWithUnexpectedKey(boolean tableScan) {
+ sql(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\""
+ .formatted("UndefinedClassName")
+ );
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " +
cmpOp.comp + " ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
+ }
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithTableScan()
{
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema and undefined table key type. */
+ private void checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(boolean
tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " + cmpOp.comp +
" ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
+ }
+ }
+
+ /** */
+ @Test
+ public void
testEmptyResultsWithoutKeyTypeWithUnexpectedBOKeyWithTableScan() {
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedBOKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedBOKeyWithIdxScan()
{
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedBOKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema and binary object as search param. */
Review Comment:
Spelling typo in comment: "serch" -> "search".
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionContext.java:
##########
@@ -32,11 +32,13 @@
import java.util.concurrent.atomic.AtomicBoolean;
import java.util.function.Consumer;
import java.util.function.Function;
+import java.util.function.Supplier;
Review Comment:
Unused import `java.util.function.Supplier` is not referenced anywhere in
this file; it should be removed to avoid style/lint failures and keep imports
clean.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionContext.java:
##########
@@ -302,11 +310,16 @@ public IgniteLogger logger() {
return baseDataContext.get(name);
}
- /** */
+ /** Returns a parameter with internal representation or {@link
BinaryObject}. */
public Object getParameter(String name, Type storageType) {
assert name.startsWith("?") : name;
- return TypeUtils.toInternal(this, params.get(name), storageType);
+ Object param = params.get(name);
+
+ if (storageType == java.lang.Object.class && !(param instanceof
BinaryObject))
+ return binaryMarshaller.apply(param);
+
Review Comment:
`getParameter` unconditionally calls `binaryMarshaller.apply(param)` when
`storageType` is `Object.class`, which can throw NPE if the parameter is null
(or if `params` / `binaryMarshaller` is null in test contexts). This breaks the
previous behavior where null parameters were handled by `TypeUtils.toInternal`
and can cause query failures for `WHERE _key = ?` with a null argument.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +352,135 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithTableScan() {
+ checkEmptyResultsWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema. */
+ private void checkEmptyResultsWithUnexpectedKey(boolean tableScan) {
+ sql(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\""
+ .formatted("UndefinedClassName")
+ );
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " +
cmpOp.comp + " ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
+ }
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithTableScan()
{
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema and undefined table key type. */
+ private void checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(boolean
tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " + cmpOp.comp +
" ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
+ }
+ }
+
+ /** */
+ @Test
+ public void
testEmptyResultsWithoutKeyTypeWithUnexpectedBOKeyWithTableScan() {
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedBOKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedBOKeyWithIdxScan()
{
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedBOKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema and binary object as search param. */
+ private void checkEmptyResultsWithoutKeyTypeWithUnexpectedBOKey(boolean
tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ Object arg = client.binary().builder("UserDefinedBinary").build();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " + cmpOp.comp +
" ?")
+ .withParams(arg)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
Review Comment:
These tests are named `checkEmptyResults...` but they don't assert that the
result set is empty. `QueryChecker.check()` does not verify row count unless
`resultSize(...)` (or expected rows) is specified, so the test may pass even if
incorrect rows are returned.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -304,32 +193,157 @@ private void checkCompositePk(
}
/** */
- private void checkCompositePkWithDifferentCmpOperations(boolean
useBinaryObject) {
+ @Test
+ public void testCompositePkSearchByPartOfKeyTableScan() {
+ compositePkEqualitySearchByPartOfKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkSearchByPartOfKeyIdxScan() {
+ compositePkEqualitySearchByPartOfKey(false);
+ }
+
+ /**
+ * Tests composite primary key search using only part of the key (single
column).
+ * Verifies that querying with just the {@code id} column uses index scan,
+ * while querying with just the {@code name} column falls back to table
scan
+ * since it's not the leading column of the composite primary key.
+ */
Review Comment:
This Javadoc says the `name`-only predicate "falls back to table scan", but
the test intentionally forces either table-scan or index-scan via
`DISABLE_RULE(...)` and asserts whichever plan is forced. The comment should be
updated to reflect the actual behavior being tested.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +352,135 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithTableScan() {
+ checkEmptyResultsWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema. */
Review Comment:
Spelling typo in comment: "serch" -> "search".
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/IndexScan.java:
##########
@@ -146,9 +146,9 @@ private int[] fieldToInlinedKeysMapping() {
for (InlineIndexKeyType keyType : inlinedKeys) {
// Variable length types can be not fully inlined, so it's
probably better to directly read full cache row
- // instead of trying to read inlined value and than falllback to
cache row reading.
+ // instead of trying to read inlined value and then fallback to
cache row reading.
// Inlined JAVA_OBJECT can't be compared with fill cache row in
case of hash collision, this can lead to
- // issues when processing the next index page in cursor if current
page was concurrently splitted.
+ // issues when processing the next index page in cursor if current
page was concurrently splited.
Review Comment:
Typos in comment: "fill cache row" should be "full cache row" and "splited"
should be "split".
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +352,135 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithTableScan() {
+ checkEmptyResultsWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema. */
+ private void checkEmptyResultsWithUnexpectedKey(boolean tableScan) {
+ sql(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\""
+ .formatted("UndefinedClassName")
+ );
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " +
cmpOp.comp + " ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
+ }
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithTableScan()
{
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema and undefined table key type. */
+ private void checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(boolean
tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " + cmpOp.comp +
" ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
Review Comment:
These tests are named `checkEmptyResults...` but they don't assert that the
result set is empty. `QueryChecker.check()` does not verify row count unless
`resultSize(...)` (or expected rows) is specified, so the test may pass even if
incorrect rows are returned.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +352,135 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithTableScan() {
+ checkEmptyResultsWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema. */
+ private void checkEmptyResultsWithUnexpectedKey(boolean tableScan) {
+ sql(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\""
+ .formatted("UndefinedClassName")
+ );
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " +
cmpOp.comp + " ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
Review Comment:
These tests are named `checkEmptyResults...` but they don't assert that the
result set is empty. `QueryChecker.check()` does not verify row count unless
`resultSize(...)` (or expected rows) is specified, so the test may pass even if
incorrect rows are returned.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +352,135 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithTableScan() {
+ checkEmptyResultsWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema. */
+ private void checkEmptyResultsWithUnexpectedKey(boolean tableScan) {
+ sql(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\""
+ .formatted("UndefinedClassName")
+ );
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, _key from PUBLIC.PERSON where _key " +
cmpOp.comp + " ?")
+ .withParams(new Object())
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
+ }
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithTableScan()
{
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(true);
+ }
+
+ /** */
+ @Test
+ public void testEmptyResultsWithoutKeyTypeWithUnexpectedKeyWithIdxScan() {
+ checkEmptyResultsWithoutKeyTypeWithUnexpectedKey(false);
+ }
+
+ /** No exceptions are raised with serch for _key different from defined in
schema and undefined table key type. */
Review Comment:
Spelling typo in comment: "serch" -> "search".
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/exec/LogicalRelImplementorTest.java:
##########
@@ -133,6 +133,7 @@ public class LogicalRelImplementorTest extends
GridCommonAbstractTest {
NoOpIoTracker.INSTANCE,
0,
null,
+ null,
null
Review Comment:
`ExecutionContext` now requires a binary marshaller callback, but this test
passes `null`, which can cause an NPE if parameter conversion is exercised
(e.g., by future changes). Other tests provide a stub that throws to make
unexpected usage explicit; this test should do the same.
--
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]
