github-actions[bot] commented on code in PR #65437:
URL: https://github.com/apache/doris/pull/65437#discussion_r3556223737
##########
be/src/exec/operator/file_scan_operator.cpp:
##########
@@ -130,10 +130,8 @@ Status
FileScanLocalState::_init_scanners(std::list<ScannerSPtr>* scanners) {
const bool is_load =
state()->desc_tbl().get_tuple_descriptor(scan_params->src_tuple_id) != nullptr;
// TODO: Use scanner v2 for all queries.
- const bool use_file_scanner_v2 =
- state()->query_options().__isset.enable_file_scanner_v2 &&
- state()->query_options().enable_file_scanner_v2 && !is_load &&
- _split_source->all_scan_ranges_match(*scan_params,
FileScannerV2::is_supported);
+ const bool use_file_scanner_v2 =
state()->query_options().__isset.enable_file_scanner_v2 &&
Review Comment:
This now selects FileScannerV2 for every non-load file query whenever
`enable_file_scanner_v2` is set, but that session variable defaults to true.
For local scan sources the removed `all_scan_ranges_match(...,
FileScannerV2::is_supported)` check was the only thing keeping unsupported
formats/table formats on the existing `FileScanner` path. For example,
`LakeSoulScanNode` does not use remote batch mode, returns `FORMAT_JNI`, and
writes `table_format_type=lakesoul`;
`FileScannerV2::is_supported_jni_table_format()` does not include `lakesoul`,
and the updated unit test now expects such a range to return `NotSupported`.
With this selector, a normal LakeSoul query that previously used `FileScanner`
will fail by default because the default FE value for `enable_file_scanner_v2`
is true. Please keep the eager support check for local split sources, or
otherwise only bypass it for remote split sources that cannot enumerate ranges
up front.
##########
fe/fe-core/src/main/java/org/apache/doris/qe/SessionVariable.java:
##########
@@ -3032,9 +3032,10 @@ public static boolean isEagerAggregationOnJoin() {
public static final String ENABLE_MC_LIMIT_SPLIT_OPTIMIZATION =
"enable_mc_limit_split_optimization";
@VarAttrDef.VarAttr(
name = ENABLE_EXTERNAL_TABLE_BATCH_MODE,
+ fuzzy = true,
Review Comment:
Making this variable fuzzy means external fuzzy sessions can now advertise
`enable_external_table_batch_mode=false`, but several batch-mode
implementations still ignore that flag. Iceberg checks
`getEnableExternalTableBatchMode()` before returning true, while Hive, Hudi,
and MaxCompute only compare the selected partition count with
`num_partitions_in_batch_mode`. A fuzzy run can therefore print this variable
as false and still enter batch mode on those scan nodes, so the new fuzzy
coverage is testing a configuration that is not actually honored consistently.
Please either gate the other `isBatchMode()` implementations on this flag as
Iceberg does, or keep this variable out of the fuzzy set until every external
batch-mode path respects it.
--
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]
