github-actions[bot] commented on code in PR #65548:
URL: https://github.com/apache/doris/pull/65548#discussion_r3578952070
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/implementation/AggregateStrategies.java:
##########
@@ -690,6 +691,12 @@ private LogicalAggregate<? extends Plan>
storageLayerAggregate(
List<SlotReference> usedSlotInTable = (List<SlotReference>)
Project.findProject(aggUsedSlots,
logicalScan.getOutput());
+ // COUNT(*) has no aggregate arguments, even though later column
pruning retains one
+ // arbitrary scan slot. Preserve the semantic arguments here so the BE
never needs to infer
+ // COUNT(col) from the post-pruning scan shape.
+ List<ExprId> countArgumentExprIds = mergeOp == PushDownAggOp.COUNT
Review Comment:
This still loses `COUNT(CAST(slot))` semantics. The rule allows numeric
`Cast(SlotReference)` arguments, but the new COUNT argument list is built by
collecting only nested `SlotReference`s, and `PhysicalPlanTranslator`
serializes only those slot IDs. For a query like
`COUNT(CAST(bigint_not_null_col AS TINYINT))`, BE sees
`COUNT(bigint_not_null_col)` and can answer from metadata even though the cast
itself can produce NULL in non-strict mode or raise a strict-cast overflow
error. Please either reject cast arguments for storage-layer COUNT pushdown, or
only allow casts that are proven null-preserving/non-throwing and carry that
proof through to BE.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -384,6 +390,20 @@ Status FileScannerV2::_get_block_impl(RuntimeState* state,
Block* block, bool* e
*eof = false;
continue;
}
+ if (_should_skip_empty(status, _should_stop ||
_io_ctx->should_stop)) {
Review Comment:
This generic EOF skip still lets one Native corruption path be counted as an
empty split. `NativeReader::_read_next_pblock()` returns `eof=true` when
reading the next block length yields `bytes_read == 0`, even though
`_current_offset < _file_size`; `_ensure_schema_loaded()` then reports
`EndOfFile("empty native file")`, and this branch aborts the split and
increments `EmptyFileNum`. Remote readers such as HDFS can return a zero-byte
read from `hdfsPread` before the cached file size is reached, so a
stale/truncated Native file can be silently skipped. Please make the zero-byte
length-prefix read a hard error unless `_current_offset >= _file_size` was
already true, or use a dedicated empty-file status that malformed short reads
cannot produce.
##########
be/src/exec/operator/scan_operator.cpp:
##########
@@ -1231,6 +1237,9 @@ Status ScanOperatorX<LocalStateType>::init(const
TPlanNode& tnode, RuntimeState*
} else {
_push_down_agg_type = TPushAggOp::type::NONE;
}
+ if (tnode.__isset.push_down_count_slot_ids) {
Review Comment:
This stores the new semantic COUNT argument field, but the current-head V1
`FileScanner` path never consumes it before applying the old row-count
shortcut. `FileScanLocalState` can still select V1 when
`enable_file_scanner_v2=false` or V2 is unsupported; V1 only passes
`push_down_agg_type` in `_fill_base_init_context()`, then wraps `CountReader`
for any `TPushAggOp::COUNT` with `table_level_row_count` or a Parquet/ORC
`supports_count_pushdown()` result. That means a new FE can send explicit
`COUNT(col)` slot IDs, but V1 still treats the plan like ambiguous old COUNT
pushdown and can return table/file row counts without the
nullopt/empty/non-empty safety checks. Please either disable external COUNT
metadata pushdown when V1 is selected, or thread the COUNT argument list
through V1 and apply the same safety gates before using table/file row counts.
--
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]
