zstan commented on code in PR #6374:
URL: https://github.com/apache/ignite-3/pull/6374#discussion_r2269433479
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/mapping/MappedFragment.java:
##########
@@ -131,12 +131,32 @@ public Long2ObjectMap<ColocationGroup> groupsBySourceId()
{
public MappedFragment
replaceColocationGroups(Long2ObjectMap<ColocationGroup> replacedGroups) {
List<ColocationGroup> newGroups = new
ArrayList<>(groupsBySourceId.size());
+ // Because a colocation group may contain multiple sources, partition
pruning (PP) splits a colocation group in multiple groups.
+ // Each source id affected by PP goes into a separate group.
+ //
+ // Consider the following scenario:
+ // ColocationGroup [ sourceIds = [0, 1], ... ] where sourceIds point
to the same table but the one with sourceId=0
+ // has a predicate and the one with source=2 does not. In this case we
need to create two colocation groups:
+ // one for sourceId=0 and another for sourceId=1.
+ //
+ // We should get these colocation groups in the end:
+ //
+ // - ColocationGroup [ sourceId = [0] ... ] this one has the number of
partitions reduced.
+ // - ColocationGroup [ sourceId = [1] ... ] which has all partitions.
Review Comment:
```suggestion
// - ColocationGroup [ sourceId = [1] ... ] this one has all
partitions.
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/exec/mapping/MappedFragment.java:
##########
@@ -131,12 +131,32 @@ public Long2ObjectMap<ColocationGroup> groupsBySourceId()
{
public MappedFragment
replaceColocationGroups(Long2ObjectMap<ColocationGroup> replacedGroups) {
List<ColocationGroup> newGroups = new
ArrayList<>(groupsBySourceId.size());
+ // Because a colocation group may contain multiple sources, partition
pruning (PP) splits a colocation group in multiple groups.
+ // Each source id affected by PP goes into a separate group.
+ //
+ // Consider the following scenario:
+ // ColocationGroup [ sourceIds = [0, 1], ... ] where sourceIds point
to the same table but the one with sourceId=0
+ // has a predicate and the one with source=2 does not. In this case we
need to create two colocation groups:
Review Comment:
```suggestion
// has a predicate and the one with sourceId=1 does not. In this
case we need to create two colocation groups:
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/pruning/PartitionPruningMetadata.java:
##########
@@ -40,6 +42,11 @@ public class PartitionPruningMetadata implements
Serializable {
/** Constructor. */
public PartitionPruningMetadata(Long2ObjectMap<PartitionPruningColumns>
data) {
+ for (long id : data.keySet()) {
+ if (id == -1) {
+ throw new IllegalArgumentException("sourceId has not been set:
" + data);
Review Comment:
i think - if it used for test purpose we need to raise assertions otherwise
it looks like unreadable from user point of view:
```
java.lang.ExceptionInInitializerError
at
java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:710)
at
org.apache.ignite.internal.util.ExceptionUtils$10.copy(ExceptionUtils.java:1053)
at
org.apache.ignite.internal.util.ExceptionUtils$ExceptionFactory.createCopy(ExceptionUtils.java:875)
at
org.apache.ignite.internal.util.ExceptionUtils.copyExceptionWithCause(ExceptionUtils.java:677)
at
org.apache.ignite.internal.util.ExceptionUtils.copyExceptionWithCauseInternal(ExceptionUtils.java:810)
at
org.apache.ignite.internal.util.ExceptionUtils.copyExceptionWithCause(ExceptionUtils.java:655)
at
org.apache.ignite.internal.util.IgniteUtils.getInterruptibly(IgniteUtils.java:838)
at
org.apache.ignite.internal.sql.api.IgniteSqlImpl.sync(IgniteSqlImpl.java:688)
at
org.apache.ignite.internal.sql.api.IgniteSqlImpl.execute(IgniteSqlImpl.java:234)
```
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/pruning/PartitionPrunerImpl.java:
##########
@@ -38,52 +38,53 @@ public class PartitionPrunerImpl implements PartitionPruner
{
@Override
public List<MappedFragment> apply(
List<MappedFragment> mappedFragments,
- Object[] dynamicParameters
+ Object[] dynamicParameters,
+ PartitionPruningMetadata metadata
) {
+ if (metadata.data().isEmpty()) {
+ return mappedFragments;
+ }
+
List<MappedFragment> updatedFragments = new
ArrayList<>(mappedFragments.size());
Long2ObjectMap<List<String>> newNodesByExchangeId = new
Long2ObjectArrayMap<>();
// Partition pruning (PP). For each fragment:
//
- // 1. Extract PP metadata from each fragment's root in the form of
[colo_col1=<val>, ..] (see PartitionPruningMetadataExtractor)
- //
- // 2. If PP metadata exists then update fragment's colocation group
+ // If PP metadata exists then update fragment's colocation group
// to retain partition that are necessary to perform an operator (e.g.
for a scan operator such
// partitions only include that ones that can contain data).
//
// Iterate over fragments again to update fragments that receive data
from fragments updated at step 2.
Review Comment:
no more "step 2 " i suppose ?
--
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
