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


##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/IgniteMdRowCount.java:
##########
@@ -282,9 +288,21 @@ private static IntMap<KeyColumnOrigin> 
findOrigins(RelMetadataQuery mq, RelNode
             if (table == null)
                 continue;
 
-            int keyPos = 
keyColumns(table).indexOf(origin.getOriginColumnOrdinal());
+            int srcKeyColIdx = origin.getOriginColumnOrdinal();
+
+            RelDataType insertRowType = 
table.descriptor().insertRowType(Commons.typeFactory(joinInput));
+            RelDataType curRowType = origin.getOriginTable().getRowType();
+
+            assert curRowType.getFieldCount() >= insertRowType.getFieldCount();
+
+            if (curRowType.getFieldCount() > insertRowType.getFieldCount()) {
+                /** Current row type probably contains {@link 
QueryUtils#KEY_FIELD_NAME} and {@link QueryUtils#VAL_FIELD_NAME}. */
+                srcKeyColIdx -= curRowType.getFieldCount() - 
insertRowType.getFieldCount();
+            }

Review Comment:
   1. insertRowType can contain _KEY/_VAL fields as well (see 
DistributedJoinIntegrationTest#testTrimExchange for example)
   2. Join can be on _KEY field, in this case srcKeyColIdx will be negative
   3. Current implementation is wrong. Implementation without shift is even 
more correct (but doesn't take into account key field aliases and proxy PK 
index). Foe example any two fields PK index is not working:
   ```
           sql("CREATE TABLE t1(id1 INT, id2 INT, val VARCHAR, PRIMARY KEY(id1, 
id2))");
           sql("CREATE TABLE t2(id1 INT, id2 INT, val VARCHAR, PRIMARY KEY(id1, 
id2))");
           sql("SELECT * FROM t1 JOIN t2 USING(id1, id2)");
   ```
   4. There are a lot of errors in this part of code already. Can we introduce 
some unit test to check this code explicitely? Because current tests doesn't 
check this code at all. 



##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/IgniteMdRowCount.java:
##########
@@ -149,4 +263,195 @@ public double getRowCount(IgniteAggregate rel, 
RelMetadataQuery mq) {
     public double getRowCount(IgniteLimit rel, RelMetadataQuery mq) {
         return rel.estimateRowCount(mq);
     }
+
+    /** */
+    private static IntMap<KeyColumnOrigin> findOrigins(RelMetadataQuery mq, 
RelNode joinInput, ImmutableIntList keys) {
+        IntMap<KeyColumnOrigin> res = new IntRWHashMap<>();

Review Comment:
   No concurrency here, IntHashMap can be used. Maybe even array 
(KeyColumnOrigin[]) since key is limited 



-- 
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-unsubscr...@ignite.apache.org

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

Reply via email to