github-actions[bot] commented on code in PR #61518:
URL: https://github.com/apache/doris/pull/61518#discussion_r3600619383
##########
be/src/exec/scan/parallel_scanner_builder.cpp:
##########
@@ -296,6 +296,12 @@ Status ParallelScannerBuilder::_load() {
std::shared_ptr<OlapScanner> ParallelScannerBuilder::_build_scanner(
BaseTabletSPtr tablet, int64_t version, const
std::vector<OlapScanRange*>& key_ranges,
TabletReadSource&& read_source, io::FileCacheStatistics&&
initial_file_cache_stats) {
+ std::string table_name;
+ std::string partition_name;
+ if (auto it = _tablet_contexts.find(tablet->tablet_id()); it !=
_tablet_contexts.end()) {
Review Comment:
This lookup happens only in `_build_scanner()`, after `build_scanners()` has
already run `_load()`. `_load()` calls `get_segment_num_rows()` with
`create_preload_io_context()`, which leaves both names empty; when row counts
must be read from segment footers, that path can create and persist file-cache
blocks with `context_id = 0`. Later scanner reads only reuse/touch the existing
cells and do not merge this context into them, so those footer blocks remain
anonymous. Please apply the tablet context to the preload `IOContext` before
`get_segment_num_rows()` and cover that cache-populating path.
##########
be/src/io/cache/fs_file_cache_storage.cpp:
##########
@@ -321,8 +322,16 @@ Status FSFileCacheStorage::finalize(const FileCacheKey&
key, const size_t size)
}
BlockMetaKey mkey(key.meta.tablet_id, UInt128Wrapper(key.hash),
key.offset);
- BlockMeta meta(key.meta.type, size, key.meta.expiration_time);
- _meta_store->put(mkey, meta);
+ uint64_t context_id = 0;
+ if (_meta_store) {
+ // Table/partition context is only used for observability. Any
dictionary lookup/write
+ // failure must degrade to an empty context id instead of affecting
cache writes.
+ context_id =
+ _meta_store->get_or_create_context_id(key.meta.table_name,
key.meta.partition_name);
Review Comment:
`finalize()` runs once per file-cache block, and
`get_or_create_context_id()` always issues a synchronous RocksDB `Get` even for
an existing pair. Because the default block size is 1 MiB and no context ID is
resolved before block splitting, a 1 TiB cold fill repeats roughly one million
reads for what is commonly one table/partition, while holding each block's
finalize path. Please intern/cache the forward mapping in memory or resolve the
context once upstream and propagate its ID to sibling blocks.
##########
be/src/exec/scan/file_scanner.h:
##########
@@ -310,6 +313,9 @@ class FileScanner : public Scanner {
Status _init_io_ctx() {
_io_ctx = create_file_scan_io_context(_state);
+ if (_local_state) {
Review Comment:
This adds the table context only to FileScanner V1, but supported non-load
scans use FileScannerV2 by default (`enable_file_scanner_v2 = true`). V2's
`_init_io_ctx()` only calls `create_file_scan_io_context()`, and
`_prepare_next_split()` never copies `TFileRangeDesc.partition_name`, so
ordinary Parquet/ORC queries still reach the cache with both names empty and
persist `context_id = 0`. This is distinct from the existing V1 range-timing
thread. Please mirror both the table initialization and per-split partition
refresh in V2, with a two-partition V2 test.
##########
be/src/io/cache/cache_block_meta_store.cpp:
##########
@@ -97,6 +178,9 @@ Status CacheBlockMetaStore::init() {
column_families.emplace_back(rocksdb::kDefaultColumnFamilyName,
rocksdb::ColumnFamilyOptions());
// File cache meta column family
column_families.emplace_back(FILE_CACHE_META_COLUMN_FAMILY,
rocksdb::ColumnFamilyOptions());
+ // File cache context dictionary column family
+ column_families.emplace_back(FILE_CACHE_CONTEXT_DICT_COLUMN_FAMILY,
Review Comment:
Adding a persistent column family makes this local DB incompatible with
rolling back to the previous BE: RocksDB's descriptor-based `Open` requires
every existing family to be listed, while the old binary lists only `default`
and `file_cache_meta`. After the new BE creates `file_cache_context_dict`, the
old BE cannot reopen the metadata DB; initialization merely logs the failure
and continues with no working metadata store. Please keep the dictionary in an
existing family with namespaced keys (or otherwise preserve downgrade-open
compatibility) and add an upgrade-then-downgrade reopen test.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -776,6 +777,41 @@ public static Map<String, String>
getPartitionInfoMap(PartitionData partitionDat
return partitionInfoMap;
}
+ public static Map<String, String>
getIdentityPartitionInfoMapForCache(PartitionData partitionData,
+ PartitionSpec partitionSpec, String timeZone) {
+ Map<String, String> partitionInfoMap = Maps.newLinkedHashMap();
+ List<NestedField> fields =
partitionData.getPartitionType().asNestedType().fields();
+ List<PartitionField> partitionFields = partitionSpec.fields();
+ Preconditions.checkArgument(fields.size() == partitionFields.size(),
+ "PartitionData fields size does not match PartitionSpec fields
size");
+
+ for (int i = 0; i < fields.size(); i++) {
+ NestedField field = fields.get(i);
+ PartitionField partitionField = partitionFields.get(i);
+ if (!partitionField.transform().isIdentity()) {
+ return null;
+ }
+ TypeID partitionTypeId = field.type().typeId();
+ if (partitionTypeId == TypeID.BINARY || partitionTypeId ==
TypeID.FIXED) {
+ return null;
+ }
+ String columnName =
partitionSpec.schema().findColumnName(partitionField.sourceId());
+ if (columnName == null) {
+ return null;
+ }
+ Object value = partitionData.get(i);
+ try {
+ partitionInfoMap.put(columnName.toLowerCase(Locale.ROOT),
Review Comment:
`getIdentityPartitionColumns()` preserves Iceberg's source-schema case, and
`fillPartitionContextFromMap()` later performs an exact
`containsKey(partitionKey)`. For an identity partition on source column `Dt`,
this helper stores `dt` while the consumer asks for `Dt`, so it builds no path
columns or partition context and the BE caches the file without a partition
name. This is distinct from the earlier partition-field/source-column issue:
the source column is now selected correctly, but its case is changed. Please
keep the exact `columnName` and add a mixed-case identity-partition range test.
##########
be/src/information_schema/schema_file_cache_info_scanner.cpp:
##########
@@ -114,10 +118,20 @@ Status
SchemaFileCacheInfoScanner::_fill_block_impl(Block* block) {
// Convert hash to string
std::string hash_str = key.hash.to_string();
+ std::string table_name;
+ std::string partition_name;
+ if (value.context_id != 0) {
+ if (auto context = meta_store->get_context(value.context_id);
context) {
Review Comment:
This resolves the dictionary inside the per-block metadata loop, so
`FILE_CACHE_INFO` does a RocksDB `Get` for every row even when most rows share
the same context ID. With 1 MiB blocks, a 1 TiB cache can add about one million
reverse lookups for a single table/partition. The restart loader already uses
the appropriate local `context_id -> pair` cache; please do the same here so
the query scales with distinct contexts rather than blocks.
##########
be/src/cloud/cloud_internal_service.cpp:
##########
@@ -548,7 +548,9 @@ Status trigger_peer_server_fill(io::FileBlockSPtr& fb,
int64_t fill_tablet_id,
// re-enters peer race, the original block can remain
DOWNLOADING for the
// duration of nested peer retries and timeouts.
.is_warmup = false,
- .bypass_peer_read = true},
+ .bypass_peer_read = true,
+ .table_name = "",
Review Comment:
A pull-through peer fill originates from an Olap scan whose `IOContext` now
has table/partition names, but the race passes `ctx=nullptr`,
`PFetchPeerDataRequest` carries no names, and this server-side download
hardcodes both fields empty. A miss on the designated fill compute group
therefore persists an anonymous block, and later named hits only touch/reuse
it. Please copy the two strings into the race's owned arguments, add optional
RPC fields for rolling compatibility, and initialize both the server
CacheContext and download context from them.
##########
be/src/exec/scan/file_scanner.cpp:
##########
@@ -1642,6 +1643,7 @@ Status FileScanner::read_lines_from_range(const
TFileRangeDesc& range,
const ExternalFileMappingInfo&
external_info,
int64_t* init_reader_ms, int64_t*
get_block_ms) {
_current_range = range;
+ _update_io_context_from_range();
Review Comment:
In the external row-ID/TopN second phase, `RowIdStorageReader` constructs
this FileScanner with no `FileScanLocalState`. The new `_init_io_ctx()`
therefore cannot set the table name, while this range update restores only the
partition; cache misses for previously untouched column pages are persisted
under an empty table plus a nonempty partition, and later hits do not repair
that mapping. Please carry the scan-node table name in
`ExternalFileMappingInfo` (or equivalent owned context) and restore it before
the second-phase read, with a lazy-materialization cache-context test.
##########
be/src/io/cache/cache_block_meta_store.cpp:
##########
@@ -367,6 +455,77 @@ void CacheBlockMetaStore::delete_key(const BlockMetaKey&
key) {
_write_queue.enqueue(op);
}
+uint64_t CacheBlockMetaStore::get_or_create_context_id(std::string_view
table_name,
+ std::string_view
partition_name) {
+ if ((table_name.empty() && partition_name.empty()) || !_db ||
!_context_dict_cf_handle) {
+ return 0;
+ }
+
+ const std::string lookup_key = _build_context_key(table_name,
partition_name);
+ std::string value;
+ rocksdb::Status status =
+ _db->Get(rocksdb::ReadOptions(), _context_dict_cf_handle.get(),
lookup_key, &value);
+ if (status.ok()) {
+ auto context_id = parse_context_lookup_value(value);
+ if (context_id.has_value()) {
+ return *context_id;
+ }
+ LOG(WARNING) << "Invalid context lookup value for key";
+ } else if (!status.IsNotFound()) {
+ LOG(WARNING) << "Failed to get context id from rocksdb: " <<
status.ToString();
+ }
+
+ std::lock_guard<std::mutex> lock(_context_mutex);
+
+ value.clear();
+ status = _db->Get(rocksdb::ReadOptions(), _context_dict_cf_handle.get(),
lookup_key, &value);
+ if (status.ok()) {
+ auto context_id = parse_context_lookup_value(value);
+ if (context_id.has_value()) {
+ return *context_id;
+ }
+ LOG(WARNING) << "Invalid context lookup value after retry for key";
+ return 0;
+ }
+ if (!status.IsNotFound()) {
+ LOG(WARNING) << "Failed to get context id from rocksdb after retry: "
<< status.ToString();
+ return 0;
+ }
+
+ const uint64_t context_id = _next_context_id.fetch_add(1,
std::memory_order_acq_rel);
+
+ rocksdb::WriteBatch batch;
+ batch.Put(_context_dict_cf_handle.get(), lookup_key,
build_context_lookup_value(context_id));
Review Comment:
These two dictionary records have no reclamation path. Normal LRU/block
removal deletes only the `BlockMetaKey`, and
`BlockFileCache::clear_file_cache_impl()` also removes blocks individually
without reference accounting, so successively scanned/evicted or dropped
external partitions leave their forward and reverse entries forever.
`_load_next_context_id()` then scans the full historical set on every restart.
Please add context lifetime management (for example a mark-and-sweep against
live block metadata or durable reference counts) and verify that the last block
eviction eventually reclaims both keys.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/hudi/source/HudiScanNode.java:
##########
@@ -325,21 +326,39 @@ private void setHudiParams(TFileRangeDesc rangeDesc,
HudiSplit hudiSplit) {
tableFormatFileDesc.setHudiParams(fileDesc);
Map<String, String> partitionValues =
hudiSplit.getHudiPartitionValues();
if (partitionValues != null) {
- List<String> formPathKeys = new ArrayList<>();
- List<String> formPathValues = new ArrayList<>();
- for (Map.Entry<String, String> entry : partitionValues.entrySet())
{
- formPathKeys.add(entry.getKey());
- formPathValues.add(entry.getValue());
- }
- FilePartitionUtils.ParsedColumnsFromPath parsedColumnsFromPath =
-
FilePartitionUtils.normalizeColumnsFromPath(formPathValues);
- rangeDesc.setColumnsFromPathKeys(formPathKeys);
- rangeDesc.setColumnsFromPath(parsedColumnsFromPath.getValues());
-
rangeDesc.setColumnsFromPathIsNull(parsedColumnsFromPath.getIsNull());
+ fillPartitionContextFromMap(rangeDesc, partitionValues,
getPathPartitionKeys());
}
rangeDesc.setTableFormatParams(tableFormatFileDesc);
}
+ private void fillPartitionContextFromMap(
+ TFileRangeDesc rangeDesc, Map<String, String> partitionValues,
List<String> orderedPartitionKeys) {
+ List<String> fromPathKeys = new ArrayList<>();
+ List<String> fromPathValues = new ArrayList<>();
+ for (String partitionKey : orderedPartitionKeys) {
+ if (!partitionValues.containsKey(partitionKey)) {
+ continue;
+ }
+ fromPathKeys.add(partitionKey);
+ fromPathValues.add(partitionValues.get(partitionKey));
+ }
+ List<TPartitionKeyValue> partitionKeyValues =
+ FileScanNode.buildPartitionKeyValues(fromPathKeys,
fromPathValues);
Review Comment:
On the default runtime-partition-pruning path,
`HudiUtils.getPartitionInfoMap()` copies Hive's `__HIVE_DEFAULT_PARTITION__`
sentinel verbatim. The generic range construction has already normalized that
value to `""` with `isNull=true`, but this call rebuilds and overwrites the
fields without null flags, so BE materializes the sentinel as a non-null
partition value (and numeric/date partitions can fail to cast). This is
distinct from the prior actual-Java-null issue. Please normalize the Hudi map
values with `FilePartitionUtils` and preserve the null flags, with a
default-partition scan-range 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]
