github-actions[bot] commented on PR #61842: URL: https://github.com/apache/doris/pull/61842#issuecomment-4153145846
## Code Review Summary **PR**: [chore](regression) Merge nereids_p0 test cases into query_p0 This PR consolidates 1185 files by merging unique test content from `nereids_p0` into `query_p0` and deleting the nereids_p0 originals. The overall approach is sound and well-scoped. ### Critical Checkpoint Conclusions **Goal & Correctness**: The PR successfully merges unique nereids_p0 test content into query_p0, removes `enable_nereids_planner`/`enable_fallback_to_original_planner` settings from merged content, and updates database references from `nereids_test_query_db` to `test_query_db`. The newly added `.out` file (`test_show_create_view.out`) correctly uses the `regression_test_query_p0_show` prefix instead of the nereids variant. **Scope**: All 1185 changed files are within `regression-test/` (data and suites). No production code, configuration, or non-test files are affected. This is appropriately scoped. **Concurrency**: N/A — test file reorganization only. **Lifecycle/Static Init**: N/A. **Configuration Items**: N/A. **Incompatible Changes**: N/A — no format or symbol changes. **Parallel Code Paths**: N/A. **Test Coverage**: This PR is itself about test consolidation. The merged content preserves the existing test coverage. **Observability**: N/A. **Transaction/Persistence**: N/A. **FE-BE Variables**: N/A. **Performance**: N/A. ### Findings (Pre-existing, inherited from nereids_p0) The following issues were present in the original nereids_p0 source files and carried over during the merge. They are **not newly introduced** by this PR, but this consolidation provides a good opportunity to clean them up: 1. **`qt_sql` identifier reuse** (Medium, pre-existing): In `test_gis_function.groovy`, the identifier `qt_sql` is used 358 times (up from 105 pre-merge). In the Doris regression framework, only the **last** `qt_sql` invocation's output is validated against the `.out` file, meaning the vast majority of these tests have no golden-file validation. The same pattern exists in `test_date_function.groovy`. Consider using unique identifiers (e.g., `qt_sql_contains_1`, `qt_sql_intersects_2`, etc.) in a follow-up. 2. **`enable_nereids_dml` settings** (Low, pre-existing): 27 instances of `sql 'set enable_nereids_dml=true'` remain across 22 files in query_p0 (e.g., `insert_into_table/`, `delete/`, `update/`). Per AGENTS.md: *"nereids optimizer and pipeline engine settings can use default states."* These are redundant if nereids DML is now the default. Could be cleaned up in a follow-up. 3. **`nereids_insert_into_table_test` database name** (Low, pre-existing): 36 references across 12 files in `insert_into_table/` use this nereids-prefixed database name. Functionally correct but cosmetically inconsistent after the merge. 4. **Tables dropped after use** (Low, pre-existing): In `test_subquery.groovy` (newly merged content), several tables are dropped both before AND after use. Per standards, tables should only be dropped before use to preserve the environment for debugging. Also seen in other merged files. 5. **`def tableName` pattern** (Low, pre-existing): Several merged files use `def tableName = "..."` instead of hardcoding table names directly in SQL, contrary to Doris regression test standards. ### Verdict **No blocking issues found.** The PR is a well-executed mechanical merge of nereids_p0 test content into query_p0. The nereids-specific settings (`enable_nereids_planner`, `enable_fallback_to_original_planner`) have been properly removed from merged content, database references have been correctly updated, and the `.out` file is correctly adapted. All findings above are pre-existing patterns inherited from the source files. Consider addressing the `qt_sql` identifier reuse issue in a follow-up PR, as it significantly reduces the validation value of the merged test cases. -- 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]
