korlov42 commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1287016497
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -281,29 +281,17 @@ public <RowT> CompletableFuture<?> deleteAll(
private <RowT> BinaryRowEx convertRow(RowT row, ExecutionContext<RowT>
ectx, boolean keyOnly) {
RowHandler<RowT> hnd = ectx.rowHandler();
- for (ColumnDescriptor colDesc : columnsOrderedByPhysSchema) {
- if (keyOnly && !colDesc.key()) {
- continue;
- }
-
- Object value = hnd.get(colDesc.logicalIndex(), row);
-
- // TODO Remove this check when
https://issues.apache.org/jira/browse/IGNITE-19096 is complete
- assert value != DEFAULT_VALUE_PLACEHOLDER;
-
- if (value == null) {
- break;
- }
- }
-
RowAssembler rowAssembler = keyOnly ?
RowAssembler.keyAssembler(schemaDescriptor) : new
RowAssembler(schemaDescriptor);
for (ColumnDescriptor colDesc : columnsOrderedByPhysSchema) {
if (keyOnly && !colDesc.key()) {
continue;
}
- Object val = hnd.get(colDesc.logicalIndex(), row);
+ Object val = hnd.get(keyOnly ? colDesc.physicalIndex() :
colDesc.logicalIndex(), row);
Review Comment:
this is not correct. Physical index specified order of columns in tuple that
is stored in persistence, not the order after projection
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/TableModifyConverterRule.java:
##########
@@ -126,4 +138,35 @@ private static PhysicalNode
createAggregate(IgniteTableModify tableModify, RelOp
return new IgniteProject(cluster,
outTrait.replace(IgniteDistributions.single()), sumAgg, projections,
convertedRowType);
}
+
+ /**
+ * To perform delete we need row with key fields only.
+ * But input distribution contains the indexes of the key columns
according to the schema (i.e. for the full row).
+ * This method aligns the keys to their indexes so that a row containing
only the key fields can be read.
+ *
+ * <pre>
+ * For example we have a table with the following columns in the following
order: a INT, b INT, c INT, d INT.
+ *
+ * 1. (a) is PK. Key fields row contains [a], we can read it using
0-index. No alignment required.
+ * 2. (b) is PK. Key fields row contains [b], distribution key contains
logical index '1', which requires it to be "shifted" to '0'.
+ * 3. (b, d) is PK. Key fields row contains [b, d], distribution keys [1,
3] need to be shifted to [0, 1].
+ * 4. (d, b, c) is PK. Key fields row contains [d, b, c]. distribution
keys [3, 1, 2] need to be shifted to [2, 0, 1].
+ * </pre>
+ *
+ * @param distribution Distribution specification.
+ * @return Distribution with adjusted keys.
+ */
+ private IgniteDistribution
adjustDistributionKeysForDelete(IgniteDistribution distribution) {
Review Comment:
this adjustment doesn't take into account that colocation columns may be
only a subset of key columns.
Assume a table `CREATE TABLE myT (key1 INT, key2 INT, ..., PRIMARY KEY
(key1, key2)) COLOCATE BY (key2)`. For this table, input row type will be
`(key1, key2)` and distribution `affnity[1]`.
After adjustment, distribution will be `affinity[0]`, which is not correct
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -780,6 +790,9 @@ private interface TableBuilderBase<ChildT> {
/** Sets the distribution of the table. */
ChildT distribution(IgniteDistribution distribution);
+ /** Adds a key column to the table. */
+ ChildT addKeyColumn(String name, NativeType type);
Review Comment:
I would prefer to have separate method `primaryKey(String... cols)`. The
reason is that order of columns in primary key may differ from order of columns
in table row. With current approach it's not possible to specify primary key in
order different from columns in table
--
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]
