[feature](fe) Show inverted index storage format by partition ID - #65776
hoshinojyunn wants to merge 1 commit into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
hoshinojyunn
changed the title
|
run buildall |
| ? result.getInvertedIndexV3Tablets() : Lists.newArrayList(); | ||
| if (verbose) { | ||
| Map<Long, String> storageFormats = new HashMap<>(); | ||
| for (long tabletId : result.getV1Tablets()) { |
There was a problem hiding this comment.
[Blocking] Please preserve the distinction between an unset optional field and a known-empty list.
After this PR enables the command in cloud mode, a new FE can call an old Cloud BE whose BaseBackendService::check_storage_format() returns a non-null result with v1_tablets and v2_tablets unset. Thrift getters return null here, so this loop and the summary .size() calls throw NullPointerException.
An old local BE sets v1_tablets/v2_tablets but does not know the new inverted-index fields. Converting those absent fields to empty lists also makes the non-verbose result report 0, although the format count is unknown.
Please guard all optional lists with isSet..., retain whether the inverted-index fields were present, and return UNKNOWN or a clear unsupported-version error for absent data. Please also add mixed-version tests for both response shapes: old local BE (only v1/v2 set) and old Cloud BE (all lists unset).
|
/review |
Cloud UT Coverage ReportIncrement line coverage Increment coverage report
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes.
The PR implements the happy path for fresh internal V2/V3 tables and both current BE producers set the append-only Thrift fields, but it is not safe to merge yet. The inline comments cover a Cloud tablet-map data race, cache-only Cloud inventory that silently omits valid tablets, incomplete filter/catalog/type validation, repeated-column predicates that are silently dropped, legacy FE/BE default disagreement, incorrect SQL LIKE semantics, a doubled summary RPC payload, and avoidable O(N log N) FE work. The existing mixed-version optional-field thread remains blocking and was not duplicated.
Critical checkpoint conclusions:
- Goal and focus: the new columns and current-version local/Cloud happy paths are wired, but rolling-upgrade observability is not correct for legacy metadata, old BE responses, or non-resident Cloud tablets. There was no additional user-provided review focus.
- Scope and parallel paths: the code is otherwise focused and the local BE classification is consistent for current V1/V2/V3 enums. MaxCompute/Paimon validation is a missed parallel path; the partitions TVF's independent leading-20-column projection remains safe because the new proc cell is appended.
- Concurrency and lifecycle: the new Cloud RPC reaches an unlocked unordered-map size read concurrent with cache insertion/eviction, and its result depends on volatile cache materialization/restart/eviction lifecycle. No new thread, static-initialization dependency, ownership cycle, or lock-order issue was found beyond that race.
- Compatibility and persistence: new Thrift IDs are append-only and current producers set all presence bits. New-FE/old-BE absence handling is already covered by discussion r3627008129. Separately, replayed FE property defaults disagree with BE's V1 tablet-schema compatibility rule.
- Error handling and semantics: external predicates/orders can be accepted and ignored, a numeric RHS leaks NumberFormatException, repeated predicates on the new column overwrite one another, and the newly exposed LIKE route does not implement SQL wildcard semantics.
- Performance: the response unconditionally duplicates every tablet ID even for count-only output, while verbose output also replaces linear emission with two hash maps plus a TreeSet, adding avoidable wire/deserialization cost, O(N log N) work, and significant boxed heap on large backends.
- Transactions, data correctness, configuration, and observability: this is a read-only observability change with no transaction, visible-version, delete-bitmap, data-write, EditLog, or configuration mutation. The main correctness risk is misleading or incomplete reported state.
- Tests: the added tests cover fresh range/list/auto/dynamic V2/V3 equality happy paths. Missing coverage includes mixed versions, legacy replay/V1, empty/restarted/evicted Cloud tablets, concurrent cache mutation, external-catalog rejection, invalid RHS types, repeated-column conjunctions, LIKE wildcards, and large summary/verbose inputs. Per the review-run contract, no build or test was executed locally; conclusions are from static code/control-flow review.
Review status: complete for the current head after full changed-file coverage, independent main verification, duplicate suppression, and normal plus risk-focused convergence.
| } | ||
|
|
||
| void CloudBackendService::check_storage_format(TCheckStorageFormatResult& result) { | ||
| for (const auto& tablet : _engine.tablet_mgr().get_all_tablet()) { |
There was a problem hiding this 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.
|
|
||
| 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()); |
There was a problem hiding this 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.
| partitionInfo.add(partition.getRowCount()); | ||
| trow.addToColumnValue(new TCell().setLongVal(partition.getRowCount())); | ||
|
|
||
| String invertedIndexStorageFormat = olapTable.getInvertedIndexFileStorageFormat().name(); |
There was a problem hiding this 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.
| 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)) { |
There was a problem hiding this 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.
| 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)) { |
There was a problem hiding this 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.
| 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)) { |
There was a problem hiding this 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.
| List<Long> invertedIndexV3Tablets = result.isSetInvertedIndexV3Tablets() | ||
| ? result.getInvertedIndexV3Tablets() : Lists.newArrayList(); | ||
| if (verbose) { | ||
| Map<Long, String> storageFormats = new HashMap<>(); |
There was a problem hiding this 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.
| struct TCheckStorageFormatResult { | ||
| 1: optional list<i64> v1_tablets; | ||
| 2: optional list<i64> v2_tablets; | ||
| 3: optional list<i64> inverted_index_v1_tablets; |
There was a problem hiding this 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 UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 29489 ms |
TPC-DS: Total hot run time: 177411 ms |
ClickBench: Total hot run time: 25.6 s |
BE Regression && UT Coverage ReportIncrement line coverage Increment coverage report
|
morrySnow
changed the title
f803007 to
f24f338
Compare
### What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary: SHOW PARTITION <partition_id> did not expose the inverted-index storage format configured for the table that owns the partition. Add InvertedIndexStorageFormat to the SHOW PARTITION result, while keeping SHOW PARTITIONS unchanged so it continues to provide the PartitionId used by the new inspection path. Resolve absent or DEFAULT persisted values to V1, matching the legacy BE tablet-schema behavior.
### Release note
SHOW PARTITION <partition_id> now displays InvertedIndexStorageFormat.
### Check List (For Author)
- Test:
- Regression test: regression-test/suites/cloud_p0/test_partition_cloud_inverted_index_format.groovy
- Behavior changed: Yes (SHOW PARTITION <partition_id> displays the effective inverted-index storage format)
- Does this need documentation: No
f24f338 to
c3ba54f
Compare
hoshinojyunn
changed the title
3 participants
What problem does this PR solve?
Issue Number: None
Related PR: None
Problem Summary:
SHOW PARTITION <partition_id>did not expose a table's inverted-index storage format. This made it difficult to inspect the configured format for a specific partition after obtaining its ID.This change adds
InvertedIndexStorageFormatonly toSHOW PARTITION <partition_id>.SHOW PARTITIONS FROM <table>intentionally retains its existing result columns; it is used to obtainPartitionId, which can then be supplied toSHOW PARTITION. Tables without a persisted format reportV1, matching the legacy BE tablet-schema behavior.Example:
SHOW PARTITIONS FROM events_full;SHOW PARTITION 123456;Release note
SHOW PARTITION <partition_id>now displaysInvertedIndexStorageFormat.Check List (For Author)
Test
regression-test/suites/cloud_p0/test_partition_cloud_inverted_index_format.groovy,regression-test/suites/query_p0/show/test_partition_local_inverted_index_format.groovyBehavior changed:
SHOW PARTITION <partition_id>displays the effective inverted-index storage format.Does this need documentation?
Check List (For Reviewer who merge this PR)