github-actions[bot] commented on code in PR #67342:
URL: https://github.com/apache/doris/pull/67342#discussion_r3895073837
##########
fe/fe-core/src/main/java/org/apache/doris/alter/SchemaChangeHandler.java:
##########
@@ -2816,6 +2816,13 @@ private boolean processAddIndex(CreateIndexClause
alterClause, OlapTable olapTab
AnnIndexPropertiesChecker.checkProperties(indexDef.getProperties());
}
+ if (indexDef.getIndexType() == IndexType.INVERTED
+ && olapTable.getInvertedIndexFileStorageFormat() ==
TInvertedIndexFileStorageFormat.V1) {
Review Comment:
[P1] Treat the legacy DEFAULT sentinel as V1
Tables created before this property existed deserialize its missing FE value
as `DEFAULT`, while their physical tablet schema defaults to V1. This
exact-enum check lets ADD INVERTED INDEX through, and ADD ANN also passes its
own exact-V1 check. With the default light-index-change path, the format-free
task makes BE copy that legacy schema and build against V1 (including ANN,
which is unsupported there); with light change disabled, `DEFAULT` is instead
mapped to V3 and silently upgrades the table. Please resolve/reject the
effective legacy V1 value before both index types while leaving
replay/restore/BUILD of committed indexes unchanged.
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -1283,25 +1283,23 @@ public static TInvertedIndexFileStorageFormat
analyzeInvertedIndexFileStorageFor
invertedIndexFileStorageFormat =
properties.get(PROPERTIES_INVERTED_INDEX_STORAGE_FORMAT);
properties.remove(PROPERTIES_INVERTED_INDEX_STORAGE_FORMAT);
} else {
- if (Config.inverted_index_storage_format.equalsIgnoreCase("V1")) {
- return TInvertedIndexFileStorageFormat.V1;
- } else if
(Config.inverted_index_storage_format.equalsIgnoreCase("V2")) {
+ if (Config.inverted_index_storage_format.equalsIgnoreCase("V2")) {
return TInvertedIndexFileStorageFormat.V2;
} else {
return TInvertedIndexFileStorageFormat.V3;
}
}
if (invertedIndexFileStorageFormat.equalsIgnoreCase("v1")) {
- return TInvertedIndexFileStorageFormat.V1;
+ throw new AnalysisException(
Review Comment:
[P1] Migrate the remaining V1 success-path suite
This unconditional rejection makes
`regression-test/suites/load_p2/test_single_replica_load.groovy` fail on every
non-cloud P2 run: its ordinary CREATE TABLE still sets
`inverted_index_storage_format = "V1"`, has no expected-error wrapper, and then
stream-loads the assumed table. The temporary
`enable_inverted_index_v1_for_variant` flag cannot bypass this analyzer. Please
migrate that suite to V2 (or provision a pre-existing V1 fixture if it is
intended to test upgrade compatibility) along with the other suites in this PR.
##########
regression-test/suites/fault_injection_p0/test_skip_index_compaction_fault_injection.groovy:
##########
@@ -174,12 +153,6 @@ suite("test_skip_index_compaction_fault_injection",
"nonConcurrent") {
has_update_be_config = true
check_config.call("inverted_index_compaction_enable", "true");
- try {
-
GetDebugPoint().enableDebugPointForAllBEs("Compaction::open_inverted_index_file_reader")
- run_test.call(tableName1)
- } finally {
-
GetDebugPoint().disableDebugPointForAllBEs("Compaction::open_inverted_index_file_reader")
- }
try {
Review Comment:
[P2] Preserve reader-open compaction fault coverage
Removing the V1 setup also removes the only
`Compaction::open_inverted_index_file_reader` run, but that hook is in the
format-independent compaction loop immediately before the retained writer hook
and is reachable with V2. This leaves the source-reader failure path untested.
Please run the reader hook against a fresh V2 table before the writer-hook case.
##########
regression-test/suites/fault_injection_p0/test_write_inverted_index_exception_fault_injection.groovy:
##########
@@ -229,7 +229,7 @@
suite("test_write_inverted_index_exception_fault_injection", "nonConcurrent") {
"InvertedIndexColumnWriter::create_unsupported_type_for_inverted_index"
]
- def inverted_index_storage_format = ["v1", "v2"]
+ def inverted_index_storage_format = ["v2"]
Review Comment:
[P2] Drop or replace the V1-only debug point
After reducing this matrix to V2, `debug_points` still contains
`IndexFileWriter::write_v1_out_dir_createOutput_nullptr`. That hook exists only
in `IndexStorageFormatV1::create_output_stream`; the V2 path never executes it.
Because this loop also accepts a normal insert as success, that iteration now
passes without injecting any failure. Please remove it or add and assert an
equivalent V2 create-output fault so the suite does not silently claim coverage
it no longer has.
--
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]
