github-actions[bot] commented on code in PR #65548:
URL: https://github.com/apache/doris/pull/65548#discussion_r3576573151
##########
be/src/format_v2/table_reader.h:
##########
@@ -1433,16 +1433,15 @@ class TableReader {
request->columns.clear();
if (agg_type == TPushAggOp::type::COUNT) {
// COUNT pushdown historically meant COUNT(*) and therefore
carried no columns. For
- // complex COUNT(col), materializing the full MAP/LIST/STRUCT
value only to test the
- // top-level NULL bit can be extremely expensive. When the scan
projects exactly one
- // directly-mapped complex column, pass that file column to the
reader so formats such
- // as Parquet can count the column shape from metadata/levels
without decoding payload
- // values like MAP value strings. Other COUNT cases stay on the
existing row-count path
- // to avoid changing count(*) semantics.
+ // COUNT(col), Nereids retains exactly the counted slot in the
file scan. Pass that
+ // single direct mapping for both scalar and complex columns:
besides allowing nullable
+ // complex values to be counted from levels, it lets the file
reader validate logical
+ // types before returning footer row counts. For example, a
required TIME_MILLIS leaf
+ // has the same count as COUNT(*), but it must still fail as an
unsupported requested
+ // column rather than silently returning the row-group count.
if (_data_reader.column_mapper->mappings().size() == 1) {
const auto& mapping =
_data_reader.column_mapper->mappings()[0];
if (mapping.file_local_id.has_value() && mapping.file_type !=
nullptr &&
-
is_complex_type(remove_nullable(mapping.file_type)->get_primitive_type()) &&
Review Comment:
This still needs to preserve the COUNT(*) vs COUNT(col) distinction.
`TPushAggOp::COUNT` is used for both shapes, and the only signal here is the
post-pruning scan mapping. For a real `COUNT(*)`, column pruning can leave one
arbitrary physical column under the scan (`pruneOutput()` picks a minimum
column, then `newProjectIfNotPruned()` wraps it with a literal/empty project),
so this branch turns an empty row-count request into `COUNT(that_column)`. File
readers such as Parquet/ORC then count only non-null values for a nullable
placeholder column, or reject an unsupported placeholder leaf, even though
COUNT(*) should return the selected row count without observing any column
nullability/logical type. Please pass a count column only when FE/BE knows the
aggregate argument was `COUNT(col)` and the mapping is semantically countable;
true COUNT(*) requests should keep `request.columns` empty.
##########
be/src/format_v2/table_reader.h:
##########
@@ -1433,16 +1433,15 @@ class TableReader {
request->columns.clear();
if (agg_type == TPushAggOp::type::COUNT) {
// COUNT pushdown historically meant COUNT(*) and therefore
carried no columns. For
- // complex COUNT(col), materializing the full MAP/LIST/STRUCT
value only to test the
- // top-level NULL bit can be extremely expensive. When the scan
projects exactly one
- // directly-mapped complex column, pass that file column to the
reader so formats such
- // as Parquet can count the column shape from metadata/levels
without decoding payload
- // values like MAP value strings. Other COUNT cases stay on the
existing row-count path
- // to avoid changing count(*) semantics.
+ // COUNT(col), Nereids retains exactly the counted slot in the
file scan. Pass that
+ // single direct mapping for both scalar and complex columns:
besides allowing nullable
+ // complex values to be counted from levels, it lets the file
reader validate logical
+ // types before returning footer row counts. For example, a
required TIME_MILLIS leaf
+ // has the same count as COUNT(*), but it must still fail as an
unsupported requested
+ // column rather than silently returning the row-group count.
if (_data_reader.column_mapper->mappings().size() == 1) {
const auto& mapping =
_data_reader.column_mapper->mappings()[0];
if (mapping.file_local_id.has_value() && mapping.file_type !=
nullptr &&
Review Comment:
This still needs to preserve the COUNT(*) vs COUNT(col) distinction.
`TPushAggOp::COUNT` is used for both shapes, and the only signal here is the
post-pruning scan mapping. For a real `COUNT(*)`, column pruning can leave one
arbitrary physical column under the scan (`pruneOutput()` picks a minimum
column, then `newProjectIfNotPruned()` wraps it with a literal/empty project),
so this branch turns an empty row-count request into `COUNT(that_column)`. File
readers such as Parquet/ORC then count only non-null values for a nullable
placeholder column, or reject an unsupported placeholder leaf, even though
COUNT(*) should return the selected row count without observing any column
nullability/logical type. Please pass a count column only when FE/BE knows the
aggregate argument was `COUNT(col)` and the mapping is semantically countable;
true COUNT(*) requests should keep `request.columns` empty.
--
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]
