github-actions[bot] commented on code in PR #66475:
URL: https://github.com/apache/doris/pull/66475#discussion_r3826887253
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -3203,6 +3217,23 @@ public TInvertedIndexFileStorageFormat
getInvertedIndexFileStorageFormat() {
return tableProperty.getInvertedIndexFileStorageFormat();
}
+ public TInvertedIndexFileStorageFormat
getPartitionInvertedIndexFileStorageFormat() {
+ if (!Config.enable_partition_inverted_index_storage_format_rollout) {
Review Comment:
[P1] Do not gate durable format interpretation on local config. This switch
is mutable, master-only, default-false, and not journaled, while these getters
discard non-null TableProperty/PartitionInfo values whenever the local JVM sees
false. After enabling rollout and creating mixed partitions, a restart or
follower promotion therefore reports them as the legacy table format and can
pass that wrong value into schema-change/new-tablet creation. The same local
difference makes TRUNCATE replay record a different format from the
leader-created tablets. Please make activation durable/cluster-wide or, at
minimum, always honor already-persisted raw values and use the gate only for
accepting new rollout metadata; add restart/failover replay coverage.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/OlapTable.java:
##########
@@ -2261,6 +2266,8 @@ public Partition replacePartition(Partition newPartition,
partitionInfo.addPartition(newPartition.getId(), dataProperty,
replicaAlloc, isInMemory, isMutable);
}
+ partitionInfo.setInvertedIndexFileStorageFormat(newPartition.getId(),
Review Comment:
[P1] Preserve the old partition format before dropping it. replacePartition
fills the truncate recycle record manually but omits
invertedIndexFileStorageFormat, then dropPartition removes the only raw value.
For a V2 partition truncated after the future default changes to V3, recovering
the pre-truncate partition (after removing the replacement) therefore attaches
its unchanged V2 tablets/files to V3 catalog metadata. Please copy the old raw
format into RecyclePartitionParam (or reuse fillInfo) before dropPartition, and
cover truncate -> remove replacement -> recover.
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandler.java:
##########
@@ -145,6 +148,25 @@ public void updateTableProperties(Database db, String
tableName, Map<String, Str
List<Partition> partitions = Lists.newArrayList();
OlapTable olapTable = (OlapTable)
db.getTableOrMetaException(tableName, Table.TableType.OLAP);
UpdatePartitionMetaParam param = new UpdatePartitionMetaParam();
+ if
(properties.containsKey(PropertyAnalyzer.PROPERTIES_PARTITION_INVERTED_INDEX_STORAGE_FORMAT))
{
Review Comment:
[P1] Validate index capabilities for every affected physical format before
accepting this future default. A table with an existing SNII-only CommonGrams
index can set the future format to V3 here, after which the next partition is
created as V3 with an unsupported index definition. The inverse mismatch
remains in ADD/BUILD INDEX: those paths classify the whole table using
getInvertedIndexFileStorageFormat even though this PR executes schema/index
work per partition. Please validate ALTER/ADD against all extant plus future
formats and classify partition-selected BUILD from the selected partitions (or
reject unsplittable mixed selections).
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/PartitionsTableValuedFunction.java:
##########
@@ -85,7 +85,10 @@ public class PartitionsTableValuedFunction extends
MetadataTableValuedFunction {
new Column("ReplicaAllocation", ScalarType.createStringType()),
new Column("IsMutable",
ScalarType.createType(PrimitiveType.BOOLEAN)),
new Column("SyncWithBaseTables",
ScalarType.createType(PrimitiveType.BOOLEAN)),
- new Column("UnsyncTables", ScalarType.createStringType()));
+ new Column("UnsyncTables", ScalarType.createStringType()),
+ new Column("CommittedVersion",
ScalarType.createType(PrimitiveType.BIGINT)),
+ new Column("RowCount",
ScalarType.createType(PrimitiveType.BIGINT)),
+ new Column("InvertedIndexStorageFormat",
ScalarType.createStringType()));
Review Comment:
[P1] Keep the TVF schema aligned with the proc row. PartitionsProcDir emits
CommittedVersion, RowCount, BinlogSize, then InvertedIndexStorageFormat, but
this schema omits BinlogSize and derives source indices from its own ordinal.
MetadataGenerator therefore fetches proc cell 22 for the new field, returning a
human-readable binlog size instead of V2/V3/SNII. Add BinlogSize before this
column (or use an explicit source map) and add a value-level TVF assertion.
##########
be/src/cloud/cloud_rowset_writer.cpp:
##########
@@ -74,6 +74,10 @@ Status CloudRowsetWriter::init(const RowsetWriterContext&
rowset_writer_context)
_rowset_meta->set_newest_write_timestamp(_context.newest_write_timestamp);
}
_rowset_meta->set_tablet_schema(_context.tablet_schema);
+ if (_context.persist_inverted_index_storage_format &&
Review Comment:
[P1] Propagate the source format through snapshot restore.
CloudSnapshotMgr::_create_rowset_meta calls RowsetFactory directly without
setting inverted_index_storage_format or persist_inverted_index_storage_format,
so this new conditional leaves the restored rowset field absent.
commit_restore_job subsequently detaches the embedded schema into the
first-writer-wins (index_id,schema_version) KV. A restored V3/SNII rowset can
then be reopened with another partition's V2 shared schema even though its
copied files retain the original format. Please set both context fields from
the source rowset/schema and add a mixed-format restore test.
##########
be/src/storage/tablet/tablet_meta.cpp:
##########
@@ -823,7 +827,11 @@ void TabletMeta::init_from_pb(const TabletMetaPB&
tablet_meta_pb) {
// init _schema
TabletSchemaSPtr schema = std::make_shared<TabletSchema>();
- schema->init_from_pb(tablet_meta_pb.schema());
+ TabletSchemaPB schema_pb = tablet_meta_pb.schema();
Review Comment:
[P1] Enforce or document the old-BE activation precondition. These top-level
overrides are the only place a BE distinguishes two physical formats sharing
one schema-KV key; a pre-change BE ignores the unknown fields and uses
whichever format the shared schema contains. The default-false switch can serve
as a manual fence only if operators wait until every serving BE supports these
fields, but the dynamic setter performs no capability/version check and the PR
adds no documented/tested activation order. Enabling it while an old BE still
serves lets that BE read or write a new V3/SNII partition as V2. Please enforce
the all-BEs-supported precondition, or make the activation contract explicit
and test it.
##########
fe/fe-core/src/main/java/org/apache/doris/catalog/PartitionInfo.java:
##########
@@ -457,6 +482,13 @@ public void resetPartitionIdForRestore(
if (!isSinglePartitioned) {
idToItem.put(newPartId, origIdToItem.get(origPartId));
}
+ if (origIdToInvertedIndexFileStorageFormat != null) {
+ TInvertedIndexFileStorageFormat invertedIndexFileStorageFormat
=
+ origIdToInvertedIndexFileStorageFormat.get(origPartId);
+ if (invertedIndexFileStorageFormat != null) {
+ idToInvertedIndexFileStorageFormat.put(newPartId,
invertedIndexFileStorageFormat);
Review Comment:
[P1] Preserve this map for selected-partition restore too.
resetPartitionIdForRestore handles whole-table ID remapping, but RestoreJob's
live and replay selected-partition paths add the restored partition without
copying remotePartitionInfo's raw format. Restoring a V2 partition into a table
whose current default is V3 therefore leaves FE reporting/using V3 for
unchanged V2 snapshot files, including subsequent per-partition schema-change
tablet creation. Please copy the source value into destination PartitionInfo in
both selected-restore publication paths and add mixed-format
selected-restore/replay coverage.
##########
fe/fe-core/src/test/java/org/apache/doris/cloud/alter/CloudSchemaChangeHandlerTest.java:
##########
@@ -437,6 +440,94 @@ public void
testNotifyBackendsToSyncTabletMetaReturnsWhenBackendDiscoveryFails()
Mockito.verifyNoInteractions(backendServiceProxy);
}
+ @Test
+ public void
testUpdatePartitionInvertedIndexStorageFormatDoesNotScanPartitions() throws
Exception {
+ Database database = Mockito.mock(Database.class);
+ OlapTable table = Mockito.mock(OlapTable.class);
+ Env env = Mockito.mock(Env.class);
+ Map<String, String> properties = new HashMap<>();
+
properties.put(PropertyAnalyzer.PROPERTIES_PARTITION_INVERTED_INDEX_STORAGE_FORMAT,
"SNII");
+
+ Mockito.when(database.getTableOrMetaException("tbl",
Table.TableType.OLAP)).thenReturn(table);
+
+ boolean previousRollout =
Config.enable_partition_inverted_index_storage_format_rollout;
+ try (MockedStatic<Config> config = Mockito.mockStatic(Config.class,
Mockito.CALLS_REAL_METHODS);
+ MockedStatic<Env> envStatic = Mockito.mockStatic(Env.class);
+ MockedStatic<DynamicPartitionUtil> dynamicPartitionUtil =
+ Mockito.mockStatic(DynamicPartitionUtil.class)) {
+ Config.enable_partition_inverted_index_storage_format_rollout =
true;
+ config.when(Config::isCloudMode).thenReturn(true);
+ envStatic.when(Env::getCurrentEnv).thenReturn(env);
+
+ new CloudSchemaChangeHandler().updateTableProperties(database,
"tbl", properties);
+
+ Mockito.verify(env).modifyTableProperties(database, table,
properties);
+ Mockito.verify(table, Mockito.never()).getAllPartitions();
Review Comment:
[P2] This test does not exercise the scan it claims to exclude. Env is
mocked, so modifyTableProperties never reaches its real unconditional
table.getPartitions loop, which runs under the table write lock and rewrites
in-memory/storage-policy entries even when this future-only property is the
sole change. On a large auto-partitioned table the new ALTER remains
O(partitions). Please skip that generic propagation when no partition-wide
property changed and make the test execute or directly cover the real Env path.
--
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]
