Copilot commented on code in PR #61439:
URL: https://github.com/apache/doris/pull/61439#discussion_r2946510781
##########
be/src/storage/iterator/vgeneric_iterators.cpp:
##########
@@ -100,33 +76,6 @@ Status VStatisticsIterator::next_batch(Block* block) {
for (auto& column : columns) {
Review Comment:
After removing the `COUNT_NULL` path, an unexpected/unknown
`push_down_agg_type_opt` value (e.g., serialized by an older node during
rolling upgrade) will now fall into the generic zone-map branch in
`next_batch()`, which can silently produce incorrect results. Add an explicit
validation/switch for supported values (COUNT/MINMAX/MIX) and fail fast or
downgrade to a safe behavior instead of treating unknown values as the zone-map
path.
##########
gensrc/thrift/PlanNodes.thrift:
##########
@@ -861,8 +861,7 @@ enum TPushAggOp {
MINMAX = 1,
COUNT = 2,
MIX = 3,
- COUNT_ON_INDEX = 4,
- COUNT_NULL = 5
+ COUNT_ON_INDEX = 4
Review Comment:
`TPushAggOp` is a Thrift enum used across FE/BE. Removing `COUNT_NULL = 5`
from the enum can break rolling upgrades or mixed-version deployments: older
nodes may still serialize value 5, which newer code will treat as an unknown
enum value. Consider keeping `COUNT_NULL` in the enum as a deprecated/reserved
value (without using it), and explicitly mapping it to safe behavior (e.g.,
treat as `NONE` / no pushdown) to preserve wire compatibility.
--
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]
