You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@ignite.apache.org by "Vladsz83 (via GitHub)" <gi...@apache.org> on 2025/05/29 14:46:42 UTC
[PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Vladsz83 opened a new pull request, #12113:
URL: https://github.com/apache/ignite/pull/12113
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
### The Contribution Checklist
- [ ] There is a single JIRA ticket related to the pull request.
- [ ] The web-link to the pull request is attached to the JIRA ticket.
- [ ] The JIRA ticket has the _Patch Available_ state.
- [ ] The pull request body describes changes that have been made.
The description explains _WHAT_ and _WHY_ was made instead of _HOW_.
- [ ] The pull request title is treated as the final commit message.
The following pattern must be used: `IGNITE-XXXX Change summary` where `XXXX` - number of JIRA issue.
- [ ] A reviewer has been mentioned through the JIRA comments
(see [the Maintainers list](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute#HowtoContribute-ReviewProcessandMaintainers))
- [ ] The pull request has been checked by the Teamcity Bot and
the `green visa` attached to the JIRA ticket (see [TC.Bot: Check PR](https://mtcga.gridgain.com/prs.html))
### Notes
- [How to Contribute](https://cwiki.apache.org/confluence/display/IGNITE/How+to+Contribute)
- [Coding abbreviation rules](https://cwiki.apache.org/confluence/display/IGNITE/Abbreviation+Rules)
- [Coding Guidelines](https://cwiki.apache.org/confluence/display/IGNITE/Coding+Guidelines)
- [Apache Ignite Teamcity Bot](https://cwiki.apache.org/confluence/display/IGNITE/Apache+Ignite+Teamcity+Bot)
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com _#ignite_ channel.
--
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
Re: [PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12113:
URL: https://github.com/apache/ignite/pull/12113#discussion_r2210248149
##########
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:
All these methods just copy `RelMdColumnOrigins`. Also are removed in AI3. I suggest the same: keep only changed methods.
--
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
Re: [PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12113:
URL: https://github.com/apache/ignite/pull/12113#discussion_r2205429423
##########
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:
This is the difference - mapping. Without it, `IgniteMdRowCount#joinRowCount()` cannot determine proper column origin and find the index.
--
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
Re: [PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 closed pull request #12113: IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2
URL: https://github.com/apache/ignite/pull/12113
--
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
Re: [PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12113:
URL: https://github.com/apache/ignite/pull/12113#discussion_r2226288173
##########
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:
See new impl.
--
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
Re: [PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Posted by "Vladsz83 (via GitHub)" <gi...@apache.org>.
Vladsz83 commented on code in PR #12113:
URL: https://github.com/apache/ignite/pull/12113#discussion_r2210248951
##########
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:
All these methods just copy `RelMdColumnOrigins`. Also are removed in AI3. I suggest the same: keep only changed methods.
--
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
Re: [PR] IGNITE-25129 : Calcite. TPC-H query #5: performance drop on scale 0.01 V2 [ignite]
Posted by "alex-plekhanov (via GitHub)" <gi...@apache.org>.
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-unsubscribe@ignite.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org