You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by GitBox <gi...@apache.org> on 2022/12/20 08:54:39 UTC
[GitHub] [ignite-3] korlov42 commented on a diff in pull request #1434: IGNITE-18211 Sql. Adjust Affinity distribution to the new colocation rules.
korlov42 commented on code in PR #1434:
URL: https://github.com/apache/ignite-3/pull/1434#discussion_r1052963930
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/externalize/RelJson.java:
##########
@@ -657,10 +666,13 @@ IgniteDistribution toDistribution(Object distribution) {
}
Map<String, Object> map = (Map<String, Object>) distribution;
- Number cacheId = (Number) map.get("cacheId");
- if (cacheId != null) {
- return IgniteDistributions.hash(ImmutableIntList.copyOf((List<Integer>) map.get("keys")),
- DistributionFunction.affinity(cacheId.intValue(), cacheId));
+ String tableIdStr = (String) map.get("tableId");
+
+ if (tableIdStr != null) {
+ UUID tableId = UUID.fromString(tableIdStr);
+ Object zoneId = map.get("zoneId");
+
+ return IgniteDistributions.hash((List<Integer>) map.get("keys"), DistributionFunction.affinity(tableId, zoneId));
Review Comment:
Why don't you use `IgniteDistributions#affinity`?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/TableDescriptorImpl.java:
##########
@@ -66,13 +59,17 @@ public class TableDescriptorImpl extends NullInitializerExpressionFactory implem
private final ImmutableBitSet keyFields;
+ private final IgniteDistribution distribution;
+
/**
* Constructor.
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
*/
public TableDescriptorImpl(
List<ColumnDescriptor> columnDescriptors,
- IntList colocationColumns
+ IntList colocationColumns,
+ UUID tableId,
+ Object zoneId
Review Comment:
Let's provide distribution as a param
##########
modules/sql-engine/src/test/java/org/apache/ignite/internal/sql/engine/planner/JoinColocationPlannerTest.java:
##########
@@ -162,13 +159,14 @@ public void joinComplexToSimpleAff() throws Exception {
}
/**
- * Re-hashing for complex affinity is not supported.
+ * Re-hashing for complex affinity.
Review Comment:
Could you please add more details to the javadoc, describing what's going on and why do we expect the particular result?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/schema/TableDescriptor.java:
##########
@@ -109,4 +110,11 @@ default RelDataType selectForUpdateRowType(IgniteTypeFactory factory) {
* @return Actual count of columns.
*/
int columnsCount();
+
+ /**
+ * Returns list of colocation columns of the table.
+ *
+ * @return Actual list of colocation columns.
+ */
+ List<Integer> colocationColumns();
Review Comment:
why do we need this?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/ExecutionServiceImpl.java:
##########
@@ -717,4 +724,59 @@ private CompletableFuture<Void> closeExecNode(boolean cancel) {
interface ImplementorFactory<RowT> {
LogicalRelImplementor<RowT> create(ExecutionContext<RowT> ctx);
}
+
+ private static class ColocationServiceImpl<T> implements ColocationHashFunctionFactory<T> {
+ private final SqlSchemaManager sqlSchemaManager;
+ private final RowHandler<T> rowHandler;
+
+ private ColocationServiceImpl(SqlSchemaManager sqlSchemaManager, RowHandler<T> rowHandler) {
+ this.sqlSchemaManager = sqlSchemaManager;
+ this.rowHandler = rowHandler;
+ }
+
+ @Override
+ public ColocationHashFunction<T> create(UUID tableId, int[] colocationFields) {
+ return new ColocationHashFunctionImpl(tableId, colocationFields);
+ }
+
+ private class ColocationHashFunctionImpl implements ColocationHashFunction<T> {
+ private final int[] colocationFields;
+ private final NativeType[] colTypes;
+
+ private ColocationHashFunctionImpl(UUID tableId, int[] colocationFields) {
+ this.colTypes = colocationColumnTypes(tableId, colocationFields.length);
+ this.colocationFields = colocationFields;
+ }
+
+ private NativeType[] colocationColumnTypes(UUID tableId, int fieldCnt) {
+ NativeType[] colTypes = new NativeType[fieldCnt];
+ TableDescriptor tblDesc = sqlSchemaManager.tableById(tableId, -1).descriptor();
+
+ List<Integer> colocationColumns = tblDesc.colocationColumns();
+
+ assert colocationColumns.size() >= fieldCnt : "fieldsCount=" + fieldCnt + ", colocationColumns=" + colocationColumns;
Review Comment:
How it's going to work if table colocated by more column than we expected?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/trait/IgniteDistributions.java:
##########
@@ -62,44 +63,28 @@ public static IgniteDistribution broadcast() {
return BROADCAST;
}
- /**
- * Affinity.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
- *
- * @param key Affinity key.
- * @param cacheName Affinity cache name.
- * @param identity Affinity identity key.
- * @return Affinity distribution.
- */
- public static IgniteDistribution affinity(int key, String cacheName, Object identity) {
- // TODO: fix cacheId
- return affinity(key, 0, identity);
- }
-
/**
* Affinity.
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
Review Comment:
please improve the javadoc here and below
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/trait/IgniteDistributions.java:
##########
@@ -62,44 +63,28 @@ public static IgniteDistribution broadcast() {
return BROADCAST;
}
- /**
- * Affinity.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
- *
- * @param key Affinity key.
- * @param cacheName Affinity cache name.
- * @param identity Affinity identity key.
- * @return Affinity distribution.
- */
- public static IgniteDistribution affinity(int key, String cacheName, Object identity) {
- // TODO: fix cacheId
- return affinity(key, 0, identity);
- }
-
/**
* Affinity.
* TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
*
* @param key Affinity key.
- * @param cacheId Affinity cache ID.
- * @param identity Affinity identity key.
+ * @param zoneId Affinity zone ID.
* @return Affinity distribution.
*/
- public static IgniteDistribution affinity(int key, int cacheId, Object identity) {
- return hash(ImmutableIntList.of(key), DistributionFunction.affinity(cacheId, identity));
+ public static IgniteDistribution affinity(int key, UUID tableId, Object zoneId) {
Review Comment:
does it make sense to change type of the `zoneId` param to int?
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/trait/DistributionFunction.java:
##########
@@ -243,27 +239,35 @@ public <RowT> Destination<RowT> destination(ExecutionContext<RowT> ctx, Affinity
}
}
- AffinityAdapter<RowT> affinity = new AffinityAdapter<>(affSrvc.affinity(intFoo()/*CU.UNDEFINED_CACHE_ID*/), k.toIntArray(),
- ctx.rowHandler());
+ return new Partitioned<>(assignments, row -> {
+ int hash = 0;
+
+ for (int idx : k) {
+ hash = 31 * hash + ctx.rowHandler().get(idx, row).hashCode();
Review Comment:
looks like need to cover DistributionFunctions with unit tests
##########
modules/schema/src/main/java/org/apache/ignite/internal/util/ColocationHashFunction.java:
##########
@@ -15,20 +15,17 @@
* limitations under the License.
*/
-package org.apache.ignite.internal.sql.engine.metadata;
-
-import java.util.function.ToIntFunction;
+package org.apache.ignite.internal.util;
/**
- * AffinityService interface.
- * TODO Documentation https://issues.apache.org/jira/browse/IGNITE-15859
+ * Function to calculate a hash of the colocation fields of a row.
*/
-public interface AffinityService {
+@FunctionalInterface
+public interface ColocationHashFunction<T> {
Review Comment:
Why do we need such interface in a Schema module? To be honest, I don't get the idea
--
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