github-actions[bot] commented on code in PR #65460: URL: https://github.com/apache/doris/pull/65460#discussion_r3564003706
########## regression-test/suites/external_table_p0/hive/test_transactional_hive.groovy: ########## @@ -1,195 +0,0 @@ -// 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_transactional_hive", "p0,external") { - String skip_checking_acid_version_file = "false" - - def q01 = { - sql """set skip_checking_acid_version_file=${skip_checking_acid_version_file}""" - qt_q01 """ - select * from orc_full_acid order by id; - """ - qt_q02 """ - select value from orc_full_acid order by id; - """ - qt_q04 """ - select * from orc_full_acid_empty; - """ - qt_q05 """ - select count(*) from orc_full_acid_empty; - """ - } - - def q01_par = { - sql """set skip_checking_acid_version_file=${skip_checking_acid_version_file}""" - qt_q01 """ - select * from orc_full_acid_par order by id; - """ - qt_q02 """ - select value from orc_full_acid_par order by id; - """ - qt_q03 """ - select * from orc_full_acid_par where value = 'BB' order by id; - """ - qt_q04 """ - select * from orc_full_acid_par_empty; - """ - qt_q05 """ - select count(*) from orc_full_acid_par_empty; - """ - } - - def q01_par_limit = { - qt_q01_limit """ - select * from orc_full_acid_par order by id limit 3; - """ - qt_q02_limit """ - select value from orc_full_acid_par order by id limit 3; - """ - qt_q03_limit """ - select * from orc_full_acid_par where value = 'BB' order by id limit 3; - """ - qt_q04_limit """ - select * from orc_full_acid_par_empty limit 3; - """ - } - - def test_acid = { - - sql """set enable_fallback_to_original_planner=false;""" Review Comment: This removes the only end-to-end SQL coverage for transactional Hive reads while the production read path is still active. The deleted suite forces Nereids with `enable_fallback_to_original_planner=false` and then exercises converted/full-ACID HMS tables through partition predicates, range predicates, counts, limits, and explain. After this patch there are no remaining regression references to the removed ACID table names or `skip_checking_acid_version_file`, but `HiveScanNode` still registers `HiveTransaction` and builds `TTransactionalHiveDesc` scan ranges that BE handles as `transactional_hive`. If the feature is no longer supported, the reachable path should reject it before users can hit it; otherwise please keep a small regression case that still covers the planner/split/BE scan path until the production cleanup lands. ########## regression-test/suites/external_table_p0/hive/test_hive_orc_predicate.groovy: ########## @@ -24,44 +24,31 @@ suite("test_hive_orc_predicate", "p0,external") { } for (String hivePrefix : ["hive2", "hive3"]) { - try { - String hms_port = context.config.otherConfigs.get(hivePrefix + "HmsPort") - String catalog_name = "${hivePrefix}_test_predicate" - String externalEnvIp = context.config.otherConfigs.get("externalEnvIp") + String hms_port = context.config.otherConfigs.get(hivePrefix + "HmsPort") + String catalog_name = "${hivePrefix}_test_predicate" + String externalEnvIp = context.config.otherConfigs.get("externalEnvIp") - sql """drop catalog if exists ${catalog_name}""" - sql """create catalog if not exists ${catalog_name} properties ( - "type"="hms", - 'hive.metastore.uris' = 'thrift://${externalEnvIp}:${hms_port}' - );""" - sql """use `${catalog_name}`.`multi_catalog`""" + sql """drop catalog if exists ${catalog_name}""" + sql """create catalog if not exists ${catalog_name} properties ( + "type"="hms", + 'hive.metastore.uris' = 'thrift://${externalEnvIp}:${hms_port}' + );""" + sql """use `${catalog_name}`.`multi_catalog`""" - qt_predicate_fixed_char1 """ select * from fixed_char_table where c = 'a';""" - qt_predicate_fixed_char2 """ select * from fixed_char_table where c = 'a ';""" + qt_predicate_fixed_char1 """ select * from fixed_char_table where c = 'a';""" + qt_predicate_fixed_char2 """ select * from fixed_char_table where c = 'a ';""" - qt_predicate_changed_type1 """ select * from type_changed_table where id = '1';""" - qt_predicate_changed_type2 """ select * from type_changed_table where id = '2';""" - qt_predicate_changed_type3 """ select * from type_changed_table where id = '3';""" + qt_predicate_changed_type1 """ select * from type_changed_table where id = '1';""" + qt_predicate_changed_type2 """ select * from type_changed_table where id = '2';""" + qt_predicate_changed_type3 """ select * from type_changed_table where id = '3';""" - qt_predicate_null_aware_equal_in_rt """select * from table_a inner join table_b on table_a.age <=> table_b.age and table_b.id in (1,3) order by table_a.id;""" + qt_predicate_null_aware_equal_in_rt """select * from table_a inner join table_b on table_a.age <=> table_b.age and table_b.id in (1,3) order by table_a.id;""" - // use check_orc_init_sargs_success to test full acid push down - sql """use `${catalog_name}`.`default`""" - if (hivePrefix == "hive3") { - sql """ set check_orc_init_sargs_success = true; """ Review Comment: This deletion also removes the only regression use of `check_orc_init_sargs_success`. That variable is what makes BE fail when `_init_search_argument` cannot build an ORC SARG; without it, the remaining ORC predicate cases only verify returned rows and would still pass if a planner/expression change silently stopped pushing a filter into ORC SARG initialization. Since the removed dependency is the transactional table, please move this assertion to one of the non-transactional ORC predicate cases (or add an equivalent replacement) so predicate-pushdown coverage survives the ACID test removal. ########## regression-test/suites/external_table_p0/hive/test_transactional_hive.groovy: ########## @@ -1,195 +0,0 @@ -// 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_transactional_hive", "p0,external") { - String skip_checking_acid_version_file = "false" - - def q01 = { - sql """set skip_checking_acid_version_file=${skip_checking_acid_version_file}""" - qt_q01 """ - select * from orc_full_acid order by id; - """ - qt_q02 """ - select value from orc_full_acid order by id; - """ - qt_q04 """ - select * from orc_full_acid_empty; - """ - qt_q05 """ - select count(*) from orc_full_acid_empty; - """ - } - - def q01_par = { - sql """set skip_checking_acid_version_file=${skip_checking_acid_version_file}""" - qt_q01 """ - select * from orc_full_acid_par order by id; - """ - qt_q02 """ - select value from orc_full_acid_par order by id; - """ - qt_q03 """ - select * from orc_full_acid_par where value = 'BB' order by id; - """ - qt_q04 """ - select * from orc_full_acid_par_empty; - """ - qt_q05 """ - select count(*) from orc_full_acid_par_empty; - """ - } - - def q01_par_limit = { - qt_q01_limit """ - select * from orc_full_acid_par order by id limit 3; - """ - qt_q02_limit """ - select value from orc_full_acid_par order by id limit 3; - """ - qt_q03_limit """ - select * from orc_full_acid_par where value = 'BB' order by id limit 3; - """ - qt_q04_limit """ - select * from orc_full_acid_par_empty limit 3; - """ - } - - def test_acid = { - - sql """set enable_fallback_to_original_planner=false;""" - try { - sql """ select * from orc_to_acid_tb """ - }catch( Exception e) { - assertTrue(e.getMessage().contains("For no acid table convert to acid, please COMPACT")); - } - - qt_2 """ select * from orc_to_acid_compacted_tb order by id """ - qt_3 """ select * from orc_to_acid_compacted_tb where part_col=101 order by id """ - qt_4 """ select * from orc_to_acid_compacted_tb where part_col=102 order by id """ - qt_5 """ select * from orc_to_acid_compacted_tb where id < 3 order by id """ - qt_6 """ select * from orc_to_acid_compacted_tb where id > 3 order by id """ - - - qt_7 """ select * from orc_acid_minor order by id """ - qt_10 """ select * from orc_acid_minor where id < 3 order by id """ - qt_11 """ select * from orc_acid_minor where id > 3 order by id """ - - - qt_12 """ select * from orc_acid_major order by id """ - qt_15 """ select * from orc_acid_major where id < 3 order by id """ - qt_16 """ select * from orc_acid_major where id > 3 order by id """ - - qt_17 """ select * from orc_acid_major order by id limit 1 """ - qt_18 """ select * from orc_acid_major where id < 3 order by id limit 1 """ - qt_19 """ select * from orc_acid_major where id > 3 order by id limit 1""" - } - - def test_acid_write = { - sql """set enable_fallback_to_original_planner=false;""" - - - - try { - sql """ - CREATE TABLE acid_tb ( Review Comment: This also removes the only test coverage I can find for the transactional Hive DDL/DML guardrails. The deleted `test_acid_write` block verifies that creating a transactional Hive table, inserting into an existing transactional Hive table, and dropping one all fail with the expected user-facing errors. Those guards still exist in `HiveMetadataOps.createTable`, `InsertIntoTableCommand`, and `HiveMetadataOps.dropTable`, but after this patch there is no remaining regression or FE unit test for them. Please keep a small negative test for these rejection paths until the production behavior is removed or intentionally changed. ########## regression-test/suites/external_table_p2/hive/test_hive_translation_insert_only.groovy: ########## @@ -1,98 +0,0 @@ -// 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_hive_translation_insert_only", "p2,external") { - - String enabled = context.config.otherConfigs.get("enableExternalEmrTest") - //hudi hive use same catalog in p2. - if (enabled == null || !enabled.equalsIgnoreCase("true")) { - logger.info("disable test") - return; - } - - String props = context.config.otherConfigs.get("emrCatalogCommonProp") - String hms_catalog_name = "test_hive_translation_insert_only" - - sql """drop catalog if exists ${hms_catalog_name};""" - sql """ - CREATE CATALOG IF NOT EXISTS ${hms_catalog_name} - PROPERTIES ( - "type" = "hms", - 'hive.version' = '3.1.3', - ${props} - ); - """ - - logger.info("catalog " + hms_catalog_name + " created") - sql """switch ${hms_catalog_name};""" - logger.info("switched to catalog " + hms_catalog_name) - sql """ use regression;""" - - qt_1 """ select * from text_insert_only order by id """ Review Comment: This deletes the only regression coverage I can find for insert-only transactional Hive reads. The suite verifies ordered reads and counts for `text_insert_only`, `parquet_insert_only_major`, and `orc_insert_only_minor`, and the embedded setup shows these are `transactional='true'` tables with `transactional_properties='insert_only'`. That path is distinct from the full-ACID suite: production still classifies transactional Hive tables via `isHiveTransactionalTable()`, creates a `HiveTransaction`, and calls `getFilesByTransaction(..., hiveTransaction.isFullAcid(), ...)` with `isFullAcid=false` for insert-only tables. Please keep a minimal insert-only read/count regression, or remove/reject this production path in the same change. -- 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]
