morningman commented on code in PR #66399:
URL: https://github.com/apache/doris/pull/66399#discussion_r3733148370
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -99,7 +101,11 @@ bool is_supported_table_format(const TFileRangeDesc& range)
{
return false;
}
return table_format == "NotSet" || table_format == "tvf" || table_format
== "hive" ||
- table_format == "iceberg" || table_format == "paimon" ||
table_format == "hudi";
+ table_format == "iceberg" || table_format == "paimon" ||
table_format == "hudi" ||
+ // A lake split of a fluss primary-key table read as its lake plus
its log tail. It is the
Review Comment:
Confirmed — and the suites were hiding it rather than covering it: eleven of
the twelve fluss suites `set enable_file_scanner_v2 = true`, which is exactly
why this never showed up.
Fixed in a01436d8f01, by forcing V2 rather than failing during planning. The
variable reads as a preference for scans both implementations support ("uses
FileScannerV2 for supported query scans"), so failing a query outright for a
table type that is supported seemed the worse of the two.
It takes two halves, because the choice is made before any range is fetched
and so can only be read off the **scan-level** params:
- `FlussScanPlanProvider.populateScanLevelParams` now stamps the scan-level
`table_format_type`. It is stamped last, so the lake half’s turn at the same
params cannot take the marker away.
- `_should_use_file_scanner_v2` admits that marker unconditionally — the
mirror image of the `transactional_hive` stamp, which uses this same channel to
force the opposite choice.
The node is stamped for being planned by this connector rather than for the
ranges it ended up with: one node can carry both `fluss` and `fluss_union`
ranges, and a rule that depended on what `planScan` produced would go quiet
exactly when the planning order changed.
For tests: the pins stay (they are there for the fuzzy mode), and there is
now a read with the variable **off** on each range kind — log, PK, partitioned,
and the union read that carries both kinds on one node — asserting the variable
cannot change the answer. Plus `FileScanOperatorFlussTest` at the unit level. I
checked the mutation: dropping the branch turns exactly those tests red.
Unrelated to this PR, but I ran into it while looking for the precedent to
copy: **the `adbc` branch in `_should_use_file_scanner_v2` looks like it can
never fire.** ADBC implements neither `populateScanLevelParams` nor
`setTableFormatParams` anywhere, so `scan_params.__isset.table_format_params`
is false for an ADBC scan and it falls through to the session variable —
meaning `enable_file_scanner_v2=false` should break ADBC the same way it broke
fluss. That is static reading only, I have not run it. Happy to open a separate
issue if it holds up.
--
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]
