tkalkirill commented on code in PR #13225:
URL: https://github.com/apache/ignite/pull/13225#discussion_r3473823656
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/UserDefinedFunctionsIntegrationTest.java:
##########
@@ -332,15 +332,18 @@ public void testTableFunctions() throws Exception {
assertThrows("SELECT * from raiseException(?, ?, ?)",
RuntimeException.class, "Test exception",
1, "test", true);
+ var empObj1 = new Employer("emp1", 1000d);
+ var empObj10 = new Employer("emp10", 10000d);
+
// Object type.
- assertQuery("SELECT * from withObjectType(1)")
- .returns(1, new Employer("emp1", 1000d))
- .returns(10, new Employer("emp10", 10000d))
+ assertQuery("SELECT * from withObjectType(1) ORDER BY ID")
+ .returns(1, client.binary().toBinary(empObj1))
Review Comment:
Why is a binary object expected here if the function returns a POJO?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteSqlFunctions.java:
##########
@@ -119,15 +119,15 @@ public static BigDecimal toBigDecimal(boolean val, int
precision, int scale) {
}
/** CAST(VARCHAR AS DECIMAL). */
- public static BigDecimal toBigDecimal(String s, int precision, int scale) {
+ public static @Nullable BigDecimal toBigDecimal(String s, int precision,
int scale) {
Review Comment:
Forgot `@Nullable String s`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteSqlFunctions.java:
##########
@@ -119,15 +119,15 @@ public static BigDecimal toBigDecimal(boolean val, int
precision, int scale) {
}
/** CAST(VARCHAR AS DECIMAL). */
- public static BigDecimal toBigDecimal(String s, int precision, int scale) {
+ public static @Nullable BigDecimal toBigDecimal(String s, int precision,
int scale) {
if (s == null)
return null;
return toBigDecimal(new BigDecimal(s.trim()), precision, scale);
}
/** Converts {@code val} to a {@link BigDecimal} with the given {@code
precision} and {@code scale}. */
- public static BigDecimal toBigDecimal(Number val, int precision, int
scale) {
+ public static @Nullable BigDecimal toBigDecimal(Number val, int precision,
int scale) {
Review Comment:
Forgot `@Nullable Number val`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/IndexScan.java:
##########
@@ -204,7 +204,7 @@ protected TreeIndex<IndexRow> treeIndex() {
}
/** From Row to IndexRow convertor. */
- protected IndexRow row2indexRow(Row bound) {
+ protected @Nullable IndexRow row2indexRow(Row bound) {
Review Comment:
Forgot `@Nullable Row bound`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/exp/IgniteSqlFunctions.java:
##########
@@ -146,7 +146,7 @@ public static BigDecimal toBigDecimal(Number val, int
precision, int scale) {
}
/** Cast object depending on type to DECIMAL. */
- public static BigDecimal toBigDecimal(Object o, int precision, int scale) {
+ public static @Nullable BigDecimal toBigDecimal(Object o, int precision,
int scale) {
Review Comment:
Forgot `@Nullable Object o`.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +353,104 @@ 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 qryTemplate = "select /*+ DISABLE_RULE('%s') */ _key from
PUBLIC.PERSON where _key %s ? ORDER BY _key";
Review Comment:
Why disable the rule?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -338,6 +353,104 @@ 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 qryTemplate = "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 = qryTemplate.formatted(tableScan ?
+ "LogicalIndexScanConverterRule" :
"LogicalTableScanConverterRule", cmpOp.comp);
+
+ if (res == null)
+ res = sql(qry, arg);
+ else {
+ List<List<?>> secondRes = sql(qry, arg);
+
+ assertEquals("Different results size", res.size(),
secondRes.size());
+
+ for (int pos = 0; pos < secondRes.size(); pos++)
+ assertEquals(res.get(pos).get(0),
secondRes.get(pos).get(0));
+ }
+
+ assertQuery(qry)
+ .withParams(arg)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .check();
Review Comment:
And what will the return value be?
If I’ve understood the test correctly, we’re attempting a search using a key
type (or class) that differs from the one defined in the schema. I would expect
an error—but here, everything will be fine?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -304,32 +193,158 @@ private void checkCompositePk(
}
/** */
- private void checkCompositePkWithDifferentCmpOperations(boolean
useBinaryObject) {
+ @Test
+ public void testCompositePkSearchByPartOfKeyTableScan() {
+ compositePkEqualitySearchByPartOfKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkSearchByPartOfKeyIdxScan() {
+ compositePkEqualitySearchByPartOfKey(false);
+ }
+
+ /**
+ * Tests composite primary key equality search using only part of the key
(single column).
+ * Verifies that queries with equality comparison on either the {@code id}
or {@code name} column
+ * return correct results using either table scan or index scan depending
on the {@code tableScan} flag.
+ *
+ * @param tableScan {@code true} to test with table scan, {@code false} to
test with index scan.
+ */
+ public void compositePkEqualitySearchByPartOfKey(boolean tableScan) {
sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
- for (int i = 0; i < 10; i++) {
- sql(
- "insert into PUBLIC.PERSON(id, name, surname, age) values (?,
?, ?, ?)",
- i, "foo" + i, "bar" + i, 18 + i
- );
- }
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select _key, id, name from PUBLIC.PERSON
order by id");
+ BinaryObject _key = (BinaryObject)sqlRs.get(6).get(0);
+ int id = (Integer)sqlRs.get(6).get(1);
+ String name = (String)sqlRs.get(6).get(2);
+
+ // Select by uniq id.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where id = ?")
+ .withParams(id)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+
+ // Select by uniq name.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where name = ?")
+ .withParams(name)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyTableScan() {
+ compositePkWithOrderByKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyIdxScan() {
+ compositePkWithOrderByKey(false);
+ }
+
+ /**
+ * Tests composite primary key ordering by {@code _key}.
+ * Verifies that ordering by the composite primary key produces correct
results
+ * when comparing binary objects using {@link #binaryObjectCmpForDml}.
+ */
+ public void compositePkWithOrderByKey(boolean tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select id, name, age, _key from
PUBLIC.PERSON");
+ sqlRs.sort((o1, o2) -> binaryObjectCmpForDml(o1.get(3), o2.get(3)));
+
+ QueryChecker qryChecker = assertQuery("select /*+ DISABLE_RULE('" +
+ (tableScan ? "LogicalIndexScanConverterRule" :
"LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON order by _key")
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME);
+
+ sqlRs.forEach(objects ->
qryChecker.returns(objects.toArray(Object[]::new)));
+
+ qryChecker.check();
+ }
+
+ /** */
+ @Test
+ public void testBinaryCompositePkComparisonsWithTableScan() {
+ checkCompositePkWithDifferentCmpOperations(true, true);
+ }
+
+ /** */
+ @Test
+ public void testBinaryCompositePkComparisonsWithIdxScan() {
+ checkCompositePkWithDifferentCmpOperations(true, false);
+ }
+
+ /** */
+ @Test
+ public void testJavaObjCompositePkComparisonsWithTableScan() {
+ checkCompositePkWithDifferentCmpOperations(false, true);
+ }
+
+ /** */
+ @Test
+ public void testJavaObjCompositePkComparisonsWithIdxScan() {
+ checkCompositePkWithDifferentCmpOperations(false, false);
+ }
+
+ /**
+ * Checks composite primary key comparisons with different comparison
operations.
+ * Creates a table with composite key (id, name) using {@link
PersonCompositeKey}, inserts test data,
+ * and verifies that all comparison operations (EQ, NE, LT, LE, GT, GE)
work correctly with both table scan
+ * and index scan query strategies.
+ *
+ * @param useBinaryObject {@code true} to use binary object representation
for key comparison,
+ * {@code false} to use deserialized object.
+ * @param tableScan {@code true} to test with table scan, {@code false} to
test with index scan.
+ */
+ private void checkCompositePkWithDifferentCmpOperations(boolean
useBinaryObject, boolean tableScan) {
+ sql(String.format(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\"",
+ PersonCompositeKey.class.getName()
+ ));
+
+ fillTable();
List<List<?>> sqlRs = sql("select id, name, age, _key from
PUBLIC.PERSON order by id");
- BinaryObjectImpl _key8 = (BinaryObjectImpl)sqlRs.get(8).get(3);
- for (CmpOp cmpOp : CmpOp.values()) {
- if (cmpOp == CmpOp.EQ)
- continue;
+ Object key8 = sqlRs.get(8).get(3);
+ for (CmpOp cmpOp : CmpOp.values()) {
List<List<?>> expRows = sqlRs.stream()
- .filter(objects ->
cmpOp.expRowByKeyPred.test((BinaryObjectImpl)objects.get(3), _key8))
- .collect(toList());
+ .filter(objects ->
cmpOp.expRowByKeyPred.test((BinaryObjectImpl)objects.get(3), key8))
+ .toList();
QueryChecker qryChecker = assertQuery(String.format(
- "select id, name, age, _key from PUBLIC.PERSON where _key %s
?", cmpOp.sql
+ "select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
Review Comment:
Why disable the rule?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -304,32 +193,158 @@ private void checkCompositePk(
}
/** */
- private void checkCompositePkWithDifferentCmpOperations(boolean
useBinaryObject) {
+ @Test
+ public void testCompositePkSearchByPartOfKeyTableScan() {
+ compositePkEqualitySearchByPartOfKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkSearchByPartOfKeyIdxScan() {
+ compositePkEqualitySearchByPartOfKey(false);
+ }
+
+ /**
+ * Tests composite primary key equality search using only part of the key
(single column).
+ * Verifies that queries with equality comparison on either the {@code id}
or {@code name} column
+ * return correct results using either table scan or index scan depending
on the {@code tableScan} flag.
+ *
+ * @param tableScan {@code true} to test with table scan, {@code false} to
test with index scan.
+ */
+ public void compositePkEqualitySearchByPartOfKey(boolean tableScan) {
sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
- for (int i = 0; i < 10; i++) {
- sql(
- "insert into PUBLIC.PERSON(id, name, surname, age) values (?,
?, ?, ?)",
- i, "foo" + i, "bar" + i, 18 + i
- );
- }
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select _key, id, name from PUBLIC.PERSON
order by id");
+ BinaryObject _key = (BinaryObject)sqlRs.get(6).get(0);
+ int id = (Integer)sqlRs.get(6).get(1);
+ String name = (String)sqlRs.get(6).get(2);
+
+ // Select by uniq id.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where id = ?")
+ .withParams(id)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+
+ // Select by uniq name.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where name = ?")
+ .withParams(name)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyTableScan() {
+ compositePkWithOrderByKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyIdxScan() {
+ compositePkWithOrderByKey(false);
+ }
+
+ /**
+ * Tests composite primary key ordering by {@code _key}.
+ * Verifies that ordering by the composite primary key produces correct
results
+ * when comparing binary objects using {@link #binaryObjectCmpForDml}.
+ */
+ public void compositePkWithOrderByKey(boolean tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select id, name, age, _key from
PUBLIC.PERSON");
+ sqlRs.sort((o1, o2) -> binaryObjectCmpForDml(o1.get(3), o2.get(3)));
+
+ QueryChecker qryChecker = assertQuery("select /*+ DISABLE_RULE('" +
+ (tableScan ? "LogicalIndexScanConverterRule" :
"LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON order by _key")
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME);
+
+ sqlRs.forEach(objects ->
qryChecker.returns(objects.toArray(Object[]::new)));
+
+ qryChecker.check();
+ }
+
+ /** */
+ @Test
+ public void testBinaryCompositePkComparisonsWithTableScan() {
+ checkCompositePkWithDifferentCmpOperations(true, true);
+ }
+
+ /** */
+ @Test
+ public void testBinaryCompositePkComparisonsWithIdxScan() {
+ checkCompositePkWithDifferentCmpOperations(true, false);
+ }
+
+ /** */
+ @Test
+ public void testJavaObjCompositePkComparisonsWithTableScan() {
+ checkCompositePkWithDifferentCmpOperations(false, true);
+ }
+
+ /** */
+ @Test
+ public void testJavaObjCompositePkComparisonsWithIdxScan() {
+ checkCompositePkWithDifferentCmpOperations(false, false);
+ }
+
+ /**
+ * Checks composite primary key comparisons with different comparison
operations.
+ * Creates a table with composite key (id, name) using {@link
PersonCompositeKey}, inserts test data,
+ * and verifies that all comparison operations (EQ, NE, LT, LE, GT, GE)
work correctly with both table scan
+ * and index scan query strategies.
+ *
+ * @param useBinaryObject {@code true} to use binary object representation
for key comparison,
+ * {@code false} to use deserialized object.
+ * @param tableScan {@code true} to test with table scan, {@code false} to
test with index scan.
+ */
+ private void checkCompositePkWithDifferentCmpOperations(boolean
useBinaryObject, boolean tableScan) {
+ sql(String.format(
+ "create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name)) with \"key_type=%s\"",
+ PersonCompositeKey.class.getName()
+ ));
+
+ fillTable();
List<List<?>> sqlRs = sql("select id, name, age, _key from
PUBLIC.PERSON order by id");
- BinaryObjectImpl _key8 = (BinaryObjectImpl)sqlRs.get(8).get(3);
- for (CmpOp cmpOp : CmpOp.values()) {
- if (cmpOp == CmpOp.EQ)
- continue;
+ Object key8 = sqlRs.get(8).get(3);
+ for (CmpOp cmpOp : CmpOp.values()) {
List<List<?>> expRows = sqlRs.stream()
- .filter(objects ->
cmpOp.expRowByKeyPred.test((BinaryObjectImpl)objects.get(3), _key8))
- .collect(toList());
+ .filter(objects ->
cmpOp.expRowByKeyPred.test((BinaryObjectImpl)objects.get(3), key8))
Review Comment:
Why are BO and not POJO unquestionably expected here, as opposed to in other
scenarios?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -355,12 +355,12 @@ public static boolean hasScale(RelDataType type) {
}
/** */
- public static Object toInternal(DataContext ctx, Object val) {
+ public static @Nullable Object toInternal(DataContext ctx, Object val) {
Review Comment:
Forgot `@Nullable Object val`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/util/TypeUtils.java:
##########
@@ -355,12 +355,12 @@ public static boolean hasScale(RelDataType type) {
}
/** */
- public static Object toInternal(DataContext ctx, Object val) {
+ public static @Nullable Object toInternal(DataContext ctx, Object val) {
return val == null ? null : toInternal(ctx, val, val.getClass());
}
/** */
- public static Object toInternal(DataContext ctx, Object val, Type
storageType) {
+ public static @Nullable Object toInternal(DataContext ctx, Object val,
Type storageType) {
Review Comment:
Forgot `@Nullable Object val`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionContext.java:
##########
@@ -302,11 +314,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 (param != null && storageType == java.lang.Object.class && !(param
instanceof BinaryObject))
+ return binaryMarshaller.apply(param);
Review Comment:
Do I understand correctly that parameters will now be unconditionally
converted to a BO?
For instance, if a UDF or UDTF expects a specific parameter, will it receive
a BO instead?
If that is the case, it is not very convenient, and users might come to rely
on passing and receiving POJO in functions exactly as they are.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java:
##########
@@ -472,6 +475,10 @@ public void injectService(InjectResourcesService
injectSvc) {
udfQryLimit.set(ctx.config().getQueryThreadPoolSize() - 1);
+ binaryMarshaller = obj -> {
+ return ctx.cacheObjects().binary().toBinary(obj);
+ };
Review Comment:
```suggestion
binaryMarshaller = obj -> ctx.cacheObjects().binary().toBinary(obj);
```
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionServiceImpl.java:
##########
@@ -210,6 +210,9 @@ public class ExecutionServiceImpl<Row> extends
AbstractService implements Execut
/** */
private final Map<String, FragmentPlan> fragmentPlanCache = new
GridBoundedConcurrentLinkedHashMap<>(1024);
+ /** Binary marshaller. */
Review Comment:
Same about documentation.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -36,34 +37,57 @@ public class TableFunctionScan<Row> implements
Iterable<Row> {
/** */
private final RowFactory<Row> rowFactory;
+ /** */
+ Function<Object, Object> binaryMarshaller;
+
+ /** */
+ private static final String ERR_SIZE_TEMPLATE = "Unable to process table
function data: row length [%d]" +
+ " doesn't match defined columns number [%d].";
+
/** */
public TableFunctionScan(
RelDataType rowType,
Supplier<Iterable<?>> dataSupplier,
- RowFactory<Row> rowFactory
+ RowFactory<Row> rowFactory,
+ Function<Object, Object> marshaller
) {
this.rowType = rowType;
this.dataSupplier = dataSupplier;
this.rowFactory = rowFactory;
+ binaryMarshaller = marshaller;
}
/** {@inheritDoc} */
@Override public Iterator<Row> iterator() {
return F.iterator(dataSupplier.get(), this::convertToRow, true);
}
+ /** */
+ private static void rowSizeChecker(int rowSize, int fldCount) {
+ if (rowSize != fldCount)
+ throw new IgniteSQLException(ERR_SIZE_TEMPLATE.formatted(rowSize,
fldCount));
+ }
+
/** */
private Row convertToRow(Object rowContainer) {
if (rowContainer.getClass() != Object[].class &&
!Collection.class.isAssignableFrom(rowContainer.getClass()))
throw new IgniteSQLException("Unable to process table function
data: row type is neither Collection or Object[].");
- Object[] rowArr = rowContainer.getClass() == Object[].class
- ? (Object[])rowContainer
- : ((Collection<?>)rowContainer).toArray();
+ if (rowContainer instanceof Object[])
+ rowSizeChecker(((Object[])rowContainer).length,
rowType.getFieldCount());
+ else
+ rowSizeChecker(((Collection<?>)rowContainer).size(),
rowType.getFieldCount());
- if (rowArr.length != rowType.getFieldCount()) {
- throw new IgniteSQLException("Unable to process table function
data: row length [" + rowArr.length
- + "] doesn't match defined columns number [" +
rowType.getFieldCount() + "].");
+ Object[] rowArr;
Review Comment:
Do I understand correctly that there will be an unconditional conversion
into a BO of the result string returned by the query (conceptually, the
ResultSet)? If so, why?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/TableFunctionScan.java:
##########
@@ -36,34 +37,57 @@ public class TableFunctionScan<Row> implements
Iterable<Row> {
/** */
private final RowFactory<Row> rowFactory;
+ /** */
+ Function<Object, Object> binaryMarshaller;
+
+ /** */
+ private static final String ERR_SIZE_TEMPLATE = "Unable to process table
function data: row length [%d]" +
+ " doesn't match defined columns number [%d].";
+
/** */
public TableFunctionScan(
RelDataType rowType,
Supplier<Iterable<?>> dataSupplier,
- RowFactory<Row> rowFactory
+ RowFactory<Row> rowFactory,
+ Function<Object, Object> marshaller
) {
this.rowType = rowType;
this.dataSupplier = dataSupplier;
this.rowFactory = rowFactory;
+ binaryMarshaller = marshaller;
}
/** {@inheritDoc} */
@Override public Iterator<Row> iterator() {
return F.iterator(dataSupplier.get(), this::convertToRow, true);
}
+ /** */
+ private static void rowSizeChecker(int rowSize, int fldCount) {
+ if (rowSize != fldCount)
+ throw new IgniteSQLException(ERR_SIZE_TEMPLATE.formatted(rowSize,
fldCount));
+ }
+
/** */
private Row convertToRow(Object rowContainer) {
if (rowContainer.getClass() != Object[].class &&
!Collection.class.isAssignableFrom(rowContainer.getClass()))
throw new IgniteSQLException("Unable to process table function
data: row type is neither Collection or Object[].");
- Object[] rowArr = rowContainer.getClass() == Object[].class
- ? (Object[])rowContainer
- : ((Collection<?>)rowContainer).toArray();
+ if (rowContainer instanceof Object[])
+ rowSizeChecker(((Object[])rowContainer).length,
rowType.getFieldCount());
+ else
+ rowSizeChecker(((Collection<?>)rowContainer).size(),
rowType.getFieldCount());
- if (rowArr.length != rowType.getFieldCount()) {
- throw new IgniteSQLException("Unable to process table function
data: row length [" + rowArr.length
- + "] doesn't match defined columns number [" +
rowType.getFieldCount() + "].");
+ Object[] rowArr;
+
+ if (rowContainer.getClass().isArray()) {
+ rowArr = (Object[])rowContainer;
+ for (int pos = 0; pos < rowArr.length; ++pos)
+ rowArr[pos] = binaryMarshaller.apply(rowArr[pos]);
+ }
+ else {
+ Collection<?> coll = (Collection<?>)rowContainer;
+ rowArr = coll.stream().map(e ->
binaryMarshaller.apply(e)).toArray();
Review Comment:
Won't this in "hot code"? Perhaps using a loop would be better?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/IndexWrappedKeyScan.java:
##########
@@ -54,7 +54,7 @@ public IndexWrappedKeyScan(
}
/** */
- @Override protected IndexRow row2indexRow(Row bound) {
+ @Override @Nullable protected IndexRow row2indexRow(Row bound) {
Review Comment:
Forgot `@Nullable Row bound`.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/IndexWrappedKeyScan.java:
##########
@@ -63,22 +63,29 @@ public IndexWrappedKeyScan(
Object key = rowHnd.get(QueryUtils.KEY_COL, bound);
assert key != null : String.format("idxName=%s, bound=%s", idx.name(),
Commons.toString(rowHnd, bound));
- if (key instanceof BinaryObject)
- return binaryObject2indexRow((BinaryObject)key);
+ String idxTypeName =
idx.indexDefinition().typeDescriptor().keyTypeName();
- throw new IgniteException(String.format(
- "Unsupported type for index boundary: [expected=%s, current=%s]",
- BinaryObject.class.getName(), key.getClass().getName()
- ));
+ if (key instanceof BinaryObject bo) {
+ try {
+ String searchTypeName = bo.type().typeName();
+
+ if (!idxTypeName.equals(searchTypeName))
+ // The same behavior as for table scan.
+ return null;
+ }
+ catch (BinaryObjectException ex) {
+ // The same behavior as for table scan.
Review Comment:
Where exactly is the behavior the same?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/exec/ExecutionContext.java:
##########
@@ -148,10 +149,14 @@ public class ExecutionContext<Row> extends
AbstractQueryContext implements DataC
/** Entries holder per execution thread. */
private static final ThreadLocal<Collection<QueryTxEntry>> txEntriesHolder
= new ThreadLocal<>();
+ /** Binary marshaller. */
Review Comment:
There isn't enough documentation—can it convert to and from BO, or only one
way?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -304,32 +193,158 @@ private void checkCompositePk(
}
/** */
- private void checkCompositePkWithDifferentCmpOperations(boolean
useBinaryObject) {
+ @Test
+ public void testCompositePkSearchByPartOfKeyTableScan() {
+ compositePkEqualitySearchByPartOfKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkSearchByPartOfKeyIdxScan() {
+ compositePkEqualitySearchByPartOfKey(false);
+ }
+
+ /**
+ * Tests composite primary key equality search using only part of the key
(single column).
+ * Verifies that queries with equality comparison on either the {@code id}
or {@code name} column
+ * return correct results using either table scan or index scan depending
on the {@code tableScan} flag.
+ *
+ * @param tableScan {@code true} to test with table scan, {@code false} to
test with index scan.
+ */
+ public void compositePkEqualitySearchByPartOfKey(boolean tableScan) {
sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
- for (int i = 0; i < 10; i++) {
- sql(
- "insert into PUBLIC.PERSON(id, name, surname, age) values (?,
?, ?, ?)",
- i, "foo" + i, "bar" + i, 18 + i
- );
- }
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select _key, id, name from PUBLIC.PERSON
order by id");
+ BinaryObject _key = (BinaryObject)sqlRs.get(6).get(0);
+ int id = (Integer)sqlRs.get(6).get(1);
+ String name = (String)sqlRs.get(6).get(2);
+
+ // Select by uniq id.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where id = ?")
+ .withParams(id)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+
+ // Select by uniq name.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where name = ?")
+ .withParams(name)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyTableScan() {
+ compositePkWithOrderByKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyIdxScan() {
+ compositePkWithOrderByKey(false);
+ }
+
+ /**
+ * Tests composite primary key ordering by {@code _key}.
+ * Verifies that ordering by the composite primary key produces correct
results
+ * when comparing binary objects using {@link #binaryObjectCmpForDml}.
+ */
+ public void compositePkWithOrderByKey(boolean tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select id, name, age, _key from
PUBLIC.PERSON");
+ sqlRs.sort((o1, o2) -> binaryObjectCmpForDml(o1.get(3), o2.get(3)));
+
+ QueryChecker qryChecker = assertQuery("select /*+ DISABLE_RULE('" +
Review Comment:
Why disable the rule?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/integration/SelectByKeyFieldTest.java:
##########
@@ -304,32 +193,158 @@ private void checkCompositePk(
}
/** */
- private void checkCompositePkWithDifferentCmpOperations(boolean
useBinaryObject) {
+ @Test
+ public void testCompositePkSearchByPartOfKeyTableScan() {
+ compositePkEqualitySearchByPartOfKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkSearchByPartOfKeyIdxScan() {
+ compositePkEqualitySearchByPartOfKey(false);
+ }
+
+ /**
+ * Tests composite primary key equality search using only part of the key
(single column).
+ * Verifies that queries with equality comparison on either the {@code id}
or {@code name} column
+ * return correct results using either table scan or index scan depending
on the {@code tableScan} flag.
+ *
+ * @param tableScan {@code true} to test with table scan, {@code false} to
test with index scan.
+ */
+ public void compositePkEqualitySearchByPartOfKey(boolean tableScan) {
sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
- for (int i = 0; i < 10; i++) {
- sql(
- "insert into PUBLIC.PERSON(id, name, surname, age) values (?,
?, ?, ?)",
- i, "foo" + i, "bar" + i, 18 + i
- );
- }
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select _key, id, name from PUBLIC.PERSON
order by id");
+ BinaryObject _key = (BinaryObject)sqlRs.get(6).get(0);
+ int id = (Integer)sqlRs.get(6).get(1);
+ String name = (String)sqlRs.get(6).get(2);
+
+ // Select by uniq id.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where id = ?")
+ .withParams(id)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+
+ // Select by uniq name.
+ assertQuery("select /*+ DISABLE_RULE('" + (tableScan ?
"LogicalIndexScanConverterRule" : "LogicalTableScanConverterRule") +
+ "') */ id, name, age, _key from PUBLIC.PERSON where name = ?")
+ .withParams(name)
+ .matches(tableScan ?
+ QueryChecker.containsTableScan("PUBLIC", "PERSON") :
+ QueryChecker.containsIndexScan("PUBLIC", "PERSON",
PRIMARY_KEY_INDEX)
+ )
+ .columnNames("ID", "NAME", "AGE", KEY_FIELD_NAME)
+ .returns(id, name, 24, _key)
+ .check();
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyTableScan() {
+ compositePkWithOrderByKey(true);
+ }
+
+ /** */
+ @Test
+ public void testCompositePkWithOrderByKeyIdxScan() {
+ compositePkWithOrderByKey(false);
+ }
+
+ /**
+ * Tests composite primary key ordering by {@code _key}.
+ * Verifies that ordering by the composite primary key produces correct
results
+ * when comparing binary objects using {@link #binaryObjectCmpForDml}.
+ */
+ public void compositePkWithOrderByKey(boolean tableScan) {
+ sql("create table PUBLIC.PERSON(id int, name varchar, surname varchar,
age int, primary key(id, name))");
+
+ fillTable();
+
+ List<List<?>> sqlRs = sql("select id, name, age, _key from
PUBLIC.PERSON");
+ sqlRs.sort((o1, o2) -> binaryObjectCmpForDml(o1.get(3), o2.get(3)));
Review Comment:
Why are BO compared instead of PDJO?
--
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]
