github-actions[bot] commented on code in PR #66093: URL: https://github.com/apache/doris/pull/66093#discussion_r3655512640
########## regression-test/suites/row_binlog_p0/test_row_binlog_cluster_key.groovy: ########## @@ -0,0 +1,93 @@ +// 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_row_binlog_cluster_key", "nonConcurrent") { + if (isCloudMode()) { + return + } + + def clusterKeyTable = "test_row_binlog_cluster_key" + def noClusterKeyTable = "test_row_binlog_without_cluster_key" + def inheritedBinlogDb = "test_row_binlog_cluster_key_db" + + sql "DROP TABLE IF EXISTS ${clusterKeyTable} FORCE" + sql "DROP DATABASE IF EXISTS ${inheritedBinlogDb} FORCE" + sql "CREATE DATABASE ${inheritedBinlogDb}" + sql """ + ALTER DATABASE ${inheritedBinlogDb} SET PROPERTIES ( + "binlog.enable" = "false", + "binlog.format" = "ROW" + ) + """ + onFinish { Review Comment: Please remove this `onFinish` cleanup. The repository's regression-test contract requires dropping test objects before use and preserving them after execution for debugging. `ScriptContext` runs finish callbacks from `finally`, so when either assertion below fails this immediately deletes the database/table state needed to inspect the effective binlog properties and generated cluster-key metadata. The pre-test `DROP DATABASE IF EXISTS` already makes reruns repeatable. ########## regression-test/suites/row_binlog_p0/test_row_binlog_cluster_key.groovy: ########## @@ -0,0 +1,93 @@ +// 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_row_binlog_cluster_key", "nonConcurrent") { + if (isCloudMode()) { + return + } + + def clusterKeyTable = "test_row_binlog_cluster_key" + def noClusterKeyTable = "test_row_binlog_without_cluster_key" + def inheritedBinlogDb = "test_row_binlog_cluster_key_db" + + sql "DROP TABLE IF EXISTS ${clusterKeyTable} FORCE" + sql "DROP DATABASE IF EXISTS ${inheritedBinlogDb} FORCE" + sql "CREATE DATABASE ${inheritedBinlogDb}" + sql """ + ALTER DATABASE ${inheritedBinlogDb} SET PROPERTIES ( + "binlog.enable" = "false", + "binlog.format" = "ROW" + ) + """ + onFinish { + try_sql "DROP DATABASE IF EXISTS ${inheritedBinlogDb} FORCE" + } + + setFeConfigTemporary([random_add_order_by_keys_for_mow: true]) { + test { + sql """ + CREATE TABLE ${clusterKeyTable} ( + id BIGINT NOT NULL, + cluster_value INT NOT NULL, + payload VARCHAR(32) NOT NULL + ) + UNIQUE KEY(id) + ORDER BY(cluster_value) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "true", + "light_schema_change" = "true", + "binlog.enable" = "true", + "binlog.format" = "ROW" + ) + """ + exception "Unique merge-on-write tables with cluster keys do not support binlog<Row>" + } + + sql """ + CREATE TABLE ${inheritedBinlogDb}.${noClusterKeyTable} ( + id BIGINT NOT NULL, + cluster_value INT NOT NULL, + payload VARCHAR(32) NOT NULL + ) + UNIQUE KEY(id) + DISTRIBUTED BY HASH(id) BUCKETS 1 + PROPERTIES ( + "replication_num" = "1", + "enable_unique_key_merge_on_write" = "true", + "light_schema_change" = "true", + "binlog.enable" = "true" + ) + """ + + def binlogProperties = sql """ + SELECT PROPERTY_NAME, PROPERTY_VALUE + FROM information_schema.table_properties + WHERE TABLE_CATALOG = 'internal' + AND TABLE_SCHEMA = '${inheritedBinlogDb}' + AND TABLE_NAME = '${noClusterKeyTable}' + AND PROPERTY_NAME IN ('binlog.enable', 'binlog.format') + ORDER BY PROPERTY_NAME + """ + assertEquals([["binlog.enable", "true"], ["binlog.format", "ROW"]], binlogProperties) Review Comment: Please use named `qt_`/`order_qt_` cases and commit the generated `.out` instead of these direct assertions (including the `SHOW CREATE` check below). Added regression tests are required to record determined results through the harness; currently this PR adds no `.out`, so the expected properties and absence of an `ORDER BY` clause are not part of the normal golden-result artifact and review flow. ########## regression-test/suites/row_binlog_p0/test_row_binlog_cluster_key.groovy: ########## @@ -0,0 +1,93 @@ +// 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_row_binlog_cluster_key", "nonConcurrent") { + if (isCloudMode()) { + return + } + + def clusterKeyTable = "test_row_binlog_cluster_key" Review Comment: Please hardcode these fixed table names at their SQL/query use sites. The repository's regression-test contract explicitly prohibits `def tableName` for ordinary test tables; both values here are static and never parameterized or iterated, so the indirection is unnecessary. The database-name variable can remain because that rule is specifically about table names. -- 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]
