github-actions[bot] commented on code in PR #64522:
URL: https://github.com/apache/doris/pull/64522#discussion_r3434473085
##########
regression-test/suites/ann_index_p0/create_ann_index_test.groovy:
##########
@@ -403,7 +403,7 @@ suite("create_ann_index_test") {
DISTRIBUTED BY HASH(id) BUCKETS 2
PROPERTIES (
"replication_num" = "1",
- "inverted_index_storage_format" = "V1"
+ "inverted_index_storage_format" = "V2"
Review Comment:
Changing this table to V2 makes the surrounding `test { ... exception "ANN
index is not supported in index format V1" }` fail: V2 is the supported ANN
index format, so `CREATE TABLE` should succeed and the expected V1 exception is
no longer thrown. If this case is meant to verify the new V1 ban, keep the
property as V1 and expect the deprecation error; otherwise remove or rewrite
this negative case.
##########
regression-test/suites/inverted_index_p0/storage_format/test_inverted_index_v1_deprecated.groovy:
##########
@@ -0,0 +1,121 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+
+suite("test_inverted_index_v1_deprecated", "p0") {
+ // AC1: explicit v1 (lowercase) must be rejected
+ test {
+ sql """
+ CREATE TABLE test_v1_rejected (
+ k INT,
+ v STRING,
+ INDEX idx_v (v) USING INVERTED
+ ) ENGINE=OLAP DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "inverted_index_storage_format" = "v1"
+ )
+ """
+ exception "Inverted index V1 is deprecated and no longer allowed for
new index creation."
+ }
+
+ // AC1 (case-insensitive): uppercase V1 must also be rejected
+ test {
+ sql """
+ CREATE TABLE test_v1_rejected_upper (
+ k INT,
+ v STRING,
+ INDEX idx_v (v) USING INVERTED
+ ) ENGINE=OLAP DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "inverted_index_storage_format" = "V1"
+ )
+ """
+ exception "Inverted index V1 is deprecated and no longer allowed for
new index creation."
+ }
+
+ // AC2: no format specified -> default succeeds and format is not V1
+ // AC3: ALTER TABLE ADD INDEX on default table succeeds
+ try {
+ sql "DROP TABLE IF EXISTS test_v1_deprecated_default"
+ sql """
+ CREATE TABLE test_v1_deprecated_default (
+ k INT,
+ v STRING
+ ) ENGINE=OLAP DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1"
+ )
+ """
+ def showCreate = sql "SHOW CREATE TABLE test_v1_deprecated_default"
+ assertTrue(showCreate.size() > 0)
+
assertFalse(showCreate[0][1].contains("\"inverted_index_storage_format\" =
\"V1\""))
+
+ sql "ALTER TABLE test_v1_deprecated_default ADD INDEX idx_v (v) USING
INVERTED"
+ def showCreateAfter = sql "SHOW CREATE TABLE
test_v1_deprecated_default"
+ assertTrue(showCreateAfter[0][1].contains("idx_v"))
+
assertFalse(showCreateAfter[0][1].contains("\"inverted_index_storage_format\" =
\"V1\""))
+ } finally {
+ sql "DROP TABLE IF EXISTS test_v1_deprecated_default"
+ }
+
+ // AC4: existing V1 table metadata load and read/write must not be
affected.
+ // Uses a deterministic fixture: temporarily unlock V1 creation via admin
config,
+ // seed the legacy table, verify reads/writes/deletes, then clean up.
+ try {
+ sql "DROP TABLE IF EXISTS test_v1_legacy_compat"
+ sql "ADMIN SET FRONTEND CONFIG ('allow_inverted_index_v1_creation' =
'true')"
+ sql """
+ CREATE TABLE test_v1_legacy_compat (
+ k INT,
+ v STRING,
+ INDEX idx_v (v) USING INVERTED
+ ) ENGINE=OLAP DUPLICATE KEY(k)
+ DISTRIBUTED BY HASH(k) BUCKETS 1
+ PROPERTIES (
+ "replication_allocation" = "tag.location.default: 1",
+ "inverted_index_storage_format" = "v1"
+ )
+ """
+ sql "ADMIN SET FRONTEND CONFIG ('allow_inverted_index_v1_creation' =
'false')"
+
+ def showCreate = sql "SHOW CREATE TABLE test_v1_legacy_compat"
+
assertTrue(showCreate[0][1].contains("\"inverted_index_storage_format\" =
\"V1\""))
+
+ // Verify basic DML works on a V1 table (tests metadata compat, not BE
index reads)
Review Comment:
This fixture still leaves a new-index creation path open. After creating
this V1 table and setting `allow_inverted_index_v1_creation` back to false,
`ALTER TABLE test_v1_legacy_compat ADD INDEX idx_k (k) USING INVERTED` goes
through `SchemaChangeHandler.processAddIndex`, which uses
`olapTable.getInvertedIndexFileStorageFormat()` and never calls the new
`PropertyAnalyzer` rejection. Because the table format is V1, the added index
is still created as V1. That contradicts the PR goal and the stated ALTER TABLE
coverage; please reject/remap add-index on V1 tables and add this as an
expected-error case.
##########
regression-test/suites/inverted_index_p0/index_format_v2/test_index_change_format.groovy:
##########
@@ -46,6 +46,6 @@ suite("test_index_change_format", "inverted_index_format_v2"){
}
}
- createAndAlterTable("test_index_change_format_v1", "V1", "V2")
- createAndAlterTable("test_index_change_format_v2", "V2", "V1")
+ // createAndAlterTable("test_index_change_format_v1", "V1", "V2")
+ // createAndAlterTable("test_index_change_format_v2", "V2", "V1")
Review Comment:
Commenting out both calls makes this suite a no-op. The V2 -> V1 case is
still relevant after this PR because a normal V2 table can still be created,
and attempts to alter its `inverted_index_storage_format` to V1 should remain
rejected. Please keep that expected-failure case active; only the V1 -> V2
setup needs a legacy fixture or removal.
##########
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:
With the format loop reduced to only `v2`, the
`IndexFileWriter::write_v1_out_dir_createOutput_nullptr` debug point that
remains in `debug_points` is never hit; that debug point exists only in
`be/src/storage/index/index_storage_format_v1.cpp`. The test now runs the
normal V2 insert/select path for that entry and reports success without
exercising the intended failure branch. Please either remove the V1-only debug
point from this V2-only suite or keep a deterministic V1 fixture for that
specific fault.
##########
regression-test/suites/variant_p0/predefine/test_predefine_ddl.groovy:
##########
@@ -282,26 +282,26 @@ suite("test_predefine_ddl", "p0") {
exception("invalid INVERTED index")
}
- test {
- sql "DROP TABLE IF EXISTS ${tableName}"
- sql """CREATE TABLE ${tableName} (
- `id` bigint NULL,
- `var` string NULL,
- INDEX idx_ab (var) USING INVERTED PROPERTIES("parser"="unicode",
"support_phrase" = "true") COMMENT '',
- INDEX idx_ab_2 (var) USING INVERTED
- ) ENGINE=OLAP DUPLICATE KEY(`id`) DISTRIBUTED BY HASH(`id`)
- BUCKETS 1 PROPERTIES ( "replication_allocation" =
"tag.location.default: 1", "disable_auto_compaction" = "true",
"inverted_index_storage_format" = "v1")"""
- exception("column: var cannot have multiple inverted indexes with file
storage format: V1")
- }
+ // test {
+ // sql "DROP TABLE IF EXISTS ${tableName}"
+ // sql """CREATE TABLE ${tableName} (
+ // `id` bigint NULL,
+ // `var` string NULL,
+ // INDEX idx_ab (var) USING INVERTED
PROPERTIES("parser"="unicode", "support_phrase" = "true") COMMENT '',
+ // INDEX idx_ab_2 (var) USING INVERTED
+ // ) ENGINE=OLAP DUPLICATE KEY(`id`) DISTRIBUTED BY HASH(`id`)
+ // BUCKETS 1 PROPERTIES ( "replication_allocation" =
"tag.location.default: 1", "disable_auto_compaction" = "true",
"inverted_index_storage_format" = "v1")"""
+ // exception("column: var cannot have multiple inverted indexes with
file storage format: V1")
+ // }
- test {
- sql """CREATE TABLE ${tableName} (
- `id` bigint NULL,
- `var` variant <'c' :char(10)> NULL
- ) ENGINE=OLAP DUPLICATE KEY(`id`) DISTRIBUTED BY HASH(`id`)
- BUCKETS 1 PROPERTIES ( "replication_allocation" =
"tag.location.default: 1", "disable_auto_compaction" = "true",
"inverted_index_storage_format" = "v1")"""
- exception("VARIANT unsupported sub-type: char(10)")
- }
+ // test {
+ // sql """CREATE TABLE ${tableName} (
+ // `id` bigint NULL,
+ // `var` variant <'c' :char(10)> NULL
+ // ) ENGINE=OLAP DUPLICATE KEY(`id`) DISTRIBUTED BY HASH(`id`)
+ // BUCKETS 1 PROPERTIES ( "replication_allocation" =
"tag.location.default: 1", "disable_auto_compaction" = "true",
"inverted_index_storage_format" = "v1")"""
+ // exception("VARIANT unsupported sub-type: char(10)")
Review Comment:
This expected-error case is not about the inverted-index file format; it
validates that `variant <'c' :char(10)>` is rejected. Commenting it out removes
unrelated variant subtype coverage. Please re-enable it with default/V2 table
properties so the `VARIANT unsupported sub-type: char(10)` assertion remains
active.
--
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]
