sinhaparth5 commented on PR #67540: URL: https://github.com/apache/doris/pull/67540#issuecomment-5567879390
Thanks for the review, addressed four of the five points: **CTE definitions not traversed** — fixed. `Plan.foreach()` only recurses `children()`, and a CTE's WITH-list lives in `extraPlans()`, so `EXISTS (WITH x AS (...) SELECT 1 FROM x)` was invisible to the walk. The walk now recurses into `extraPlans()` too. Added `testCorrelatedCteRejected`. **Nested-field / non-slot forms escaping detection** — fixed both. Matching on the last name part meant `main_table.payload.ref_id` only compared "ref_id" against the table's columns, missing correlation through struct fields. The check is now qualifier-based: it looks at every name part before the last one and flags correlation if any of them equals the outer table's name, so it no longer depends on what the reference resolves to. Also added explicit handling for `UnboundStar`'s `exceptedSlots`/`replacedAlias`, since it's a leaf and its REPLACE/EXCEPT expressions were never visited either. **False positive on same-named local columns** — fixed, same qualifier-based change covers this. `EXISTS (SELECT 1 FROM lookup_table l WHERE l.ref_id = 10)` was being rejected because "l.ref_id" reduced to bare "ref_id", which collides with `main_table`'s own column, even though "l" doesn't refer to `main_table` at all. You're right that this also meant the original correlated-rejection test didn't prove what it claimed: it would throw on the local `l.ref_id` by itself, before ever reaching the actual correlated `main_table.ref_id`. Added `testSameNameLocalColumnNotFalsePositive` to lock in the fix. **[P2] Wrong exception type** — fixed. The rejection now gets converted from the unchecked Nereids `AnalysisException` to the checked `common.AnalysisException` that `validate()` declares, via a try/catch around the predicate walk, so `StmtExecutor` takes the normal `UserException` path instead of logging routine invalid input as an unexpected bug. **Persisted and replayed policy state** — not addressed in this round, on purpose. `RowPolicy.gsonPostProcess`, image load, and edit-log replay all still bypass this check, so a policy already on disk before this fix, or one written by an old leader mid rolling-upgrade, stays unvalidated. I didn't bolt validation onto the replay path because the failure mode is different and worse there: an existing policy that fails the new check at FE startup could take the FE down, rather than just rejecting one new CREATE ROW POLICY statement. That needs a real decision (fail-closed and refuse to load vs. log and skip the policy vs. something else), not a quick addition here, so I'm leaving it as follow-up work rather than rushing a fix that could break existing clusters. Verified with a full build of fe-common and fe-core (~4473 main source files, 1473 test source files) and all 5 `CreatePolicyCommandTest` cases passing, including the two new regression tests. -- 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]
