korlov42 commented on code in PR #4151:
URL: https://github.com/apache/ignite-3/pull/4151#discussion_r1717010499
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/pruning/PartitionPruningMetadataExtractor.java:
##########
@@ -156,57 +156,99 @@ public IgniteRel visit(IgniteTableScan rel) {
}
private static class ModifyNodeShuttle extends IgniteRelShuttle {
- List<RexNode> projections = null;
- List<List<RexNode>> expressions = null;
- boolean unionRaised = false;
+ List<TargetMapping> mapping;
IgniteRel previous = null;
private static final Map<Class<?>, Set<Class<?>>> allowRelTransfers =
new HashMap<>();
+ List<List<RexNode>> finalExpressions = new ArrayList<>();
+ IgniteTable table;
+
+ List<List<RexNode>> prevProjects = new ArrayList<>();
+
+ ModifyNodeShuttle(IgniteTable table) {
+ this.table = table;
+ }
+
static {
allowRelTransfers.put(IgniteValues.class,
Set.of(IgniteProject.class, IgniteExchange.class, IgniteTrimExchange.class));
Review Comment:
transition UnionAll -> Values is missing (as well as tests I believe)
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/pruning/PartitionPruningMetadataExtractor.java:
##########
@@ -156,57 +156,99 @@ public IgniteRel visit(IgniteTableScan rel) {
}
private static class ModifyNodeShuttle extends IgniteRelShuttle {
- List<RexNode> projections = null;
- List<List<RexNode>> expressions = null;
- boolean unionRaised = false;
+ List<TargetMapping> mapping;
IgniteRel previous = null;
private static final Map<Class<?>, Set<Class<?>>> allowRelTransfers =
new HashMap<>();
+ List<List<RexNode>> finalExpressions = new ArrayList<>();
+ IgniteTable table;
+
+ List<List<RexNode>> prevProjects = new ArrayList<>();
+
+ ModifyNodeShuttle(IgniteTable table) {
+ this.table = table;
+ }
+
static {
allowRelTransfers.put(IgniteValues.class,
Set.of(IgniteProject.class, IgniteExchange.class, IgniteTrimExchange.class));
allowRelTransfers.put(IgniteProject.class,
Set.of(IgniteTableModify.class, IgniteUnionAll.class, IgniteExchange.class,
- IgniteTrimExchange.class));
+ IgniteTrimExchange.class, IgniteProject.class));
allowRelTransfers.put(IgniteUnionAll.class,
Set.of(IgniteProject.class, IgniteTableModify.class));
}
/** {@inheritDoc} */
@Override
public IgniteRel visit(IgniteProject rel) {
- if (!unionRaised) {
- // unexpected branch
- if (projections != null) {
- throw Util.FoundOne.NULL;
- }
- projections = rel.getProjects();
- } else {
- // projections after union
- if (expressions == null) {
- expressions = new ArrayList<>();
- }
- expressions.add(rel.getProjects());
- }
+ prevProjects.add(rel.getProjects());
return super.visit(rel);
}
/** {@inheritDoc} */
@Override
public IgniteRel visit(IgniteValues rel) {
- if (expressions == null) {
- expressions = new ArrayList<>();
- }
+ List<List<RexNode>> expressions = Commons.cast(rel.getTuples());
+
+ if (!prevProjects.isEmpty()) {
+ for (List<RexNode> prj : prevProjects) {
+ List<RexNode> prevProject = prj;
+ List<RexNode> projectionsReplaced =
replaceInputRefs(prevProject);
+
+ boolean refFound =
!projectionsReplaced.equals(prevProject);
+
+ prevProject = projectionsReplaced;
+
+ assert rel.getTuples() != null;
- List<List<RexNode>> values = Commons.cast(rel.getTuples());
+ for (List<RexNode> exp : expressions) {
+ if (!refFound) {
+ // no references are found, all important in
projections
+ finalExpressions.add(prevProject);
+ } else {
+ // otherwise exchange references with appropriate
representations
+ List<RexNode> modifiedExpressions = new
ArrayList<>(prevProject);
- expressions.addAll(values);
+ for (int i = 0; i < prevProject.size(); ++i) {
+ RexNode prjNode = prevProject.get(i);
+
+ if (prjNode instanceof RexLocalRef) {
+ RexLocalRef node0 = (RexLocalRef) prjNode;
+ modifiedExpressions.set(i,
exp.get(node0.getIndex()));
+ }
+ }
+
+ finalExpressions.add(modifiedExpressions);
+ }
+ }
+ }
+ } else if (expressions != null) {
+ finalExpressions.addAll(expressions);
+ }
+
+ prevProjects.clear();
return super.visit(rel);
}
/** {@inheritDoc} */
@Override
public IgniteRel visit(IgniteUnionAll rel) {
- unionRaised = true;
+ if (mapping != null) {
+ // unexpected multiple unions
+ throw Util.FoundOne.NULL;
+ }
+
+ for (List<RexNode> prj : prevProjects) {
+ if (mapping == null) {
+ mapping = new ArrayList<>(prevProjects.size());
+ }
+
+ mapping.add(RexUtils.inversePermutation(prj,
+ table.getRowType(Commons.typeFactory()), false));
+ }
+
+ prevProjects.clear();
Review Comment:
this doesn't look correct. What if we have tree like
`Project(UnionAll(Project, Project))`? We must preserve topmost project for all
branches of Union node
##########
modules/sql-engine/src/main/java/org/apache/ignite/internal/sql/engine/prepare/pruning/PartitionPruningMetadataExtractor.java:
##########
@@ -156,57 +156,99 @@ public IgniteRel visit(IgniteTableScan rel) {
}
private static class ModifyNodeShuttle extends IgniteRelShuttle {
- List<RexNode> projections = null;
- List<List<RexNode>> expressions = null;
- boolean unionRaised = false;
+ List<TargetMapping> mapping;
IgniteRel previous = null;
private static final Map<Class<?>, Set<Class<?>>> allowRelTransfers =
new HashMap<>();
+ List<List<RexNode>> finalExpressions = new ArrayList<>();
+ IgniteTable table;
+
+ List<List<RexNode>> prevProjects = new ArrayList<>();
+
+ ModifyNodeShuttle(IgniteTable table) {
+ this.table = table;
+ }
+
static {
allowRelTransfers.put(IgniteValues.class,
Set.of(IgniteProject.class, IgniteExchange.class, IgniteTrimExchange.class));
allowRelTransfers.put(IgniteProject.class,
Set.of(IgniteTableModify.class, IgniteUnionAll.class, IgniteExchange.class,
- IgniteTrimExchange.class));
+ IgniteTrimExchange.class, IgniteProject.class));
allowRelTransfers.put(IgniteUnionAll.class,
Set.of(IgniteProject.class, IgniteTableModify.class));
}
/** {@inheritDoc} */
@Override
public IgniteRel visit(IgniteProject rel) {
- if (!unionRaised) {
- // unexpected branch
- if (projections != null) {
- throw Util.FoundOne.NULL;
- }
- projections = rel.getProjects();
- } else {
- // projections after union
- if (expressions == null) {
- expressions = new ArrayList<>();
- }
- expressions.add(rel.getProjects());
- }
+ prevProjects.add(rel.getProjects());
return super.visit(rel);
}
/** {@inheritDoc} */
@Override
public IgniteRel visit(IgniteValues rel) {
- if (expressions == null) {
- expressions = new ArrayList<>();
- }
+ List<List<RexNode>> expressions = Commons.cast(rel.getTuples());
+
+ if (!prevProjects.isEmpty()) {
+ for (List<RexNode> prj : prevProjects) {
+ List<RexNode> prevProject = prj;
+ List<RexNode> projectionsReplaced =
replaceInputRefs(prevProject);
+
+ boolean refFound =
!projectionsReplaced.equals(prevProject);
+
+ prevProject = projectionsReplaced;
+
+ assert rel.getTuples() != null;
- List<List<RexNode>> values = Commons.cast(rel.getTuples());
+ for (List<RexNode> exp : expressions) {
+ if (!refFound) {
+ // no references are found, all important in
projections
+ finalExpressions.add(prevProject);
+ } else {
+ // otherwise exchange references with appropriate
representations
+ List<RexNode> modifiedExpressions = new
ArrayList<>(prevProject);
- expressions.addAll(values);
+ for (int i = 0; i < prevProject.size(); ++i) {
+ RexNode prjNode = prevProject.get(i);
+
+ if (prjNode instanceof RexLocalRef) {
+ RexLocalRef node0 = (RexLocalRef) prjNode;
+ modifiedExpressions.set(i,
exp.get(node0.getIndex()));
+ }
+ }
+
+ finalExpressions.add(modifiedExpressions);
+ }
+ }
+ }
+ } else if (expressions != null) {
+ finalExpressions.addAll(expressions);
+ }
+
+ prevProjects.clear();
return super.visit(rel);
}
/** {@inheritDoc} */
@Override
public IgniteRel visit(IgniteUnionAll rel) {
- unionRaised = true;
+ if (mapping != null) {
+ // unexpected multiple unions
+ throw Util.FoundOne.NULL;
+ }
+
+ for (List<RexNode> prj : prevProjects) {
+ if (mapping == null) {
+ mapping = new ArrayList<>(prevProjects.size());
+ }
+
+ mapping.add(RexUtils.inversePermutation(prj,
+ table.getRowType(Commons.typeFactory()), false));
+ }
+
+ prevProjects.clear();
+
return super.visit(rel);
}
Review Comment:
Why in method `visitChild` we are checking `previous` instead of `parent`?
```
protected void visitChild(IgniteRel parent, int i, IgniteRel child) {
if (child instanceof IgniteExchange || child instanceof
IgniteTrimExchange) {
super.visitChild(parent, i, child);
} else if (allowRelTransfers.getOrDefault(child.getClass(),
Set.of()).contains(previous.getClass())) {
super.visitChild(parent, i, child);
} else {
throw Util.FoundOne.NULL;
}
}
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
