alex-plekhanov commented on code in PR #12113:
URL: https://github.com/apache/ignite/pull/12113#discussion_r2194970581
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/IgniteMdRowCount.java:
##########
@@ -149,4 +220,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<>();
+
+ for (int i : keys) {
+ if (res.containsKey(i))
+ continue;
+
+ RelColumnOrigin origin = mq.getColumnOrigin(joinInput, i);
+
+ if (origin == null)
+ continue;
+
+ IgniteTable table =
origin.getOriginTable().unwrap(IgniteTable.class);
+
+ if (table == null)
+ continue;
+
+ int keyPos =
keyColumns(table).indexOf(origin.getOriginColumnOrdinal());
+
+ res.put(i, new KeyColumnOrigin(origin, keyPos));
+ }
+
+ return res;
+ }
+
+ /** Returns column numbers of the primary index. */
+ private static ImmutableIntList keyColumns(IgniteTable table) {
+ Map<String, IgniteIndex> indexes = table.indexes();
+
+ if (F.isEmpty(indexes))
+ return ImmutableIntList.of();
+
+ IgniteIndex idx = indexes.get("_key_PK");
Review Comment:
QueryUtils.PRIMARY_KEY_INDEX
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTest.java:
##########
@@ -659,7 +665,11 @@ public void testSplitterPartiallyColocated2() throws
Exception {
IgniteRel phys = physicalPlan(sql, publicSchema);
- assertEquals(3, splitPlan(phys).fragments().size());
+ List<Fragment> fragments = splitPlan(phys).fragments();
Review Comment:
The same as for `testSplitterPartiallyColocatedReplicatedAndPartitioned`
`testSplitterPartiallyColocated1` has wrong plan two now. It produces 3
fragments, because output distribution is single, but not located on node
initiator (but should be tested TrimExchange again)
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/IgniteMdRowCount.java:
##########
@@ -59,62 +67,125 @@ public class IgniteMdRowCount extends RelMdRowCount {
return rel.estimateRowCount(mq);
}
- /** */
- @Nullable public static Double joinRowCount(RelMetadataQuery mq, Join rel)
{
- if (!rel.getJoinType().projectsRight()) {
- // Create a RexNode representing the selectivity of the
- // semijoin filter and pass it to getSelectivity
- RexNode semiJoinSelectivity =
- RelMdUtil.makeSemiJoinSelectivityRexNode(mq, rel);
-
- return multiply(mq.getSelectivity(rel.getLeft(),
semiJoinSelectivity),
- mq.getRowCount(rel.getLeft()));
- }
+ /** Estimates rows number of a join product. If can't, falls back to
Calcite's default implementation. */
+ public static @Nullable Double joinRowCount(RelMetadataQuery mq, Join
join) {
Review Comment:
Let's try to safe variable names close to original code to reduce collisions
later
Let's also get javadoc from original code.
What about IgniteCorrelatedNestedLoopJoin.estimateRowCount? It still divided
by selectivity, do we really need it?
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTest.java:
##########
@@ -543,7 +545,11 @@ public void
testSplitterPartiallyColocatedReplicatedAndPartitioned() throws Exce
IgniteRel phys = physicalPlan(sql, publicSchema);
- assertEquals(3, splitPlan(phys).fragments().size());
+ List<Fragment> fragments = splitPlan(phys).fragments();
+
+ assertEquals(2, fragments.size());
+ assertEquals(1, fragments.stream().filter(fr -> fr.root() instanceof
Join).count());
+ assertEquals(1, fragments.stream().filter(fr -> fr.root() instanceof
IgniteSender).count());
Review Comment:
Looks redundant. It's not what we should check here.
##########
modules/calcite/src/test/java/org/apache/ignite/internal/processors/query/calcite/planner/PlannerTest.java:
##########
@@ -543,7 +545,11 @@ public void
testSplitterPartiallyColocatedReplicatedAndPartitioned() throws Exce
IgniteRel phys = physicalPlan(sql, publicSchema);
- assertEquals(3, splitPlan(phys).fragments().size());
+ List<Fragment> fragments = splitPlan(phys).fragments();
Review Comment:
This test intented to test trim-exchange to exchange convertation, with
current change it's not planned as trim exchange, but planned as exchange to
single instead.
To plan as TrimExchange you can change the condition to reduce count of rows
on left hand, for example:
`WHERE (d.projectId + 1) = ?`
Perhaps it worth to rewrite test to make it more simple.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/IgniteMdColumnOrigins.java:
##########
@@ -92,33 +88,10 @@ public class IgniteMdColumnOrigins implements
MetadataHandler<BuiltInMetadata.Co
return set;
}
- /** */
- public @Nullable Set<RelColumnOrigin> getColumnOrigins(Join rel,
RelMetadataQuery mq,
- int iOutputColumn) {
- int nLeftColumns = rel.getLeft().getRowType().getFieldList().size();
- Set<RelColumnOrigin> set;
- boolean derived = false;
-
- if (iOutputColumn < nLeftColumns) {
- set = mq.getColumnOrigins(rel.getLeft(), iOutputColumn);
-
- if (rel.getJoinType().generatesNullsOnLeft())
- derived = true;
-
- }
- else {
- set = mq.getColumnOrigins(rel.getRight(), iOutputColumn -
nLeftColumns);
-
- if (rel.getJoinType().generatesNullsOnRight())
- derived = true;
- }
-
- if (derived) {
- // nulls are generated due to outer join; that counts
- // as derivation
- set = createDerivedColumnOrigins(set);
- }
- return set;
Review Comment:
It looks correct, why it was removed?
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/metadata/IgniteMdColumnOrigins.java:
##########
@@ -183,26 +156,6 @@ public class IgniteMdColumnOrigins implements
MetadataHandler<BuiltInMetadata.Co
return createDerivedColumnOrigins(set);
}
- /** */
- public @Nullable Set<RelColumnOrigin> getColumnOrigins(Filter rel,
RelMetadataQuery mq, int iOutputColumn) {
- return mq.getColumnOrigins(rel.getInput(), iOutputColumn);
- }
-
- /** */
- public @Nullable Set<RelColumnOrigin> getColumnOrigins(Sort rel,
RelMetadataQuery mq, int iOutputColumn) {
- return mq.getColumnOrigins(rel.getInput(), iOutputColumn);
- }
-
- /** */
- public @Nullable Set<RelColumnOrigin> getColumnOrigins(TableModify rel,
RelMetadataQuery mq, int iOutputColumn) {
- return mq.getColumnOrigins(rel.getInput(), iOutputColumn);
- }
-
- /** */
- public @Nullable Set<RelColumnOrigin> getColumnOrigins(Exchange rel,
RelMetadataQuery mq, int iOutputColumn) {
- return mq.getColumnOrigins(rel.getInput(), iOutputColumn);
- }
-
/** */
Review Comment:
Why these methods was removed? getColumnOrigins RelNode doesn't cover these
cases.
##########
modules/calcite/src/main/java/org/apache/ignite/internal/processors/query/calcite/rel/ProjectableFilterableTableScan.java:
##########
@@ -195,4 +195,13 @@ public RelColumnOrigin columnOriginsByRelLocalRef(int
colIdx) {
return new RelColumnOrigin(getTable(), originColIdx, false);
}
+
+ /** Returns original index of a table column related to the projected
index. */
+ public RelColumnOrigin tableColOffset(int projectColIdx) {
+ int originColIdx = (requiredColumns == null)
+ ? projectColIdx
+ : Commons.mapping(requiredColumns,
getTable().getRowType().getFieldCount()).getTarget(requiredColumns.nth(projectColIdx));
Review Comment:
What's the difference with columnOriginsByRelLocalRef? Looks like requred
the same and looks like method returns the same (but using 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-unsubscr...@ignite.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
