github-actions[bot] commented on code in PR #65548:
URL: https://github.com/apache/doris/pull/65548#discussion_r3578213096
##########
be/src/format_v2/table_reader.h:
##########
@@ -949,7 +952,19 @@ class TableReader {
return false;
}
if (agg_type == TPushAggOp::type::COUNT) {
- return true;
+ // COUNT(*) needs no column metadata. COUNT(col) currently
supports one direct file
+ // column; multiple COUNT arguments fall back to the normal scan
so every upper
+ // aggregate receives the original rows.
+ if (_push_down_count_columns.empty()) {
+ return true;
+ }
+ if (_push_down_count_columns.size() != 1) {
+ return false;
+ }
+ const auto& mapping = _push_down_count_mapping();
+ return mapping.file_local_id.has_value() && mapping.file_type !=
nullptr &&
Review Comment:
This still allows COUNT(col) pushdown for non-trivial direct mappings. The
normal scan path does more than read the file-local slot: for
`!mapping.is_trivial`, `_materialize_present_child_mapping_column()` casts from
`mapping.file_type` to `mapping.table_type` and then
`_align_column_nullability()` rejects nullable physical data when the table
column is non-null. This path skips both checks and lets Parquet/ORC answer
from file metadata/definition levels. For an evolved external table where a
non-null table column maps to a nullable physical column, or where a cast is
required, COUNT(non_null_col) can return a metadata count instead of falling
back to the scan that validates the table contract. Please require the counted
mapping to be count-safe, at least `mapping.is_trivial` for direct mappings as
MIN/MAX already does, or add a narrower whitelist with tests for nullable
physical data mapped to non-null table columns.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/glue/translator/PhysicalPlanTranslator.java:
##########
@@ -1401,6 +1406,10 @@ public PlanFragment visitPhysicalStorageLayerAggregate(
context.setRelationPushAggOp(
storageLayerAggregate.getRelation().getRelationId(),
pushAggOp);
+ context.setRelationPushCountArgumentExprIds(
Review Comment:
This new COUNT-argument field needs a rolling-upgrade guard. It is optional
thrift, but the planner still enables external COUNT pushdown whenever the
session variable allows it. During a mixed-version upgrade, a new FE can send
`push_down_count_slot_ids` to an old BE that ignores field 55 and runs the
pre-fix COUNT inference, while an old FE can send `COUNT` to a new BE with the
field absent and the new BE treats the empty vector as explicit COUNT(*)
semantics. That reopens the COUNT(*)/COUNT(col) ambiguity this PR is fixing.
Please disable external COUNT pushdown unless all target BEs understand field
55, or otherwise make the mixed-version path fall back instead of interpreting
absent and explicitly-empty the same way.
--
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]
