xiaokang commented on code in PR #16371:
URL: https://github.com/apache/doris/pull/16371#discussion_r1094499716
##########
gensrc/thrift/PlanNodes.thrift:
##########
@@ -574,6 +574,7 @@ struct TOlapScanNode {
11: optional bool enable_unique_key_merge_on_write
12: optional TPushAggOp push_down_agg_type_opt
13: optional bool use_topn_opt
+ 16: optional list<Descriptors.TOlapTableIndex> indexes_desc
Review Comment:
seq 14
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -1767,24 +1773,54 @@ public int getAsInt() {
return;
}
lightSchemaChange = false;
+
+ CreateIndexClause createIndexClause = (CreateIndexClause)
alterClause;
+ if (createIndexClause.getIndexDef().isInvertedIndex()) {
+ alterInvertedIndexes.add(createIndexClause.getIndex());
+ isDropInvertedIndex = false;
+ lightSchemaChangeWithInvertedIndex = true;
+ }
} else if (alterClause instanceof DropIndexClause) {
if (processDropIndex((DropIndexClause) alterClause,
olapTable, newIndexes)) {
return;
}
lightSchemaChange = false;
+
+ DropIndexClause dropIndexClause = (DropIndexClause)
alterClause;
+ List<Index> existedIndexes = olapTable.getIndexes();
+ Index found = null;
+ for (Index existedIdx : existedIndexes) {
+ if
(existedIdx.getIndexName().equalsIgnoreCase(dropIndexClause.getIndexName())) {
+ found = existedIdx;
+ break;
+ }
+ }
+ IndexDef.IndexType indexType = found.getIndexType();
+ if (indexType == IndexType.INVERTED) {
Review Comment:
more generic isIndependent or something else
##########
gensrc/thrift/Types.thrift:
##########
@@ -205,7 +205,8 @@ enum TTaskType {
STORAGE_MEDIUM_MIGRATE_V2,
NOTIFY_UPDATE_STORAGE_POLICY, // deprecated
PUSH_COOLDOWN_CONF,
- PUSH_STORAGE_POLICY
+ PUSH_STORAGE_POLICY,
+ ALTER_INVERTED_INDEX
Review Comment:
It's better to use a generic name for further use by other index
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -1767,24 +1773,54 @@ public int getAsInt() {
return;
}
lightSchemaChange = false;
+
+ CreateIndexClause createIndexClause = (CreateIndexClause)
alterClause;
+ if (createIndexClause.getIndexDef().isInvertedIndex()) {
Review Comment:
more generic isIndependent or something else
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2367,4 +2403,212 @@ public void
replayModifyTableAddOrDropColumns(TableAddOrDropColumnsInfo info) th
olapTable.writeUnlock();
}
}
+
+ // the invoker should keep table's write lock
+ public void modifyTableAddOrDropInvertedIndices(Database db, OlapTable
olapTable,
+ Map<Long, LinkedList<Column>> indexSchemaMap, Map<String, String>
propertyMap,
+ List<Index> indexes, List<Index> alterInvertedIndexes,
+ boolean isDropInvertedIndex, List<Index> oriIndexes,
+ long jobId, boolean isReplay) throws UserException {
+ LOG.info("begin to modify table's meta for add or drop inverted index.
table: {}, job: {}",
+ olapTable.getName(), jobId);
+ LOG.info("indexSchemaMap:{}, indexes:{}, alterInvertedIndexes:{},
isDropInvertedIndex: {}",
+ indexSchemaMap, indexes, alterInvertedIndexes,
isDropInvertedIndex);
+ if (olapTable.getState() == OlapTableState.ROLLUP) {
Review Comment:
It's covered by the following checkState.
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2367,4 +2403,212 @@ public void
replayModifyTableAddOrDropColumns(TableAddOrDropColumnsInfo info) th
olapTable.writeUnlock();
}
}
+
+ // the invoker should keep table's write lock
+ public void modifyTableAddOrDropInvertedIndices(Database db, OlapTable
olapTable,
+ Map<Long, LinkedList<Column>> indexSchemaMap, Map<String, String>
propertyMap,
+ List<Index> indexes, List<Index> alterInvertedIndexes,
+ boolean isDropInvertedIndex, List<Index> oriIndexes,
+ long jobId, boolean isReplay) throws UserException {
+ LOG.info("begin to modify table's meta for add or drop inverted index.
table: {}, job: {}",
+ olapTable.getName(), jobId);
+ LOG.info("indexSchemaMap:{}, indexes:{}, alterInvertedIndexes:{},
isDropInvertedIndex: {}",
+ indexSchemaMap, indexes, alterInvertedIndexes,
isDropInvertedIndex);
+ if (olapTable.getState() == OlapTableState.ROLLUP) {
+ throw new DdlException("Table[" + olapTable.getName() + "]'s is
doing ROLLUP job");
+ }
+
+ // for now table's state can only be NORMAL
+ Preconditions.checkState(olapTable.getState() ==
OlapTableState.NORMAL, olapTable.getState().name());
+
+ boolean hasInvertedIndexChange = false;
+ if (!alterInvertedIndexes.isEmpty()) {
+ hasInvertedIndexChange = true;
+ }
+
+ // begin checking each table
+ // ATTN: DO NOT change any meta in this loop
+ Map<Long, List<Column>> changedIndexIdToSchema = Maps.newHashMap();
+ for (Long alterIndexId : indexSchemaMap.keySet()) {
+ if (!hasInvertedIndexChange) {
Review Comment:
hasInvertedIndexChange is not changed in loop, so should be check before loop
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeJobV2.java:
##########
@@ -337,6 +366,10 @@ protected void runPendingJob() throws AlterCancelException
{
}
private void addShadowIndexToCatalog(OlapTable tbl) {
+ if (invertedIndexChange) {
Review Comment:
add comment
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeJobV2.java:
##########
@@ -499,6 +590,13 @@ protected void runRunningJob() throws AlterCancelException
{
LOG.info("schema change tasks not finished. job: {}", jobId);
List<AgentTask> tasks =
schemaChangeBatchTask.getUnfinishedTasks(2000);
for (AgentTask task : tasks) {
+ if (invertedIndexChange) {
+ // TODO(wy): more elegant implementation
Review Comment:
Is this TODO must be fixed?
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2367,4 +2403,208 @@ public void
replayModifyTableAddOrDropColumns(TableAddOrDropColumnsInfo info) th
olapTable.writeUnlock();
}
}
+
+ // the invoker should keep table's write lock
+ public void modifyTableAddOrDropInvertedIndices(Database db, OlapTable
olapTable,
Review Comment:
modifyTableAddOrDropInvertedIndices is almost the same as
modifyTableAddOrDropColumns, can we try to merge them to reuse code as much as
possible.
##########
fe/fe-core/src/main/java/org/apache/doris/planner/OlapScanNode.java:
##########
@@ -1095,9 +1098,15 @@ protected void toThrift(TPlanNode msg) {
}
}
+ for (Index index : olapTable.getIndexes()) {
+ TOlapTableIndex tIndex = index.toThrift();
+ indexDesc.add(tIndex);
+ }
+
msg.node_type = TPlanNodeType.OLAP_SCAN_NODE;
msg.olap_scan_node = new TOlapScanNode(desc.getId().asInt(),
keyColumnNames, keyColumnTypes, isPreAggregation);
msg.olap_scan_node.setColumnsDesc(columnsDesc);
+ msg.olap_scan_node.setIndexesDesc(indexDesc);
Review Comment:
Adding indexDesc to thrift will add load for query rpc, can we limit it to
query with inverted index?
--
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]
