github-actions[bot] commented on code in PR #65548:
URL: https://github.com/apache/doris/pull/65548#discussion_r3593618398
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -462,6 +495,21 @@ Status FileScannerV2::_init_table_reader(const
TFileRangeDesc& range) {
VExprContextSPtrs table_conjuncts;
RETURN_IF_ERROR(_build_table_conjuncts(&table_conjuncts));
+ std::optional<std::vector<format::GlobalIndex>> push_down_count_columns;
+ const auto& push_down_count_slot_ids =
_local_state->get_push_down_count_slot_ids();
+ if (push_down_count_slot_ids.has_value()) {
+ push_down_count_columns.emplace();
+ push_down_count_columns->reserve(push_down_count_slot_ids->size());
+ for (const auto slot_id : *push_down_count_slot_ids) {
+ const auto global_index_it =
_slot_id_to_global_index.find(slot_id);
+ if (global_index_it == _slot_id_to_global_index.end()) {
+ return Status::InternalError(
+ "Pushed-down COUNT argument is not a projected file
scan slot, slot_id={}",
+ slot_id);
+ }
+ push_down_count_columns->push_back(global_index_it->second);
+ }
+ }
RETURN_IF_ERROR(_table_reader->init({
.projected_columns = _projected_columns,
Review Comment:
[P2] Forward COUNT semantics into Hudi's active child reader. Native Hudi
COW scans advertise Parquet/ORC, so they select FileScannerV2 and construct
`HudiHybridReader`; this line initializes only that outer object.
`_init_child_reader()` forwards `_push_down_agg_type` but not
`_push_down_count_columns`, so the child sees `nullopt` and the new old-FE
safety gate rejects metadata COUNT. This regresses ordinary Hudi COW `COUNT(*)`
(and safe direct `COUNT(col)`) from footer aggregation to a full row scan.
Please pass the optional argument state to the child and delegate/mirror
`current_split_uses_metadata_count()` so restored synthetic rows are not fed to
adaptive sizing.
##########
regression-test/suites/external_table_p0/tvf/test_file_scanner_v2_review_fixes.groovy:
##########
@@ -0,0 +1,113 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_file_scanner_v2_review_fixes", "p0,external") {
+ def backends = sql "show backends"
+ assertTrue(backends.size() > 0)
+ def backendId = backends[0][0]
+ def dataPath = context.config.dataPath + "/external_table_p0/tvf"
+ // A developer may point this at fixtures already visible to every BE (for
example, a shared
+ // checkout mounted at /tmp). CI leaves it unset and exercises the normal
per-BE distribution.
+ def predeployedFixturePath =
context.config.otherConfigs.get("fileScannerV2FixturePath")
+ def remotePath = predeployedFixturePath ?:
"/tmp/file_scanner_v2_review_fixes"
+ def fixtureNames = [
+ "file_scanner_v2_empty.csv",
+ "file_scanner_v2_struct_0_bigint.parquet",
+ "file_scanner_v2_struct_1_int.parquet",
+ "file_scanner_v2_unsupported_time.parquet"
+ ]
+
+ if (predeployedFixturePath == null) {
+ mkdirRemotePathOnAllBE("root", remotePath)
+ for (def backend : backends) {
+ for (def fixtureName : fixtureNames) {
+ scpFiles("root", backend[1], "${dataPath}/${fixtureName}",
remotePath, false)
+ }
+ }
+ }
+
+ sql "set enable_file_scanner_v2 = true"
+
+ // A zero-byte CSV is a valid zero-row split when the schema is supplied
explicitly. The EOF
+ // raised during reader preparation must skip this split instead of
failing the query.
+ order_qt_empty_csv """
+ SELECT id, name
+ FROM local(
+ "file_path" = "${remotePath}/file_scanner_v2_empty.csv",
+ "backend_id" = "${backendId}",
+ "format" = "csv",
+ "csv_schema" = "id:int;name:string")
+ ORDER BY id
+ """
+
+ // The first file defines STRUCT<a: BIGINT>, while the second stores a as
INT. Localization is
+ // split-specific: the BIGINT file compares BIGINT values, while the old
INT file safely rewrites
+ // the exactly representable BIGINT literal 10 to INT and compares in the
physical file type.
+ // If a literal cannot round-trip through INT, the mapper instead casts
the INT data to BIGINT.
+ // Repeating the multi-file read also verifies that neither predicate
rewrites nor page-cache
+ // range state leak from one file schema into the next file or the warm
scan.
+ def evolvedStructQuery = """
+ SELECT id, col.a, col.b
+ FROM local(
+ "file_path" = "${remotePath}/file_scanner_v2_struct_*.parquet",
+ "backend_id" = "${backendId}",
+ "format" = "parquet")
+ WHERE col.a = CAST(10 AS BIGINT)
+ ORDER BY id
+ """
+ order_qt_struct_leaf_promotion_cold evolvedStructQuery
+ order_qt_struct_leaf_promotion_warm evolvedStructQuery
+
+ // COUNT(col) sees a non-trivial mapping for the file whose STRUCT leaf is
INT while the merged
+ // table type is BIGINT. It must fall back to the normal scan so
schema-evolution casts and
+ // nullability checks run instead of counting footer values directly.
+ order_qt_count_evolved_struct """
+ SELECT COUNT(col.a)
Review Comment:
[P2] Exercise this COUNT fallback through a reachable planner path.
`local(...)` becomes a `LogicalTVFRelation`/`PhysicalTVFRelation`, but the
storage-layer aggregate rules only match `LogicalFileScan`, and
`visitPhysicalTVFRelation` never sets either the COUNT opcode or
`pushDownCountSlotIds`. The unit test manufactures those fields with direct
setters, so this query is just a normal scan and still returns 4 if the new
thrift transport or non-trivial-mapping fallback is broken. Please use a
relation type that actually receives storage COUNT pushdown and assert the
planned/thrift state, or add the missing TVF rule/translator wiring before
relying on this as coverage.
--
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]
