Skip to content

Commit e2b7cf6

Browse files
committed
Check what a flat_object allows while the plan is analyzed, as a list of allowed uses
The check walked the finished plan and rejected every node that was not a read. Nothing else in this package works that way -- MapPathPreMaterializer, SchemaUnifier, ForeachPlanner and QualifiedNameResolver are all helpers the visitor calls while building -- and a plan pass pays for it: it has to trace a column back through a projection to find the leaf it came from, and rebuild "attributes.duration_ms" out of a generated $f2, because Calcite renames columns and rebuilds expressions. The rule is now a list of the uses a flat_object allows, checked where a field reference resolves. Every strategy in QualifiedNameResolver converges on one point, so a command not on the list is refused without an entry of its own -- including one added later -- and the name in the message is the one the query wrote. Reading a field or a leaf, optionally cast, and typeof are the list; a column a read went into is remembered so a later command using it is refused too, named by the leaf rather than by the alias. Two things had to be arranged around this. The materializer resolves fields inside catch (RuntimeException | AssertionError), so it runs with the read permission open rather than having a refusal swallowed there and the field quietly ignored. And expand already refuses anything that is not a plain column, in its own words, so its resolution is a read and that check decides. Behaviour is unchanged over 22 queries covering every allowed and refused form, with one message corrected: dedup said "Cannot filter by", the name of what it lowers to, and now says "Cannot dedup by". Signed-off-by: Jialiang Liang <jiallian@amazon.com>
1 parent ba80882 commit e2b7cf6

7 files changed

Lines changed: 502 additions & 548 deletions

File tree

