github-actions[bot] commented on code in PR #66008:
URL: https://github.com/apache/doris/pull/66008#discussion_r3649467806
##########
be/src/exec/operator/file_scan_operator.cpp:
##########
@@ -116,14 +116,9 @@ bool FileScanLocalState::_should_use_file_scanner_v2(const
TQueryOptions& query_
const bool is_transactional_hive =
scan_params.__isset.table_format_params &&
scan_params.table_format_params.table_format_type ==
"transactional_hive";
- // JNI reader selection is stored per split, but this scan-level selector
cannot inspect the
- // split yet. Older FEs may omit both the scan-level Paimon marker and
split-level reader_type,
- // so keep JNI scans on V1 until scanner selection can distinguish every
compatibility shape.
return query_options.__isset.enable_file_scanner_v2 &&
query_options.enable_file_scanner_v2 &&
- !is_load && scan_params.format_type != TFileFormatType::FORMAT_WAL
&&
- scan_params.format_type != TFileFormatType::FORMAT_ES_HTTP &&
- scan_params.format_type != TFileFormatType::FORMAT_LANCE &&
- scan_params.format_type != TFileFormatType::FORMAT_JNI &&
!is_transactional_hive;
+ !is_load && scan_params.format_type !=
TFileFormatType::FORMAT_ES_HTTP &&
+ scan_params.format_type != TFileFormatType::FORMAT_LANCE &&
!is_transactional_hive;
Review Comment:
[P1] Do not evaluate JNI residual predicates twice
Removing the `FORMAT_JNI` exclusion sends Paimon JNI scans through this V2
path, but its predicate-ownership contract is not honored by `JniTableReader`:
FileScannerV2 retains the first unsafe/stateful conjunct and its suffix in
`_scanner_residual_conjuncts`, while the JNI child still filters with the
entire cloned `_conjuncts` vector. Scanner then applies the retained suffix
again. For example, `rand() < 0.5` is evaluated twice and keeps roughly 25%
instead of 50% of the rows; stateful/error-preserving expressions likewise see
the wrong invocation count. The current hybrid test installs
`OneRowTableReader` as the JNI child, so it bypasses this path. Keep JNI on V1
until the ownership contract is implemented, or make the JNI reader filter only
its owned prefix and cover the real JNI finalization path.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/paimon/source/PaimonScanNode.java:
##########
@@ -266,21 +266,11 @@ private void setPaimonParams(TFileRangeDesc rangeDesc,
PaimonSplit paimonSplit)
String fileFormat = getFileFormat(paimonSplit.getPathString());
if (split != null) {
- // use jni reader or paimon-cpp reader
rangeDesc.setFormatType(TFileFormatType.FORMAT_JNI);
- // Use Paimon native serialization for paimon-cpp reader
- if (sessionVariable.isEnablePaimonCppReader() && split instanceof
DataSplit) {
- fileDesc.setReaderType(TPaimonReaderType.PAIMON_CPP);
-
fileDesc.setPaimonSplit(PaimonUtil.encodeDataSplitToString((DataSplit) split));
- } else {
- fileDesc.setReaderType(TPaimonReaderType.PAIMON_JNI);
-
fileDesc.setPaimonSplit(PaimonUtil.encodeObjectToString(split));
- }
- // Set table location for paimon-cpp reader
- String tableLocation = source.getTableLocation();
- if (tableLocation != null) {
- fileDesc.setPaimonTable(tableLocation);
- }
+ // A logical DataSplit may span multiple files, so keep it intact
for the JNI reader
+ // until the C++ path has a split-aware V2 adapter.
+ fileDesc.setReaderType(TPaimonReaderType.PAIMON_JNI);
+ fileDesc.setPaimonSplit(PaimonUtil.encodeObjectToString(split));
Review Comment:
[P1] Preserve the old-BE Paimon split contract
With `enable_paimon_cpp_reader=true`, a BE from before optional
`reader_type` ignores field 17 and still selects `PaimonCppReader` from the
forwarded query option. This line now sends Java `InstantiationUtil` bytes (and
no `paimon_table`), but that old reader decodes the split with native
`Split::Deserialize`, so a new FE cannot query through an old BE during a
rolling upgrade. This is distinct from the existing explicit-`PAIMON_CPP`
thread because the old BE cannot observe the new discriminator. Preserve the
option-dependent wire format until all supported BEs understand the field, or
negotiate this capability, and add a new-FE/old-BE compatibility test.
--
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]
