github-actions[bot] commented on code in PR #61518:
URL: https://github.com/apache/doris/pull/61518#discussion_r3620702685
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/FileQueryScanNode.java:
##########
@@ -563,19 +576,16 @@ private TFileRangeDesc createFileRangeDesc(FileSplit
fileSplit, List<String> col
// fileSize only be used when format is orc or parquet and TFileType
is broker
// When TFileType is other type, it is not necessary
rangeDesc.setFileSize(fileSplit.getFileLength());
- if (!columnsFromPathKeys.isEmpty()) {
- rangeDesc.setColumnsFromPath(columnsFromPath);
- rangeDesc.setColumnsFromPathKeys(columnsFromPathKeys);
- rangeDesc.setColumnsFromPathIsNull(columnsFromPathIsNull);
- }
-
rangeDesc.setFileType(fileSplit.getLocationType());
rangeDesc.setPath(fileSplit.getPath().toStorageLocation().toString());
if (fileSplit.getLocationType() == TFileType.FILE_HDFS) {
URI fileUri = fileSplit.getPath().getPath().toUri();
rangeDesc.setFsName(fileUri.getScheme() + "://" +
fileUri.getAuthority());
}
rangeDesc.setModificationTime(fileSplit.getModificationTime());
+ List<TPartitionKeyValue> partitionKeyValues =
+ FileScanNode.buildPartitionKeyValues(columnsFromPathKeys,
columnsFromPath, columnsFromPathIsNull);
+ FileScanNode.fillPathPartitionContext(rangeDesc, desc.getTable(),
partitionKeyValues);
Review Comment:
[P2] Refresh cache context after connector setup
This context is built before `setScanParams()`. Plugin-driven Paimon adds
its partition fields only in that later hook, so partitioned files reach both
scanners with a table-only context; the first cold fill persists it and later
hits cannot repair it. Reconstructing generically from the legacy path fields
is unsafe because mixed-spec Iceberg intentionally uses those fields only for
partial row materialization. Please populate or refresh the structured context
after connector-specific setup (or in the Paimon hook), keep intentional
omission distinguishable from legacy absence, and cover Paimon plus mixed-spec
Iceberg.
##########
be/src/io/cache/cache_block_meta_store.cpp:
##########
@@ -110,16 +203,25 @@ Status CacheBlockMetaStore::init() {
}
_db.reset(db_ptr);
- // Store the file_cache_meta column family handle
- // handles[0] is default column family, handles[1] is file_cache_meta
+ // handles[0] is default column family, handles[1] is file_cache_meta.
+ // Store context dictionary records in the default family to keep
downgrade-open compatibility.
if (handles.size() >= 2) {
+ _context_dict_cf_handle.reset(handles[0]);
_file_cache_meta_cf_handle.reset(handles[1]);
- // Close default column family handle as we won't use it
- _db->DestroyColumnFamilyHandle(handles[0]);
+ if (has_legacy_context_cf && handles.size() >= 3) {
+ std::unique_ptr<rocksdb::ColumnFamilyHandle>
legacy_context_cf_handle(handles[2]);
+
RETURN_IF_ERROR(_migrate_legacy_context_dict(legacy_context_cf_handle.get()));
+ const auto drop_status =
_db->DropColumnFamily(legacy_context_cf_handle.get());
+ if (!drop_status.ok()) {
+ LOG(WARNING) << "Failed to drop legacy context dictionary
column family: "
+ << drop_status.ToString();
+ }
+ }
} else {
- return Status::InternalError("Failed to get file_cache_meta column
family handle");
+ return Status::InternalError("Failed to get default/file_cache_meta
column family handles");
}
+ RETURN_IF_ERROR(_load_next_context_id());
Review Comment:
[P2] Do not expose a partially initialized meta store
At this point the RocksDB and column-family handles are already installed,
but migration or `_load_next_context_id()` can still return before the write
worker starts. The constructor only logs that failure and
`FSFileCacheStorage::init()` continues, so later fills can allocate from the
still-default ID 1 and overwrite an association referenced by existing block
metadata; every `put()` also queues work with no consumer. Please propagate the
initialization failure (or tear down and retry before exposing the store), and
add a fault-injection reopen test that proves IDs are not reused and the worker
is running.
##########
be/src/io/cache/block_file_cache_downloader.cpp:
##########
@@ -280,6 +280,8 @@ void FileCacheBlockDownloader::download_file_cache_block(
.expiration_time = meta.expiration_time(),
.is_dryrun =
config::enable_reader_dryrun_when_download_file_cache,
.is_warmup = true,
+ .table_name = "",
Review Comment:
[P2] Preserve context during hot-block redistribution
The source block can already have a valid table/partition association, but
`get_hot_blocks_meta()` and `FileCacheBlockMeta` omit it, so this destination
warmup hardcodes both names empty and finalizes the downloaded block with
context ID 0. A later named scan only reuses that cell and cannot repair the
metadata. Please carry optional context fields through the hot-block response
and initialize the destination from them, with a current-BE
source-to-destination attribution test.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java:
##########
@@ -1306,11 +1308,14 @@ protected void toThrift(TPlanNode msg) {
} else {
msg.olap_scan_node.setKeyType(olapTable.getKeysType().toThrift());
}
- String tableName = olapTable.getName();
- if (selectedIndexId != -1) {
- tableName = tableName + "(" + getSelectedIndexName() + ")";
+ msg.olap_scan_node.setTableName(buildFileCacheTableName());
+ String partitionName = "";
+ if (selectedPartitionIds != null && selectedPartitionIds.size() == 1) {
+ Long partitionId = selectedPartitionIds.iterator().next();
+ Partition partition = olapTable.getPartition(partitionId);
+ partitionName = buildFileCachePartitionName(partition);
}
- msg.olap_scan_node.setTableName(tableName);
+ msg.olap_scan_node.setPartitionName(partitionName);
Review Comment:
[P2] Preserve per-tablet partitions for external scans
`open_scanner` rebuilds its ranges from tablet IDs, but neither
`TQueryPlanInfo` nor `TScanOpenParams` carries a tablet-to-partition mapping.
For a plan selecting multiple partitions this node fallback is also
deliberately empty, so those scans have no partition identity from either the
range or node and cold blocks are permanently finalized as table-only. Please
carry optional per-tablet partition identity through the external-plan
protocol, restore it on each regenerated range, and cover a two-partition
`open_scanner` plan followed by `FILE_CACHE_INFO`.
##########
be/src/io/io_common.h:
##########
@@ -214,6 +214,9 @@ struct IOContext {
bool bypass_peer_read {false};
FileCacheMissPolicy file_cache_miss_policy =
FileCacheMissPolicy::READ_THROUGH_AND_WRITE_BACK;
RemoteScanCacheWriteLimiter* remote_scan_cache_write_limiter = nullptr; //
Ref
+ // table/partition context (optional)
Review Comment:
[P2] Populate cache identity for point-query reads
`PointQueryExecutor` creates three fresh file-cache-aware query contexts for
primary-key lookup, row-store reads, and missing-column reads, but none sets
these new fields and `PTabletKeyLookupRequest` cannot carry them. A cold
short-circuit query can therefore finalize those blocks with context ID 0,
after which a normal OLAP scan only hits the cells and cannot repair their
attribution. Please add an optional versioned identity to the lookup
request/reusable state, populate all three contexts, and test a cold point
lookup followed by `FILE_CACHE_INFO`.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/FileScanNode.java:
##########
@@ -95,12 +96,138 @@ protected void toThrift(TPlanNode planNode) {
TFileScanNode fileScanNode = new TFileScanNode();
fileScanNode.setTupleId(desc.getId().asInt());
if (desc.getTable() != null) {
- fileScanNode.setTableName(desc.getTable().getName());
+
fileScanNode.setTableName(desc.getTable().getNameWithFullQualifiers());
Review Comment:
[P2] Version the table identity before persisting it
This changes the meaning of an existing wire field: an old FE sends only
`Table.getName()`, while the new BE now interns whatever `table_name` it
receives as a durable identity. During a rolling upgrade, two old-FE scans of
`db1.t` and `db2.t` (and the same partition label) therefore share one
dictionary context, and later new-FE hits do not replace that nonzero legacy
association. The OLAP scan-node/range field has the same
simple-name-to-qualified-name transition. Please carry an explicit
new/versioned cache identity (or presence marker) instead of treating the
legacy display name as authoritative, and test old-FE/new-BE fills followed by
a new-FE read.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java:
##########
@@ -563,6 +563,8 @@ private void addScanRangeLocations(Partition partition,
paloRange.setBinlogScanType(binlogScanType);
}
}
+ paloRange.setTableName(buildFileCacheTableName());
Review Comment:
[P2] Make the persisted identity rename-safe
These mutable names are persisted only on the block's first fill. Renaming
an OLAP table or partition leaves its tablet/rowset segment paths and cache
keys unchanged, so later scans hit the same cells under the new names without
updating the old nonzero context. `FILE_CACHE_INFO` then reports the retired
names until eviction and may conflate an object that later reuses them. Please
persist stable IDs and resolve display names at observation time, or add an
explicit rename/late-binding update path, with a rename-then-cache-hit 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]
