yujun777 commented on PR #65657:
URL: https://github.com/apache/doris/pull/65657#issuecomment-5042955359
> Request changes: the existing partition-filter propagation blocker remains
unresolved at head `72b7474d41f01a654d940c6737b51a43d6fa12f8`.
>
> The local builder change is correct: it aligns
`LogicalOlapTableStreamScan.withSelectedPartitionIds(List<Long>, boolean)` with
the parent contract, marks both boolean paths as already pruned, and stops the
repeated `PruneOlapScanPartition` rewrite. However,
`NormalizeOlapTableStreamScan` still lowers the stream into fresh base/binlog
scans by putting internally selected partition IDs into `specifiedPartitions`.
Recursive top-down pruning then classifies those IDs as a manual partition
filter and sets every executable child of a partitioned stream to
`hasPartitionPredicate=true`. An unfiltered partitioned stream query can
therefore still bypass a SQL block rule with `require_partition_filter=true`.
This is the same blocker already anchored in the existing inline thread, which
GitHub currently marks resolved even though the code path remains; I did not
add a duplicate inline comment.
>
> Critical checkpoint conclusions:
>
> * Goal and correctness: the optimizer-loop root cause is fixed locally,
but downstream execution semantics remain incorrect because the
partition-filter flag is not preserved through normalization.
> * Scope and clarity: the two-file patch is small and the production change
follows the parent builder contract.
> * Parallel paths: incremental, snapshot, and reset lowering all create
affected base/binlog scans; both filtered and unfiltered paths were traced
through logical implementation, physical translation, `StmtExecutor`, and
`SqlBlockRuleMgr`.
> * Test coverage and results: the new JUnit test deterministically catches
the old direct false-value builder behavior, but it stops after analysis and
does not exercise the real rewrite loop, the true-value path, normalization,
partial empty-partition pruning, final `OlapScanNode` state, or block-rule
enforcement. Current PR checks report FE UT, compile, CheckStyle, and
regression jobs passing, but they do not disprove these uncovered state
transitions. No local build or test was run because this is a review-only
runner and the local third-party/protoc dependencies are absent.
> * Performance: the change removes the unbounded rewrite/allocation loop
and adds no hot-path overhead.
> * Concurrency, lifecycle, configuration, persistence,
transaction/data-write, storage compatibility, FE-BE protocol, memory safety,
and observability: not involved in this patch; no issue found for these
checkpoints.
> * User focus: no additional focus was provided; the full PR and its
downstream partition-filter behavior were reviewed.
>
> Please preserve the already-computed `hasPartitionPredicate` value through
both partial empty-partition selection and normalized-child construction. Add
end-to-end filtered/unfiltered assertions, including a filtered case where only
some selected partitions are empty, before merging.
The requested changes are outside the scope of this PR.
This patch only fixes the partition-pruned state propagation and aligns
`LogicalOlapTableStreamScan.withSelectedPartitionIds()` with the
parent contract, eliminating the repeated `PruneOlapScanPartition` rewrite.
`NormalizeOlapTableStreamScan` lowering streams into executable
base/binlog `LogicalOlapScan` children with internally selected
partitions is existing behavior and is unchanged by this PR. Whether those
internal selections should have different
`require_partition_filter` semantics is a separate issue that needs an
end-to-end reproduction and a dedicated PR; it should not block
this state-propagation fix.
--
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]
