Copilot commented on code in PR #13225:
URL: https://github.com/apache/ignite/pull/13225#discussion_r3427818023
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +353,107 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ @Test
+ public void testResultsWithUnexpectedParam() {
+ sql(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\""
+ .formatted("UndefinedClassName")
+ );
+
+ checkResultsWithUnexpectedKey(new Object());
+ }
+
+ /** */
+ @Test
+ public void testResultsWithoutKeyTypeWithUnexpectedParam() {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ checkResultsWithUnexpectedKey(new Object());
+ }
+
+ /** */
+ @Test
+ public void testResultsWithoutKeyTypeWithUnexpectedBOAsParam() {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ Object arg = client.binary().builder("UserDefinedBinary").build();
+
+ checkResultsWithUnexpectedKey(arg);
+ }
+
+ /** Check results with search for _key different from defined in schema. */
+ private void checkResultsWithUnexpectedKey(Object arg) {
+ String queryTemplate = "select /*+ DISABLE_RULE('%s') */ _key from
PUBLIC.PERSON where _key %s ? ORDER BY _key";
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ List<List<?>> res = null;
+
+ for (boolean tableScan : List.of(true, false)) {
+ String qry = queryTemplate.formatted(tableScan ?
+ "LogicalIndexScanConverterRule" :
"LogicalTableScanConverterRule", cmpOp.comp);
+
+ if (res == null)
+ res = sql(qry, arg);
+ else {
+ int pos = 0;
+ List<List<?>> secondRes = sql(qry, arg);
+
+ if ((res.isEmpty() && !secondRes.isEmpty()) ||
(!res.isEmpty() && secondRes.isEmpty()))
+ fail("Different results size: " + res.size() + " " +
secondRes.size());
+
+ for (List<?> r : secondRes) {
+ assertEquals(r.get(0), res.get(pos).get(0));
+ pos++;
+ }
Review Comment:
The result-comparison logic only checks the "one empty, one non-empty" case.
If both are non-empty but sizes differ, the loop can throw
`IndexOutOfBoundsException` and the failure message won't clearly indicate the
mismatch. Add an explicit size equality assertion before comparing row-by-row.
##########
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.
if (keyType.keySize() < 0 || keyType.type() ==
IndexKeyType.JAVA_OBJECT)
Review Comment:
Typo in the comment: "splited" should be "split".
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +353,107 @@ private void
checkCompositePkWithDifferentCmpOperations(boolean useBinaryObject)
}
}
+ @Test
+ public void testResultsWithUnexpectedParam() {
+ sql(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\""
+ .formatted("UndefinedClassName")
+ );
+
+ checkResultsWithUnexpectedKey(new Object());
+ }
+
+ /** */
+ @Test
+ public void testResultsWithoutKeyTypeWithUnexpectedParam() {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ checkResultsWithUnexpectedKey(new Object());
+ }
+
+ /** */
+ @Test
+ public void testResultsWithoutKeyTypeWithUnexpectedBOAsParam() {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ Object arg = client.binary().builder("UserDefinedBinary").build();
+
+ checkResultsWithUnexpectedKey(arg);
+ }
+
+ /** Check results with search for _key different from defined in schema. */
+ private void checkResultsWithUnexpectedKey(Object arg) {
+ String queryTemplate = "select /*+ DISABLE_RULE('%s') */ _key from
PUBLIC.PERSON where _key %s ? ORDER BY _key";
+
+ fillTable();
+
+ for (CmpOp cmpOp : CmpOp.values()) {
+ List<List<?>> res = null;
+
+ for (boolean tableScan : List.of(true, false)) {
+ String qry = queryTemplate.formatted(tableScan ?
+ "LogicalIndexScanConverterRule" :
"LogicalTableScanConverterRule", cmpOp.comp);
+
+ if (res == null)
+ res = sql(qry, arg);
+ else {
+ int pos = 0;
+ List<List<?>> secondRes = sql(qry, arg);
+
+ if ((res.isEmpty() && !secondRes.isEmpty()) ||
(!res.isEmpty() && secondRes.isEmpty()))
+ fail("Different results size: " + res.size() + " " +
secondRes.size());
+
+ for (List<?> r : secondRes) {
+ assertEquals(r.get(0), res.get(pos).get(0));
+ pos++;
+ }
+ }
+
+ assertQuery(qry)
+ .withParams(new Object())
+ .matches(tableScan ?
Review Comment:
`checkResultsWithUnexpectedKey` executes the query using the provided `arg`,
but the `assertQuery` verification uses `new Object()` instead. This makes the
plan/assertion run against a different parameter value and can mask regressions
(and fails to validate index/table-scan parity for the tested argument).
--
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]
