github-actions[bot] commented on code in PR #61518:
URL: https://github.com/apache/doris/pull/61518#discussion_r3615223399
##########
be/src/storage/id_manager.h:
##########
@@ -82,15 +82,17 @@ struct ExternalFileMappingInfo {
* 1. If the file belongs to a partition, columns_from_path_keys and
columns_from_path in TFileRangeDesc are needed when materializing the partition
column
* 2. path, file_type, modification_time,compress_type .... used to read
the file
* 3. TFileFormatType can distinguish whether it is
iceberg/hive/hudi/paimon
- */
+ */
TFileRangeDesc scan_range_desc;
bool enable_file_meta_cache;
+ std::string table_name;
ExternalFileMappingInfo(int plan_node_id, const TFileRangeDesc& scan_range,
- bool file_meta_cache)
+ bool file_meta_cache, std::string table_name_)
Review Comment:
[P1] Update the remaining three-argument callers
This makes `table_name` mandatory, but `parquet_read_lines.cpp:223` and
`orc_read_lines.cpp:201` still construct `ExternalFileMappingInfo` with three
arguments, and `id_manager_test.cpp:49` still constructs the external
`FileMapping` overload with three. All three sources are included by the
recursive `doris_be_test` glob, so that target cannot compile. Please update
those callers (and assert the restored context) or retain a deliberate
compatible overload/default.
##########
be/src/exec/scan/file_scanner_v2.cpp:
##########
@@ -817,9 +819,41 @@ Status
FileScannerV2::_to_file_format(TFileFormatType::type format_type,
Status FileScannerV2::_init_io_ctx() {
_io_ctx = create_file_scan_io_context(_state);
+ if (_local_state) {
+ _io_ctx->table_name =
_local_state->cast<FileScanLocalState>().table_name();
Review Comment:
[P2] Pass this context to V2 Iceberg delete readers
This names the V2 data-file IOContext, but both position- and
equality-delete initialization create a fresh `IcebergDeleteFileIOContext` from
`RuntimeState` only. That helper never copies these names before its
Parquet/ORC reader opens the delete file, so cold delete-file blocks remain
anonymous while the associated data file is attributed. Seed the delete-file
context from the current table-reader IOContext (at least preserving the
unambiguous table name) and cover both delete types.
##########
be/src/storage/id_manager.h:
##########
@@ -82,15 +82,17 @@ struct ExternalFileMappingInfo {
* 1. If the file belongs to a partition, columns_from_path_keys and
columns_from_path in TFileRangeDesc are needed when materializing the partition
column
* 2. path, file_type, modification_time,compress_type .... used to read
the file
* 3. TFileFormatType can distinguish whether it is
iceberg/hive/hudi/paimon
- */
+ */
TFileRangeDesc scan_range_desc;
bool enable_file_meta_cache;
+ std::string table_name;
Review Comment:
[P2] Preserve context for internal row-ID rereads too
The latest fix carries the table only in `ExternalFileMappingInfo`;
`InternalFileMappingInfo` still records just tablet/rowset/segment.
`SegmentIterator` emits that mapping for global row IDs, and
`RowIdStorageReader::read_doris_format_row()` later loads the segment and
creates fresh row-store/column-store IOContexts without names. A TopN
lazy-materialization miss on a second-phase-only page is therefore persisted
with `context_id = 0`. Please carry the scan context through the internal
mapping and restore it in each second-phase IOContext.
##########
fe/fe-core/src/main/java/org/apache/doris/datasource/iceberg/IcebergUtils.java:
##########
@@ -776,6 +776,40 @@ 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()) {
Review Comment:
[P1] Do not drop identity values in mixed specs
For a spec such as `identity(dt), day(ts)`, `getIdentityPartitionColumns()`
still marks `dt` as a path-only/non-file slot, but this return discards the
`dt` value when the loop reaches `day(ts)`. `createIcebergSplit()` then keeps
only an empty accounting sentinel, and `setIcebergParams()` unsets the generic
path values without restoring `dt`, leaving the scan with no source for that
required slot. Keep identity values used for materialization separate from the
all-or-nothing cache-context decision, and add a mixed-spec
range/materialization test.
##########
gensrc/proto/file_cache.proto:
##########
@@ -59,5 +59,6 @@ message BlockMetaPb {
optional FileCacheType type = 1;
optional uint64 size = 2;
optional uint64 ttl = 3;
+ optional uint64 context_id = 4;
Review Comment:
[P2] Preserve attribution across an old-BE rewrite
The default-family migration now lets the previous BE reopen this DB, but
that binary parses a new block into its old `(type,size,ttl)` `BlockMeta` and
`change_key_meta_type()` serializes a fresh protobuf containing only fields
1-3. A new-BE fill followed by rollback, TTL reconciliation, and re-upgrade
therefore strips field 4 from a surviving block and makes `FILE_CACHE_INFO`
anonymous. The current compatibility test covers opening only; please use a
side/versioned association that old rewrites cannot erase, and add a new-write
-> old-format type rewrite -> new-read test.
##########
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
Review Comment:
[P2] Keep dictionary failures repairable
`get_or_create_context_id()` also returns 0 for a RocksDB Get/Write error or
malformed value, so this policy persists an anonymous block and reports
successful finalization. Existing-cell hits, restart loading, and cache-type
rewrites never replace that zero ID, making one transient dictionary failure
permanent until eviction even after RocksDB recovers. Return an error-bearing
result or retain pending context and retry/late-bind it; if cache writes must
stay available, add a fault-injection test proving attribution is repaired
after recovery.
##########
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());
}
planNode.setFileScanNode(fileScanNode);
super.toThrift(planNode);
}
+ public static void fillTablePartitionContext(TFileRangeDesc range, TableIf
table, String partitionName) {
+ if (range == null) {
+ return;
+ }
+ if (partitionName != null && !partitionName.isEmpty()) {
+ range.setPartitionName(partitionName);
+ }
+ }
+
+ public static String buildPartitionName(List<String> partitionKeys,
List<String> partitionValues) {
+ List<TPartitionKeyValue> partitionKeyValues =
buildPartitionKeyValues(partitionKeys, partitionValues);
+ if (partitionKeyValues.isEmpty()) {
+ return "";
+ }
+ return buildPartitionName(partitionKeyValues);
+ }
+
+ public static List<TPartitionKeyValue> buildPartitionKeyValues(
+ List<String> partitionKeys, List<String> partitionValues) {
+ return buildPartitionKeyValues(partitionKeys, partitionValues, null);
+ }
+
+ public static List<TPartitionKeyValue> buildPartitionKeyValues(
+ List<String> partitionKeys, List<String> partitionValues,
List<Boolean> partitionValueIsNull) {
+ if (partitionKeys == null || partitionValues == null) {
+ return Collections.emptyList();
+ }
+ if (partitionKeys.isEmpty() || partitionKeys.size() !=
partitionValues.size()
+ || (partitionValueIsNull != null && partitionKeys.size() !=
partitionValueIsNull.size())) {
+ return Collections.emptyList();
+ }
+ List<TPartitionKeyValue> partitionKeyValues =
Lists.newArrayListWithCapacity(partitionKeys.size());
+ for (int i = 0; i < partitionKeys.size(); i++) {
+ boolean isNull = partitionValueIsNull != null &&
Boolean.TRUE.equals(partitionValueIsNull.get(i));
+ partitionKeyValues.add(buildPartitionKeyValue(
+ partitionKeys.get(i), isNull ? null :
partitionValues.get(i)));
+ }
+ return partitionKeyValues;
+ }
+
+ public static List<TPartitionKeyValue> buildPartitionKeyValues(Map<String,
String> partitionValues) {
+ if (partitionValues == null || partitionValues.isEmpty()) {
+ return Collections.emptyList();
+ }
+ return partitionValues.entrySet().stream()
+ .sorted(Map.Entry.comparingByKey())
+ .map(entry -> buildPartitionKeyValue(entry.getKey(),
entry.getValue()))
+ .collect(Collectors.toList());
+ }
+
+ private static TPartitionKeyValue buildPartitionKeyValue(String key,
String value) {
+ TPartitionKeyValue keyValue = new TPartitionKeyValue();
+ keyValue.setKey(key);
+ if (value == null) {
+ keyValue.setValue("");
+ keyValue.setIsNull(true);
+ } else {
+ keyValue.setValue(value);
+ }
+ return keyValue;
+ }
+
+ public static String buildPartitionName(List<TPartitionKeyValue>
partitionKeyValues) {
+ if (partitionKeyValues == null || partitionKeyValues.isEmpty()) {
+ return "";
+ }
+ StringBuilder builder = new StringBuilder();
+ for (int i = 0; i < partitionKeyValues.size(); i++) {
+ TPartitionKeyValue keyValue = partitionKeyValues.get(i);
+ if (i > 0) {
+ builder.append("/");
+ }
+ builder.append(keyValue.getKey());
+ builder.append("=");
+ builder.append(keyValue.isSetValue() ? keyValue.getValue() : "");
Review Comment:
[P2] Use a collision-free persistent partition identity
This flattening ignores `is_null` and does not escape component boundaries.
A SQL NULL and a real empty string both become `dt=`, and for fixed keys
`[a,b]`, values `["x/b=y","z"]` and `["x","y/b=z"]` both become `a=x/b=y/b=z`.
Because this string is interned as the durable dictionary key, distinct
partitions can share one context ID and be permanently misreported. Encode the
ordered structured list (including null flags) unambiguously; keep a separate
human-readable label if needed.
##########
be/src/io/cache/file_cache_common.h:
##########
@@ -117,6 +118,9 @@ struct KeyMeta {
uint64_t expiration_time; // absolute time
FileCacheType type;
int64_t tablet_id {0};
+ std::string table_name;
Review Comment:
[P2] Keep deduplicated context out of every FileBlock
`CacheContext(io_ctx)` copies both names on each indirect-cache read,
including hits, and `add_cell()` copies them again into every resident
`FileBlock`. After `finalize()` resolves an ID, later metadata rewrites use
only `context_id`, yet the strings remain resident and restart loading expands
every ID back into per-block strings. At the default 1 MiB block size, a 1 TiB
cache has about 1,048,576 blocks, so two string objects plus payload per block
add substantial untracked memory and allocation traffic. Please share/intern
the context on the read path and retain only the ID after finalization/reload.
--
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]