‎core/src/main/java/org/opensearch/sql/calcite/CalcitePlanContext.java‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010
import java.sql.Connection;
1111
import java.util.ArrayList;
1212
import java.util.HashMap;
13+
import java.util.LinkedHashMap;
1314
import java.util.LinkedHashSet;
1415
import java.util.List;
1516
import java.util.Map;
@@ -106,6 +107,19 @@ public class CalcitePlanContext {
106107
*/
107108
@Getter private final Set<String> flatObjectFields = new LinkedHashSet<>();
108109

110+
/**
111+
* Columns a command read a {@code flat_object} field or one of its leaves into, by the name the
112+
* command gave them. A later command using such a column is refused the same way, since pushdown
113+
* fuses the projection back into whatever reads it.
114+
*/
115+
@Getter private final Map<String, String> flatObjectLeafColumns = new LinkedHashMap<>();
116+
117+
/** Whether the expression being resolved is one of the reads a flat_object field allows. */
118+
@Getter @Setter private boolean flatObjectReadAllowed = false;
119+
120+
/** What the command being analyzed would do with a flat_object field, for the refusal message. */
121+
@Getter @Setter private String flatObjectUse = "use";
122+
109123
private final Stack<RexCorrelVariable> correlVar = new Stack<>();
110124
private final Stack<List<RexNode>> windowPartitions = new Stack<>();
111125

‎core/src/main/java/org/opensearch/sql/calcite/CalciteRelNodeVisitor.java‎

Lines changed: 47 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -181,6 +181,7 @@
181181
import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit;
182182
import org.opensearch.sql.calcite.plan.rel.LogicalSystemLimit.SystemLimitType;
183183
import org.opensearch.sql.calcite.utils.BinUtils;
184+
import org.opensearch.sql.calcite.utils.FlatObjectScope;
184185
import org.opensearch.sql.calcite.utils.JoinAndLookupUtils;
185186
import org.opensearch.sql.calcite.utils.OpenSearchTypeFactory;
186187
import org.opensearch.sql.calcite.utils.PPLHintUtils;
@@ -240,15 +241,18 @@ public RelNode analyze(UnresolvedPlan unresolved, CalcitePlanContext context) {
240241
RelNode plan = unresolved.accept(this, context);
241242
// Here rather than in QueryService.analyze, so the unified query pipeline -- which calls this
242243
// method directly -- is held to the same rules.
243-
FlatObjectScopeValidator.validate(plan, context.getFlatObjectFields());
244244
return plan;
245245
}
246246

247247
@Override
248248
public RelNode visitChildren(Node node, CalcitePlanContext context) {
249249
RelNode result = super.visitChildren(node, context);
250250
if (node instanceof UnresolvedPlan plan) {
251-
mapPathMaterializer.materializePaths(plan, context);
251+
// Materializing a dotted path is a read, so it is allowed -- and it has to be, because the
252+
// materializer wraps field resolution in catch (RuntimeException | AssertionError), which
253+
// would swallow a refusal raised there and leave the command quietly ignoring the field.
254+
// What the command then does with the materialized column is checked where it resolves it.
255+
FlatObjectScope.runAsRead(context, () -> mapPathMaterializer.materializePaths(plan, context));
252256
}
253257
return result;
254258
}
@@ -369,7 +373,9 @@ public RelNode visitFilter(Filter node, CalcitePlanContext context) {
369373
context.relBuilder.variable(v::set);
370374
context.pushCorrelVar(v.get());
371375
}
372-
RexNode condition = rexVisitor.analyze(node.getCondition(), context);
376+
RexNode condition =
377+
FlatObjectScope.as(
378+
"filter by", context, () -> rexVisitor.analyze(node.getCondition(), context));
373379
if (containsSubqueryExpression) {
374380
context.relBuilder.filter(ImmutableList.of(v.get().id), condition);
375381
context.popCorrelVar();
@@ -612,7 +618,9 @@ private List<RexNode> expandProjectFields(
612618
}
613619
matchingFields.forEach(f -> expandedFields.add(context.relBuilder.field(f)));
614620
} else if (addedFields.add(fieldName)) {
615-
RexNode resolved = rexVisitor.analyze(field, context);
621+
RexNode resolved =
622+
FlatObjectScope.allowRead(
623+
field, fieldName, context, () -> rexVisitor.analyze(field, context));
616624
/*
617625
* Dotted path access is resolved into ITEM(map, path) function call without aliasing.
618626
* Re-apply the alias so the projected column retains the user-visible name.
@@ -801,6 +809,7 @@ public RelNode visitRename(Rename node, CalcitePlanContext context) {
801809
String newName =
802810
WildcardRenameUtils.applyWildcardTransformation(
803811
sourcePattern, targetPattern, fieldName);
812+
FlatObjectScope.trackRename(fieldName, newName, context);
804813
if (newNames.contains(newName) && !newName.equals(fieldName)) {
805814
removeFieldIfExists(newName, newNames, context);
806815
}
@@ -829,23 +838,27 @@ private void removeFieldIfExists(
829838
public RelNode visitSort(org.opensearch.sql.ast.tree.Sort node, CalcitePlanContext context) {
830839
visitChildren(node, context);
831840
List<RexNode> sortList =
832-
node.getSortList().stream()
833-
.map(
834-
expr -> {
835-
RexNode sortField = rexVisitor.analyze(expr, context);
836-
SortOption sortOption = analyzeSortOption(expr.getFieldArgs());
837-
// Default is ASC
838-
if (sortOption.getSortOrder() == DESC) {
839-
sortField = context.relBuilder.desc(sortField);
840-
}
841-
if (sortOption.getNullOrder() == NULL_LAST) {
842-
sortField = context.relBuilder.nullsLast(sortField);
843-
} else {
844-
sortField = context.relBuilder.nullsFirst(sortField);
845-
}
846-
return sortField;
847-
})
848-
.collect(Collectors.toList());
841+
FlatObjectScope.as(
842+
"sort by",
843+
context,
844+
() ->
845+
node.getSortList().stream()
846+
.map(
847+
expr -> {
848+
RexNode sortField = rexVisitor.analyze(expr, context);
849+
SortOption sortOption = analyzeSortOption(expr.getFieldArgs());
850+
// Default is ASC
851+
if (sortOption.getSortOrder() == DESC) {
852+
sortField = context.relBuilder.desc(sortField);
853+
}
854+
if (sortOption.getNullOrder() == NULL_LAST) {
855+
sortField = context.relBuilder.nullsLast(sortField);
856+
} else {
857+
sortField = context.relBuilder.nullsFirst(sortField);
858+
}
859+
return sortField;
860+
})
861+
.collect(Collectors.toList()));
849862
context.relBuilder.sort(sortList);
850863
// Apply count parameter as limit
851864
if (node.getCount() != 0) {
@@ -1292,7 +1305,12 @@ public RelNode visitEval(Eval node, CalcitePlanContext context) {
12921305
context.relBuilder.variable(v::set);
12931306
context.pushCorrelVar(v.get());
12941307
}
1295-
RexNode eval = rexVisitor.analyze(expr, context);
1308+
RexNode eval =
1309+
FlatObjectScope.allowRead(
1310+
expr,
1311+
expr.getVar().getField().toString(),
1312+
context,
1313+
() -> rexVisitor.analyze(expr, context));
12961314
if (containsSubqueryExpression) {
12971315
// RelBuilder.projectPlus doesn't have a parameter with variablesSet:
12981316
// projectPlus(Iterable<CorrelationId> variablesSet, RexNode... nodes)
@@ -1802,7 +1820,8 @@ public RelNode visitAggregation(Aggregation node, CalcitePlanContext context) {
18021820
if (!bucketNullable) {
18031821
nonNullGroupMask.set(0, nGroup);
18041822
}
1805-
visitAggregation(node, context, nonNullGroupMask, true, false);
1823+
FlatObjectScope.runAs(
1824+
"stats by", context, () -> visitAggregation(node, context, nonNullGroupMask, true, false));
18061825
return context.relBuilder.peek();
18071826
}
18081827

@@ -2273,6 +2292,7 @@ public RelNode visitLookup(Lookup node, CalcitePlanContext context) {
22732292

22742293
@Override
22752294
public RelNode visitDedupe(Dedupe node, CalcitePlanContext context) {
2295+
context.setFlatObjectUse("dedup by");
22762296
visitChildren(node, context);
22772297
List<Argument> options = node.getOptions();
22782298
Integer allowedDuplication = (Integer) options.get(0).getValue().getValue();
@@ -4484,7 +4504,10 @@ public RelNode visitExpand(Expand expand, CalcitePlanContext context) {
44844504

44854505
// 2. Get the field to expand and an optional alias.
44864506
Field arrayField = expand.getField();
4487-
RexNode resolved = rexVisitor.analyze(arrayField, context);
4507+
// Resolve as a read: the check below already refuses a flat_object leaf, by the same rule it
4508+
// uses for an object leaf or an expression, and says so in expand's own words.
4509+
RexNode resolved =
4510+
FlatObjectScope.allowReadOf(context, () -> rexVisitor.analyze(arrayField, context));
44884511
if (!(resolved instanceof RexInputRef arrayFieldRex)) {
44894512
// Anything that is not a column of the input -- a leaf of an object or a flat_object field,
44904513
// an expression -- has no column to correlate the expansion with. Raised the way the

0 commit comments

Comments
 (0)