github-actions[bot] commented on code in PR #67540:
URL: https://github.com/apache/doris/pull/67540#discussion_r3947472154
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreatePolicyCommand.java:
##########
@@ -159,12 +162,56 @@ private void validate(ConnectContext ctx) throws
AnalysisException {
throw new
org.apache.doris.nereids.exceptions.AnalysisException(
"column not exist: " + slot.getName());
}
+ } else if (expr instanceof SubqueryExpr) {
+ // Exists/InSubquery/ScalarSubquery are leaf
expressions: their subquery
+ // plan isn't a child in the expression tree, so the
foreach above never
+ // looks inside it. A subquery that reaches back into
tableIf (the row
+ // policy's own table) is a correlated subquery, and
this command has no
+ // way to keep that reference resolvable once the
policy is stored and
+ // re-parsed at query time, so reject it here instead
of silently
+ // dropping the policy later.
+ rejectCorrelatedSubquery((SubqueryExpr) expr, tableIf);
Review Comment:
[P1] Cover persisted and mixed-version policy state as well as live DDL.
`RowPolicy.gsonPostProcess` reparses `originStmt` without this validation,
`PolicyMgr.read` then installs every image policy through
`updateTablePolicies`, and edit-log replay goes directly through `replayCreate
-> unprotectedAdd`. Therefore an existing correlated policy, or one accepted
and journaled by an old leader during rolling upgrade, remains active after
restart/failover and preserves the silent row-policy failure this PR is meant
to close. Please define a fail-closed, observable behavior for
image/replay/consumption and test old-record and mixed-version paths.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreatePolicyCommand.java:
##########
@@ -159,12 +162,56 @@ private void validate(ConnectContext ctx) throws
AnalysisException {
throw new
org.apache.doris.nereids.exceptions.AnalysisException(
"column not exist: " + slot.getName());
}
+ } else if (expr instanceof SubqueryExpr) {
+ // Exists/InSubquery/ScalarSubquery are leaf
expressions: their subquery
+ // plan isn't a child in the expression tree, so the
foreach above never
+ // looks inside it. A subquery that reaches back into
tableIf (the row
+ // policy's own table) is a correlated subquery, and
this command has no
+ // way to keep that reference resolvable once the
policy is stored and
+ // re-parsed at query time, so reject it here instead
of silently
+ // dropping the policy later.
+ rejectCorrelatedSubquery((SubqueryExpr) expr, tableIf);
}
});
}
}
+ // ponytail: name-based, not a real bind. A subquery column that merely
shares a name with
+ // an outer-table column gets rejected even when the subquery has its own
local relation
+ // that would actually resolve it (e.g. a self-join on the policy's own
table). That's the
+ // deliberate trade-off of Option A from apache/doris#62729: false
rejections are safe here,
+ // silently dropping a policy is not. Upgrade to real correlation
detection (Option B) if
+ // this starts blocking legitimate policies.
+ private static void rejectCorrelatedSubquery(SubqueryExpr subquery,
TableIf outerTable) {
+ subquery.getQueryPlan().foreach(node -> {
+ for (Expression expr : ((Plan) node).getExpressions()) {
+ checkNoOuterReference(expr, outerTable);
+ }
+ });
+ }
+
+ private static void checkNoOuterReference(Expression expr, TableIf
outerTable) {
+ if (expr instanceof UnboundSlot) {
+ // Use the last name part, not getName(): for a qualified
reference like
+ // "main_table.ref_id", getName() returns the whole dotted string,
which would
+ // never match a bare column name.
+ List<String> nameParts = ((UnboundSlot) expr).getNameParts();
Review Comment:
[P1] Do not infer the outer column from the final name token. For a target
column `payload STRUCT<ref_id:INT>`, the correlated reference
`main_table.payload.ref_id` has name parts `[main_table, payload, ref_id]`, so
this looks up top-level `ref_id` and accepts the policy when only `payload`
exists. Other raw carriers are missed altogether: `UnboundStar` is a leaf and
keeps `REPLACE` expressions outside `children()`, so `SELECT l.*
REPLACE(main_table.ref_id AS ref_id)` also escapes this walk. These forms
preserve the fail-open behavior being fixed. Please classify correlation after
scope-aware binding and add nested-field/non-slot coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreatePolicyCommand.java:
##########
@@ -159,12 +162,56 @@ private void validate(ConnectContext ctx) throws
AnalysisException {
throw new
org.apache.doris.nereids.exceptions.AnalysisException(
"column not exist: " + slot.getName());
}
+ } else if (expr instanceof SubqueryExpr) {
+ // Exists/InSubquery/ScalarSubquery are leaf
expressions: their subquery
+ // plan isn't a child in the expression tree, so the
foreach above never
+ // looks inside it. A subquery that reaches back into
tableIf (the row
+ // policy's own table) is a correlated subquery, and
this command has no
+ // way to keep that reference resolvable once the
policy is stored and
+ // re-parsed at query time, so reject it here instead
of silently
+ // dropping the policy later.
+ rejectCorrelatedSubquery((SubqueryExpr) expr, tableIf);
}
});
}
}
+ // ponytail: name-based, not a real bind. A subquery column that merely
shares a name with
+ // an outer-table column gets rejected even when the subquery has its own
local relation
+ // that would actually resolve it (e.g. a self-join on the policy's own
table). That's the
+ // deliberate trade-off of Option A from apache/doris#62729: false
rejections are safe here,
+ // silently dropping a policy is not. Upgrade to real correlation
detection (Option B) if
+ // this starts blocking legitimate policies.
+ private static void rejectCorrelatedSubquery(SubqueryExpr subquery,
TableIf outerTable) {
+ subquery.getQueryPlan().foreach(node -> {
+ for (Expression expr : ((Plan) node).getExpressions()) {
+ checkNoOuterReference(expr, outerTable);
+ }
+ });
+ }
+
+ private static void checkNoOuterReference(Expression expr, TableIf
outerTable) {
+ if (expr instanceof UnboundSlot) {
+ // Use the last name part, not getName(): for a qualified
reference like
+ // "main_table.ref_id", getName() returns the whole dotted string,
which would
+ // never match a bare column name.
+ List<String> nameParts = ((UnboundSlot) expr).getNameParts();
+ String columnName = nameParts.get(nameParts.size() - 1);
+ if (outerTable.getColumn(columnName) != null) {
+ throw new
org.apache.doris.nereids.exceptions.AnalysisException(
Review Comment:
[P2] Return this expected validation failure through the declared user-error
path. This throws `org.apache.doris.nereids.exceptions.AnalysisException` (a
`RuntimeException`), while `validate` declares
`org.apache.doris.common.AnalysisException` (a `UserException`). `StmtExecutor`
therefore handles every new rejection in its generic `Maybe our bug` branch and
emits an INFO stack trace for routine invalid input; direct callers also cannot
rely on the public method's declared exception contract. Please surface the
common `AnalysisException` outside the traversal callback and exercise the real
command path in the test.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreatePolicyCommand.java:
##########
@@ -159,12 +162,56 @@ private void validate(ConnectContext ctx) throws
AnalysisException {
throw new
org.apache.doris.nereids.exceptions.AnalysisException(
"column not exist: " + slot.getName());
}
+ } else if (expr instanceof SubqueryExpr) {
+ // Exists/InSubquery/ScalarSubquery are leaf
expressions: their subquery
+ // plan isn't a child in the expression tree, so the
foreach above never
+ // looks inside it. A subquery that reaches back into
tableIf (the row
+ // policy's own table) is a correlated subquery, and
this command has no
+ // way to keep that reference resolvable once the
policy is stored and
+ // re-parsed at query time, so reject it here instead
of silently
+ // dropping the policy later.
+ rejectCorrelatedSubquery((SubqueryExpr) expr, tableIf);
}
});
}
}
+ // ponytail: name-based, not a real bind. A subquery column that merely
shares a name with
+ // an outer-table column gets rejected even when the subquery has its own
local relation
+ // that would actually resolve it (e.g. a self-join on the policy's own
table). That's the
+ // deliberate trade-off of Option A from apache/doris#62729: false
rejections are safe here,
+ // silently dropping a policy is not. Upgrade to real correlation
detection (Option B) if
+ // this starts blocking legitimate policies.
+ private static void rejectCorrelatedSubquery(SubqueryExpr subquery,
TableIf outerTable) {
+ subquery.getQueryPlan().foreach(node -> {
Review Comment:
[P1] Traverse CTE definitions, not only ordinary plan children. `LogicalCTE`
stores `WITH` definitions in `aliasQueries` exposed through `extraPlans()`,
while `TreeNode.foreach` recurses only through `children()`. Thus `EXISTS (WITH
x AS (SELECT 1 FROM lookup_table l WHERE l.ref_id = main_table.ref_id) SELECT 1
FROM x)` never visits the correlated slot and the unsupported policy is still
stored. Please cover syntax-bearing extra plans (or use analyzer-driven
correlation detection) and add a `WITH`-clause regression.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/CreatePolicyCommand.java:
##########
@@ -159,12 +162,56 @@ private void validate(ConnectContext ctx) throws
AnalysisException {
throw new
org.apache.doris.nereids.exceptions.AnalysisException(
"column not exist: " + slot.getName());
}
+ } else if (expr instanceof SubqueryExpr) {
+ // Exists/InSubquery/ScalarSubquery are leaf
expressions: their subquery
+ // plan isn't a child in the expression tree, so the
foreach above never
+ // looks inside it. A subquery that reaches back into
tableIf (the row
+ // policy's own table) is a correlated subquery, and
this command has no
+ // way to keep that reference resolvable once the
policy is stored and
+ // re-parsed at query time, so reject it here instead
of silently
+ // dropping the policy later.
+ rejectCorrelatedSubquery((SubqueryExpr) expr, tableIf);
}
});
}
}
+ // ponytail: name-based, not a real bind. A subquery column that merely
shares a name with
+ // an outer-table column gets rejected even when the subquery has its own
local relation
+ // that would actually resolve it (e.g. a self-join on the policy's own
table). That's the
+ // deliberate trade-off of Option A from apache/doris#62729: false
rejections are safe here,
+ // silently dropping a policy is not. Upgrade to real correlation
detection (Option B) if
+ // this starts blocking legitimate policies.
+ private static void rejectCorrelatedSubquery(SubqueryExpr subquery,
TableIf outerTable) {
+ subquery.getQueryPlan().foreach(node -> {
+ for (Expression expr : ((Plan) node).getExpressions()) {
+ checkNoOuterReference(expr, outerTable);
+ }
+ });
+ }
+
+ private static void checkNoOuterReference(Expression expr, TableIf
outerTable) {
+ if (expr instanceof UnboundSlot) {
+ // Use the last name part, not getName(): for a qualified
reference like
+ // "main_table.ref_id", getName() returns the whole dotted string,
which would
+ // never match a bare column name.
+ List<String> nameParts = ((UnboundSlot) expr).getNameParts();
+ String columnName = nameParts.get(nameParts.size() - 1);
+ if (outerTable.getColumn(columnName) != null) {
Review Comment:
[P1] Resolve inner slots before classifying correlation. With the schemas in
this test, a valid uncorrelated predicate such as `EXISTS (SELECT 1 FROM
lookup_table l WHERE l.ref_id = 10)` is rejected here: `l.ref_id` has name
parts `[l, ref_id]`, this drops `l`, and `main_table` also has `ref_id`. Normal
analysis binds the qualified slot in the inner scope first, so it is not
correlated. The new positive test avoids this collision, and the negative test
would already throw on its local `l.ref_id` even without `main_table.ref_id`.
Please use the analyzer's actual correlated-slot result and add a same-name
local-column case.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
