Gabriel39 commented on PR #68726:
URL: https://github.com/apache/doris/pull/68726#issuecomment-6051350242
Review notes for `6ab06fceef96b3fe5c670344d4481b73ee4587bf`: the main
concern is the scope of the new single-instance rule. The points below identify
code-level boundaries and missing validation; I have not reproduced a
wrong-result bug or measured a performance regression.
1. **One assigned scan-range entry does not necessarily mean one physical
split.**
The new rule in
[`UnassignedScanSingleRemoteTableJob.degreeOfParallelism()`](https://github.com/apache/doris/blob/6ab06fceef96b3fe5c670344d4481b73ee4587bf/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/distribute/worker/job/UnassignedScanSingleRemoteTableJob.java#L80)
checks only `COUNT && maxParallel == 1`. However,
[`DefaultScanSource.maxParallel()`](https://github.com/apache/doris/blob/6ab06fceef96b3fe5c670344d4481b73ee4587bf/fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/distribute/worker/job/DefaultScanSource.java#L46)
counts scan-range entries. In [batch
mode](https://github.com/apache/doris/blob/6ab06fceef96b3fe5c670344d4481b73ee4587bf/fe/fe-core/src/main/java/org/apache/doris/datasource/scan/FileQueryScanNode.java#L415),
each BE receives a `split_source` entry that can supply many physical splits.
A COUNT-marked scan using that representation can therefore satisfy this
condition even with substantial scan work. This reduces fragment-instanc
e parallelism; it does not necessarily serialize all scanner threads, so the
throughput impact needs measurement.
Please exclude dynamic `split_source` ranges from this shortcut, or
explicitly validate that path. If the intent is a single-file-split
optimization, check the actual assigned split representation rather than only
the number of range envelopes. Add a test where one range entry represents
multiple fetched splits.
2. **The single-instance rule is broader than the new TVF row-count
optimization.**
This is a shared external-scan job, and the condition does not require a
file TVF or distinguish `COUNT(*)`/`COUNT(1)` from existing `COUNT(col)`
pushdown paths. Consequently, the change can also affect existing
external-table plans outside the newly added TVF matcher.
The accompanying comment that the scanner already produces the complete
scalar result is also too strong: [CSV COUNT still reads
records](https://github.com/apache/doris/blob/6ab06fceef96b3fe5c670344d4481b73ee4587bf/be/src/format/csv/csv_reader.cpp#L427),
while
[`CountReader`](https://github.com/apache/doris/blob/6ab06fceef96b3fe5c670344d4481b73ee4587bf/be/src/format/count_reader.h#L53)
emits batches representing rows for the upper aggregation, rather than a
single final count row.
Please initially scope this shortcut to the intended file TVF row-count
plans with one actual assigned split. If broader external-table coverage is
intentional, document it and validate those paths separately, including COUNT
arguments and batch scans. Update the comment to describe the actual reduction
in instance overhead.
3. **Add execution-level coverage for the expanded COUNT path.**
The added end-to-end regression covers gzip CSV COUNT, while the
translator tests primarily exercise mocked plan matching. Please supplement
them with pushdown-on/off result comparisons for representative CSV, Parquet,
ORC, and JSON inputs, under supported scanner V1/V2 paths. Include empty
inputs, multiple files/splits, CSV headers/empty records, and real SQL plans
with filters, grouping, DISTINCT, and `COUNT(nullable_col)` to verify the new
TVF matcher does not apply incorrectly. For the parallelism change, include a
large batch scan and verify both instance counts and throughput against the
baseline.
I did not identify a concrete blocking issue in preserving the initialized
round-robin candidate order. The existing behavior where cache/consistent-hash
scheduling can keep a hot split on the same BE is unchanged and is not a
regression introduced by this PR.
--
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]
