github-actions[bot] commented on code in PR #65776:
URL: https://github.com/apache/doris/pull/65776#discussion_r3627166991
##########
be/src/cloud/cloud_backend_service.cpp:
##########
@@ -304,4 +305,28 @@ void
CloudBackendService::get_stream_load_record(TStreamLoadRecordResult& result
_engine.get_stream_load_recorder());
}
+void CloudBackendService::check_storage_format(TCheckStorageFormatResult&
result) {
+ for (const auto& tablet : _engine.tablet_mgr().get_all_tablet()) {
Review Comment:
[Blocking] Please take the tablet-map snapshot under one lock. This new RPC
calls get_all_tablet(), which evaluates _tablet_map->size() before the locked
traverse. TabletMap::size() reads the unordered_map without _mtx, while
cache-miss put() and the LRU Value destructor's erase() mutate that same map
under _mtx on other workers. An overlapping SHOW request therefore performs an
unsynchronized container read/write (undefined behavior); locking traverse
afterward is too late. Please have TabletMap build the shared_ptr snapshot,
including reserve, under _mtx and add a concurrent enumeration/cache-mutation
test.
##########
be/src/cloud/cloud_backend_service.cpp:
##########
@@ -304,4 +305,28 @@ void
CloudBackendService::get_stream_load_record(TStreamLoadRecordResult& result
_engine.get_stream_load_recorder());
}
+void CloudBackendService::check_storage_format(TCheckStorageFormatResult&
result) {
+ for (const auto& tablet : _engine.tablet_mgr().get_all_tablet()) {
+ result.v2_tablets.push_back(tablet->tablet_id());
Review Comment:
[Blocking] This Cloud path inventories only tablets currently resident in
each BE's volatile CloudTabletMgr cache, not all valid tablets. TabletMap is
populated only after get_tablet() materializes a tablet and entries disappear
on LRU eviction, so after a clean BE restart—or for a newly created
empty/never-read tablet—this command omits IDs that SHOW TABLETS still lists.
The new one-BE test writes every target partition first, which masks the gap.
Please source the intended inventory from durable FE/meta-service metadata, or
make the cache-only/incomplete contract explicit in the output, and add
empty/restart/eviction coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowPartitionsCommand.java:
##########
@@ -174,11 +176,14 @@ private void analyzeSubExpression(Expression subExpr)
throws AnalysisException {
} else if (!leftKey.equalsIgnoreCase(FILTER_PARTITION_ID) &&
!leftKey.equalsIgnoreCase(FILTER_BUCKETS)
&& !leftKey.equalsIgnoreCase(FILTER_REPLICATION_NUM)) {
throw new AnalysisException("Only the columns of
PartitionId/PartitionName/"
- + "State/Buckets/ReplicationNum/LastConsistencyCheckTime
are supported.");
+ + "State/Buckets/ReplicationNum/LastConsistencyCheckTime/"
+ + "InvertedIndexStorageFormat are supported.");
}
} else if (subExpr instanceof Like) {
- if (!leftKey.equalsIgnoreCase(FILTER_PARTITION_NAME) &&
!leftKey.equalsIgnoreCase(FILTER_STATE)) {
- throw new AnalysisException("Where clause :
PartitionName|State like \"p20191012|NORMAL\"");
+ if (!leftKey.equalsIgnoreCase(FILTER_PARTITION_NAME) &&
!leftKey.equalsIgnoreCase(FILTER_STATE)
+ &&
!leftKey.equalsIgnoreCase(FILTER_INVERTED_INDEX_STORAGE_FORMAT)) {
Review Comment:
[Blocking] The newly allowed LIKE path does not have SQL LIKE semantics.
PartitionsProcDir.like() converts % but never converts the SQL _
single-character wildcard, and it leaves Java-regex metacharacters active
before String.matches(). Thus InvertedIndexStorageFormat LIKE 'V_' returns no
V1/V2/V3 rows, while 'V[123]' can match even though brackets should be
literals. Please use the standard Doris LIKE matcher (including escape
handling) and add wildcard/regex-literal tests for this column.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowTabletStorageFormatCommand.java:
##########
@@ -89,26 +92,46 @@ public ShowResultSet doRun(ConnectContext ctx, StmtExecutor
executor) throws Exc
if (result == null) {
throw new AnalysisException("get tablet data from backend:
" + be.getId() + "error.");
}
+ List<Long> invertedIndexV1Tablets =
result.isSetInvertedIndexV1Tablets()
+ ? result.getInvertedIndexV1Tablets() :
Lists.newArrayList();
+ List<Long> invertedIndexV2Tablets =
result.isSetInvertedIndexV2Tablets()
+ ? result.getInvertedIndexV2Tablets() :
Lists.newArrayList();
+ List<Long> invertedIndexV3Tablets =
result.isSetInvertedIndexV3Tablets()
+ ? result.getInvertedIndexV3Tablets() :
Lists.newArrayList();
if (verbose) {
+ Map<Long, String> storageFormats = new HashMap<>();
Review Comment:
[Performance] Please avoid sorting and triplicating the full tablet
inventory in verbose mode. The old path emitted the V1/V2 lists directly in
O(N); this path now boxes every ID into a base HashMap, another inverted-format
HashMap, and a TreeSet, adding O(N log N) work and substantial FE heap for
large backends without an ordering contract or test dependency. A single
inverted-format lookup plus the existing direct V1/V2 emission loops provides
the new column while keeping the previous complexity.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowPartitionsCommand.java:
##########
@@ -158,7 +159,8 @@ private void analyzeSubExpression(Expression subExpr)
throws AnalysisException {
// FILTER_LAST_CONSISTENCY_CHECK_TIME != 'abc'
if (subExpr instanceof ComparisonPredicate) {
- if (leftKey.equalsIgnoreCase(FILTER_PARTITION_NAME) ||
leftKey.equalsIgnoreCase(FILTER_STATE)) {
+ if (leftKey.equalsIgnoreCase(FILTER_PARTITION_NAME) ||
leftKey.equalsIgnoreCase(FILTER_STATE)
+ ||
leftKey.equalsIgnoreCase(FILTER_INVERTED_INDEX_STORAGE_FORMAT)) {
Review Comment:
[Blocking] Please finish validation for this new string column. First,
validate() only special-cases HMS, so MaxCompute and Paimon accept WHERE
InvertedIndexStorageFormat = 'V3' even though their schemas lack this column
and their handlers ignore filterMap; adding the name to TITLE_NAMES also
accepts ORDER BY here while those handlers sort by partition name. Second, the
internal path accepts InvertedIndexStorageFormat = 3 because it checks only
EqualTo; execution then calls Long.parseLong("V3") and leaks
NumberFormatException. Please scope the column to supported schemas, require a
string-like RHS, and add external-catalog and invalid-type negative tests.
##########
fe/fe-core/src/main/java/org/apache/doris/common/proc/PartitionsProcDir.java:
##########
@@ -651,6 +651,10 @@ private List<Pair<List<Comparable>, TRow>>
getPartitionInfosInrernal() throws An
partitionInfo.add(partition.getRowCount());
trow.addToColumnValue(new
TCell().setLongVal(partition.getRowCount()));
+ String invertedIndexStorageFormat =
olapTable.getInvertedIndexFileStorageFormat().name();
Review Comment:
[Blocking] Please resolve the legacy default before exposing it as the
partition's storage format. When replayed FE TableProperty metadata lacks
inverted_index_storage_format, this getter reports DEFAULT (or V2 when
TableProperty is null), but BE's backward-compatibility rule maps a missing
TabletSchemaPB field to physical V1. The same upgraded table can therefore show
DEFAULT/V2 here and V1 in SHOW TABLET STORAGE FORMAT, and filtering partitions
by V1 misses it. Please align the legacy effective value with the BE rule (or
report UNKNOWN explicitly) and add replay coverage for metadata without either
field.
##########
gensrc/thrift/BackendService.thrift:
##########
@@ -143,6 +143,9 @@ struct TDiskTrashInfo {
struct TCheckStorageFormatResult {
1: optional list<i64> v1_tablets;
2: optional list<i64> v2_tablets;
+ 3: optional list<i64> inverted_index_v1_tablets;
+ 4: optional list<i64> inverted_index_v2_tablets;
Review Comment:
[Performance] Please avoid doubling the full tablet-ID RPC payload for
summary mode. Every producer now puts each tablet ID once in a legacy
base-format list and again in a new inverted-format list, but this no-argument
RPC is called before the FE branches on verbose, and the non-verbose path uses
only the five list sizes. A summary for N tablets therefore still allocates,
serializes, transfers, and deserializes 2N IDs just to print counts,
potentially hitting the Thrift message limit on large backends. Please add a
count-oriented mode/response or a compact representation that preserves
rolling-upgrade compatibility, with scale coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/trees/plans/commands/ShowPartitionsCommand.java:
##########
@@ -158,7 +159,8 @@ private void analyzeSubExpression(Expression subExpr)
throws AnalysisException {
// FILTER_LAST_CONSISTENCY_CHECK_TIME != 'abc'
if (subExpr instanceof ComparisonPredicate) {
- if (leftKey.equalsIgnoreCase(FILTER_PARTITION_NAME) ||
leftKey.equalsIgnoreCase(FILTER_STATE)) {
+ if (leftKey.equalsIgnoreCase(FILTER_PARTITION_NAME) ||
leftKey.equalsIgnoreCase(FILTER_STATE)
+ ||
leftKey.equalsIgnoreCase(FILTER_INVERTED_INDEX_STORAGE_FORMAT)) {
Review Comment:
[Blocking] Please preserve every predicate in a valid conjunction on this
new column. analyzeSubExpression() recursively accepts all AND children, but
filterMap is keyed only by column name, so each leaf overwrites the previous
one. On a V3 table, WHERE InvertedIndexStorageFormat = 'V2' AND
InvertedIndexStorageFormat = 'V3' consequently evaluates only the last equality
and returns rows although the conjunction is false. Please either reject
repeated-column predicates or retain and evaluate their full conjunction, and
add contradictory-equality plus equality/LIKE coverage.
--
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]
