[GitHub] [ignite] alex-plekhanov commented on a diff in pull request #10788: IGNITE-19725 SQL Calclite: Implement local flag support

["alex-plekhanov (via GitHub)" <gi...@apache.org>]

alex-plekhanov commented on code in PR #10788:
URL: https://github.com/apache/ignite/pull/10788#discussion_r1247786319


##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rule/LogicalScanConverterRule.java:
##########
@@ -125,7 +132,13 @@ public abstract class LogicalScanConverterRule<T extends ProjectableFilterableTa
                 RelOptCluster cluster = rel.getCluster();
                 IgniteTable table = rel.getTable().unwrap(IgniteTable.class);
 
-                RelDistribution distribution = table.distribution();
+                RelDistribution distribution;
+                QueryProperties qryProps = planner.getContext().unwrap(QueryProperties.class);

Review Comment:
   QueryProperties is an ignite-core entity. It's not a good idea to use such an entities for rules, to keep rules transferable between ignite2 and ignite3. I think it's better to use some field in PlanningContext (or perhaps in BaseQueryContext to use it for MappingContext too)



##########
modules/core/src/main/java/org/apache/ignite/internal/processors/query/QueryProperties.java:
##########
@@ -30,9 +31,13 @@ public final class QueryProperties {
     private final boolean keepBinary;
 
     /** */
-    public QueryProperties(@Nullable String cacheName, boolean keepBinary) {
+    private final boolean isLocal;

Review Comment:
   It's not neccessary to add a new field, local flag can be accessed inside calcite engine by call to `context().unwrap(SqlFieldsQuery.class)`



##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/QueryTemplate.java:
##########
@@ -23,7 +23,6 @@
 import java.util.Map;
 import java.util.Objects;
 import java.util.concurrent.atomic.AtomicReference;
-

Review Comment:
   redundant change



##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/prepare/Fragment.java:
##########
@@ -142,8 +143,12 @@ private FragmentMapping mapping(MappingQueryContext ctx, RelMetadataQuery mq, Su
         try {
             FragmentMapping mapping = IgniteMdFragmentMapping._fragmentMapping(root, mq, ctx);
 
-            if (rootFragment())
-                mapping = FragmentMapping.create(ctx.localNodeId()).colocate(mapping);
+            if (rootFragment()) {
+                if (ctx.isLocal() && !(root instanceof IgniteTableModify))

Review Comment:
   Why IgniteTableModify is excluded? ColocationGroup affects only scans as far as I know. We can't say to IgniteTableModify to filter local data, but it's absolutely fine to say it to IgniteTableModify's underlying scans. For example `DELETE FROM tbl WHERE a = 1` with local flag should only deal with data located on current node (but should delete backups on other nodes as well), and for example `INSERT INTO tbl1 ... AS SELECT FROM tbl2` with local flag should insert data to tbl1 to any node but only selected local data from tbl2.



##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/CalciteQueryProcessor.java:
##########
@@ -431,11 +432,13 @@ public ExecutionService<Object[]> executionService() {
                     if (plan == null) {
                         AtomicBoolean miss = new AtomicBoolean();
 
-                        plan = queryPlanCache().queryPlan(new CacheKey(schema.getName(), sql, null, params), () -> {
-                            miss.set(true);
+                        plan = queryPlanCache().queryPlan(
+                                new CacheKey(schema.getName(), sql, qryCtx != null ? qryCtx.unwrap(QueryProperties.class) : qryCtx, params),

Review Comment:
   QueryProperties contains properties that should not affect the query plan (keep binary, for example). But now there will be different plan keys and plan can be stored for the same query twice.



##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/ColocationGroup.java:
##########
@@ -77,6 +80,11 @@ public static ColocationGroup forSourceId(long sourceId) {
         return new ColocationGroup(new long[] {sourceId}, null, null);
     }
 
+    /** */
+    public static ColocationGroup create(long[] sourceIds, List<UUID> nodeIds, List<List<UUID>> assignments) {
+        return new ColocationGroup(Arrays.copyOf(sourceIds, sourceIds.length), nodeIds, assignments);

Review Comment:
   `Arrays.copyOf(sourceIds, sourceIds.length)` -> `sourceIds.clone()`?
   I don't like this method at all. Perhaps, colocation group constructor was private for a reason, now we have openned it. Also here we assume that sourceIds can be modified and should be copied, but leave nodeIds and assignments as is. 
   I think it's better to create method in this class to filter assignments for node and return new instance of colocation group instead of creation colocation group in FragmentMapping (move logic of `local` method from `FragmentMapping` to this class). 



-- 
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