github-actions[bot] commented on code in PR #64522:
URL: https://github.com/apache/doris/pull/64522#discussion_r3425468609
##########
fe/fe-core/src/main/java/org/apache/doris/common/util/PropertyAnalyzer.java:
##########
@@ -1270,7 +1270,9 @@ public static TInvertedIndexFileStorageFormat
analyzeInvertedIndexFileStorageFor
}
if (invertedIndexFileStorageFormat.equalsIgnoreCase("v1")) {
- return TInvertedIndexFileStorageFormat.V1;
+ throw new AnalysisException(
Review Comment:
This still leaves a new-table path that can create V1.
`CreateTableInfo.validate()` and `InternalCatalog.createOlapTable()` call this
method for new OLAP tables; when the property is absent, or when the user
specifies `"default"`, the method still returns
`TInvertedIndexFileStorageFormat.V1` if the mutable FE config
`Config.inverted_index_storage_format` is set to `V1` in the branches
above/below this hunk. That means an upgraded cluster with that config, or a
user using `"default"` while the config is V1, can still create a new V1
inverted index without hitting this error. If the goal is to block new V1
creation while keeping existing metadata readable, please reject or remap V1 in
the default/config path too and leave legacy loading through
`TableProperty.buildInvertedIndexFileStorageFormat()`.
##########
fe/fe-core/src/test/java/org/apache/doris/common/PropertyAnalyzerTest.java:
##########
@@ -302,8 +302,10 @@ public void testAnalyzeInvertedIndexFileStorageFormat()
throws AnalysisException
Map<String, String> propertiesWithV1 = new HashMap<>();
propertiesWithV1.put(PropertyAnalyzer.PROPERTIES_INVERTED_INDEX_STORAGE_FORMAT,
"v1");
- result =
PropertyAnalyzer.analyzeInvertedIndexFileStorageFormat(propertiesWithV1);
- Assertions.assertEquals(TInvertedIndexFileStorageFormat.V1,
result);
+ Assertions.assertThrows(AnalysisException.class,
Review Comment:
`assertThrows`' third argument is only the assertion failure message; it
does not check the thrown exception text. This test currently verifies only
that some `AnalysisException` is thrown. Please capture the returned exception
and assert its message if this user-facing error is part of the contract.
##########
regression-test/suites/inverted_index_p0/storage_format/test_inverted_index_v1_deprecated.groovy:
##########
@@ -0,0 +1,107 @@
+// 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: CREATE TABLE with explicit V1 must fail
+ test {
Review Comment:
This new rejection also makes existing p0 regression suites fail because
they still create V1 tables as normal setup, not as expected-error cases.
Examples include
`regression-test/suites/inverted_index_p0/load/test_insert.groovy` calling
`test.call("V1")`, the `index_change/test_index_change_*.groovy` suites,
`test_multi_analyzer_index_version.groovy`, and the `index_compaction/*` V1
cases. Those tests will now fail during `CREATE TABLE` before exercising their
intended read/schema-change/compaction behavior. Please update the affected
suites in this PR, either to stop creating new V1 tables or to use
deterministic legacy fixtures/expected-rejection tests.
##########
regression-test/suites/inverted_index_p0/storage_format/test_inverted_index_v1_deprecated.groovy:
##########
@@ -0,0 +1,107 @@
+// 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: CREATE TABLE with explicit V1 must fail
+ 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 fail
+ 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 version specified → default succeeds and is not V1
+ // AC3: ALTER TABLE ADD INDEX on default table succeeds (format inherited
from table, not user input)
+ def testTable = "test_v1_deprecated_default"
+ try {
+ sql "DROP TABLE IF EXISTS ${testTable}"
+ sql """
+ CREATE TABLE ${testTable} (
+ 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 ${testTable}"
+
assertFalse(showCreate[0][1].contains("\"inverted_index_storage_format\" =
\"V1\""))
+
+ // ALTER TABLE ADD INDEX: format comes from table schema, user cannot
specify V1 here
+ sql "ALTER TABLE ${testTable} ADD INDEX idx_v (v) USING INVERTED"
+ def showCreate2 = sql "SHOW CREATE TABLE ${testTable}"
+ assertTrue(showCreate2[0][1].contains("USING INVERTED"))
+
assertFalse(showCreate2[0][1].contains("\"inverted_index_storage_format\" =
\"V1\""))
+ } finally {
+ sql "DROP TABLE IF EXISTS ${testTable}"
+ }
+
+ // AC4: existing V1 table metadata load and read/write must not be affected
+ // Requires a pre-existing V1 table seeded before V1 was deprecated.
+ // Skips gracefully if no such table exists (e.g. fresh CI environment).
+ try {
+ sql "CREATE DATABASE IF NOT EXISTS test_v1_compat"
+ def v1Tables = sql "SHOW TABLES FROM test_v1_compat LIKE
'v1_legacy_table'"
+ if (v1Tables.size() > 0) {
+ // metadata: V1 format is preserved in SHOW CREATE TABLE
+ def ddl = sql "SHOW CREATE TABLE test_v1_compat.v1_legacy_table"
+ assertTrue(ddl[0][1].contains("\"inverted_index_storage_format\" =
\"V1\""))
+
+ // read: inverted index query works
+ def readResult = sql "SELECT COUNT(*) FROM
test_v1_compat.v1_legacy_table WHERE v MATCH 'hello'"
+ assertTrue(readResult[0][0] >= 1)
+
+ // write: INSERT into V1 table works
+ sql "INSERT INTO test_v1_compat.v1_legacy_table VALUES (9999, 'v1
compat write test')"
+ def writeResult = sql "SELECT COUNT(*) FROM
test_v1_compat.v1_legacy_table WHERE v MATCH 'compat'"
+ assertTrue(writeResult[0][0] >= 1)
+
+ sql "DELETE FROM test_v1_compat.v1_legacy_table WHERE k = 9999"
+ } else {
Review Comment:
This catch makes the legacy-compatibility coverage non-failing: any
regression in `SHOW CREATE`, read, insert, or delete for an existing V1 table
is logged as skipped, so CI still passes. If existing V1 compatibility is a
requirement of this change, this needs a deterministic fixture/setup that
actually loads legacy V1 metadata and lets failures fail; otherwise this block
gives false confidence.
--
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]
