You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "xtern (via GitHub)" <gi...@apache.org> on 2023/08/02 10:11:33 UTC
[GitHub] [ignite-3] xtern opened a new pull request, #2397: IGNITE-20030
xtern opened a new pull request, #2397:
URL: https://github.com/apache/ignite-3/pull/2397
(no comment)
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1291482778
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/TableModifyConverterRule.java:
##########
@@ -71,8 +75,29 @@ protected PhysicalNode convert(RelOptPlanner planner, RelMetadataQuery mq, Logic
IgniteTable igniteTable = relTable.unwrap(IgniteTable.class);
assert igniteTable != null;
+ IgniteDistribution distribution = igniteTable.distribution();
+
+ if (rel.getOperation() == Operation.DELETE) {
+ // To perform the delete, we need a row with key fields only.
+ // Input distribution contains the indexes of the key columns according to the schema (i.e. for the full row).
+ // Here we adjusting distribution keys so that a row containing only the key fields can be read.
+ List<Integer> keyFields = new ArrayList<>();
Review Comment:
Done
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ygerzhedovich commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1284415953
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -669,6 +669,16 @@ public ChildT distribution(IgniteDistribution distribution) {
return self();
}
+ /** {@inheritDoc} */
+ @Override
+ public ChildT addKeyColumn(String name, NativeType type) {
+ columns.add(new ColumnDescriptorImpl(
+ name, true, true, columns.size(), columns.size(), type, DefaultValueStrategy.DEFAULT_NULL, null
Review Comment:
why 'nullable' is true?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1287254692
##########
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:
please, ignore comment above, because it doesn't make any sense... Different distributions can be specified explicitly with distribution setter, and on the level of TableDescriptor an order of key columns doesn't make any difference, thus I'm ok to go with proposed change
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1291443954
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/TableModifyConverterRule.java:
##########
@@ -71,8 +75,29 @@ protected PhysicalNode convert(RelOptPlanner planner, RelMetadataQuery mq, Logic
IgniteTable igniteTable = relTable.unwrap(IgniteTable.class);
assert igniteTable != null;
+ IgniteDistribution distribution = igniteTable.distribution();
+
+ if (rel.getOperation() == Operation.DELETE) {
+ // To perform the delete, we need a row with key fields only.
+ // Input distribution contains the indexes of the key columns according to the schema (i.e. for the full row).
+ // Here we adjusting distribution keys so that a row containing only the key fields can be read.
+ List<Integer> keyFields = new ArrayList<>();
Review Comment:
Let's use IntList
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1286728838
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -281,7 +281,7 @@ private <RowT> BinaryRowEx convertRow(RowT row, ExecutionContext<RowT> ectx, boo
continue;
}
- Object value = hnd.get(colDesc.logicalIndex(), row);
+ Object value = hnd.get(keyOnly ? colDesc.physicalIndex() : colDesc.logicalIndex(), row);
Review Comment:
can we union these 2 loops : for (ColumnDescriptor colDesc : columnsOrderedByPhysSchema) { ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1288567298
##########
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:
Thank you, I believe it's fixed now.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1288568303
##########
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:
Thanks, fixed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] zstan commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "zstan (via GitHub)" <gi...@apache.org>.
zstan commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1286730156
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/TableModifyConverterRule.java:
##########
@@ -126,4 +138,34 @@ 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>
Review Comment:
as discussed earlier - can you extend this java doc ?
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1286940525
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -281,7 +281,7 @@ private <RowT> BinaryRowEx convertRow(RowT row, ExecutionContext<RowT> ectx, boo
continue;
}
- Object value = hnd.get(colDesc.logicalIndex(), row);
+ Object value = hnd.get(keyOnly ? colDesc.physicalIndex() : colDesc.logicalIndex(), row);
Review Comment:
Thanks, removed first loop.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1286744571
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/TableModifyConverterRule.java:
##########
@@ -126,4 +138,34 @@ 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>
Review Comment:
Expanded javadoc with more descriptive examples.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] AMashenkov commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "AMashenkov (via GitHub)" <gi...@apache.org>.
AMashenkov commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1291413162
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -107,6 +119,24 @@ public UpdatableTableImpl(
tmp.sort(Comparator.comparingInt(ColumnDescriptor::physicalIndex));
columnsOrderedByPhysSchema = tmp;
+
+ int keyColumnsCount = schemaDescriptor.keyColumns().length();
+
+ List<ColumnDescriptor> keyCols = new ArrayList<>(keyColumnsCount);
+ List<Integer> keyLogicalIndexes = new ArrayList<>(keyColumnsCount);
+
+ for (ColumnDescriptor colDesc : tmp) {
+ if (colDesc.key()) {
+ keyCols.add(colDesc);
+ keyLogicalIndexes.add(colDesc.logicalIndex());
+ }
+ }
+
+ keyColumnsOrderedByPhysSchema = keyCols;
+
+ ImmutableBitSet keysBitSet = ImmutableBitSet.of(keyLogicalIndexes);
Review Comment:
Let's use ImmutableBitSet of(int... bits)
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -107,6 +119,24 @@ public UpdatableTableImpl(
tmp.sort(Comparator.comparingInt(ColumnDescriptor::physicalIndex));
columnsOrderedByPhysSchema = tmp;
+
+ int keyColumnsCount = schemaDescriptor.keyColumns().length();
+
+ List<ColumnDescriptor> keyCols = new ArrayList<>(keyColumnsCount);
+ List<Integer> keyLogicalIndexes = new ArrayList<>(keyColumnsCount);
Review Comment:
```suggestion
int[] keyLogicalIndexes = new int[keyColumnsCount];
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -107,6 +119,24 @@ public UpdatableTableImpl(
tmp.sort(Comparator.comparingInt(ColumnDescriptor::physicalIndex));
Review Comment:
If we have full column set here, then all columns will have distinct physical index in range `[0..desc.columnsCount]` without gaps.
That mean we don't need a n*log(n) sort and can fill array in one pass.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1291481729
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -107,6 +119,24 @@ public UpdatableTableImpl(
tmp.sort(Comparator.comparingInt(ColumnDescriptor::physicalIndex));
columnsOrderedByPhysSchema = tmp;
+
+ int keyColumnsCount = schemaDescriptor.keyColumns().length();
+
+ List<ColumnDescriptor> keyCols = new ArrayList<>(keyColumnsCount);
+ List<Integer> keyLogicalIndexes = new ArrayList<>(keyColumnsCount);
Review Comment:
Done
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -107,6 +119,24 @@ public UpdatableTableImpl(
tmp.sort(Comparator.comparingInt(ColumnDescriptor::physicalIndex));
columnsOrderedByPhysSchema = tmp;
+
+ int keyColumnsCount = schemaDescriptor.keyColumns().length();
+
+ List<ColumnDescriptor> keyCols = new ArrayList<>(keyColumnsCount);
+ List<Integer> keyLogicalIndexes = new ArrayList<>(keyColumnsCount);
+
+ for (ColumnDescriptor colDesc : tmp) {
+ if (colDesc.key()) {
+ keyCols.add(colDesc);
+ keyLogicalIndexes.add(colDesc.logicalIndex());
+ }
+ }
+
+ keyColumnsOrderedByPhysSchema = keyCols;
+
+ ImmutableBitSet keysBitSet = ImmutableBitSet.of(keyLogicalIndexes);
Review Comment:
Done
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1291480497
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -107,6 +119,24 @@ public UpdatableTableImpl(
tmp.sort(Comparator.comparingInt(ColumnDescriptor::physicalIndex));
Review Comment:
sort removed
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "korlov42 (via GitHub)" <gi...@apache.org>.
korlov42 commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1291102021
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/util/PlanUtils.java:
##########
@@ -144,14 +144,33 @@ public static Mapping computeAggFieldMapping(List<ImmutableBitSet> groupingSets)
}
}
- Mapping mapping = Mappings.create(MappingType.INVERSE_SURJECTION, fieldIndices.length(), fieldIndices.cardinality());
+ return sortedValuesIndexMapping(fieldIndices);
+ }
+
+ /**
+ * Creates a mapping such that each value is replaced by its position in the ordered list.
+ *
+ * <p>For example, for the following values {@code [2, 3, 5, 7, 9]} the sorting positions
+ * will be {@code [0, 1, 2, 3, 4]}, the resulting mapping will be:
+ * <pre>
+ * 2 -> 0
+ * 3 -> 1
+ * 5 -> 2
+ * 7 -> 3
+ * 9 -> 4
+ * </pre>
+ *
+ * @param values Values to be mapped to its position in the ordered list.
+ */
+ public static Mapping sortedValuesIndexMapping(BitSet values) {
Review Comment:
well, we already have `org.apache.ignite.internal.sql.engine.util.Commons#trimmingMapping`...
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/rule/TableModifyConverterRule.java:
##########
@@ -71,8 +76,27 @@ protected PhysicalNode convert(RelOptPlanner planner, RelMetadataQuery mq, Logic
IgniteTable igniteTable = relTable.unwrap(IgniteTable.class);
assert igniteTable != null;
+ IgniteDistribution distribution = igniteTable.distribution();
+
+ if (rel.getOperation() == Operation.DELETE) {
+ // To perform the delete, we need a row with key fields only.
+ // Input distribution contains the indexes of the key columns according to the schema (i.e. for the full row).
+ // Here we adjusting distribution keys so that a row containing only the key fields can be read.
+ BitSet keyFields = new BitSet();
+
+ for (int i = 0; i < igniteTable.descriptor().columnsCount(); i++) {
+ ColumnDescriptor column = igniteTable.descriptor().columnDescriptor(i);
+
+ if (column.key()) {
+ keyFields.set(column.logicalIndex());
+ }
+ }
+
+ distribution = distribution.apply(PlanUtils.sortedValuesIndexMapping(keyFields));
Review Comment:
the same, let's use `org.apache.ignite.internal.sql.engine.util.Commons#trimmingMapping`
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/UpdatableTableImpl.java:
##########
@@ -71,6 +74,11 @@ public final class UpdatableTableImpl implements UpdatableTable {
private final List<ColumnDescriptor> columnsOrderedByPhysSchema;
+ private final List<ColumnDescriptor> keyColumnsOrderedByPhysSchema;
+
+ /** Mapping of column indexes for a row containing only primary key columns. */
Review Comment:
let's add a few details describing WHY do we need such mapping
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern closed pull request #2397: IGNITE-20030
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern closed pull request #2397: IGNITE-20030
URL: https://github.com/apache/ignite-3/pull/2397
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1284529326
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -669,6 +669,16 @@ public ChildT distribution(IgniteDistribution distribution) {
return self();
}
+ /** {@inheritDoc} */
+ @Override
+ public ChildT addKeyColumn(String name, NativeType type) {
+ columns.add(new ColumnDescriptorImpl(
+ name, true, true, columns.size(), columns.size(), type, DefaultValueStrategy.DEFAULT_NULL, null
Review Comment:
Thanks, fixed.
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] ygerzhedovich commented on a diff in pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "ygerzhedovich (via GitHub)" <gi...@apache.org>.
ygerzhedovich commented on code in PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397#discussion_r1284417083
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/framework/TestBuilders.java:
##########
@@ -669,6 +669,16 @@ public ChildT distribution(IgniteDistribution distribution) {
return self();
}
+ /** {@inheritDoc} */
+ @Override
+ public ChildT addKeyColumn(String name, NativeType type) {
+ columns.add(new ColumnDescriptorImpl(
+ name, true, true, columns.size(), columns.size(), type, DefaultValueStrategy.DEFAULT_NULL, null
Review Comment:
`The primary key constraint specifies that a column or columns of a table can contain only unique (non-duplicate), nonnull values. Technically, PRIMARY KEY is merely a combination of UNIQUE and NOT NULL.`
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [ignite-3] xtern merged pull request #2397: IGNITE-20030 Delete operation fails if PK is set on arbitrary columns.
Posted by "xtern (via GitHub)" <gi...@apache.org>.
xtern merged PR #2397:
URL: https://github.com/apache/ignite-3/pull/2397
--
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: notifications-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org