Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cherry-pick fix bvt test #16579

Merged
merged 3 commits into from
Jun 3, 2024
Merged

cherry-pick fix bvt test #16579

merged 3 commits into from
Jun 3, 2024

Conversation

YANGGMM
Copy link
Contributor

@YANGGMM YANGGMM commented Jun 3, 2024

User description

What type of PR is this?

  • API-change
  • BUG
  • Improvement
  • Documentation
  • Feature
  • Test and CI
  • Code Refactoring

Which issue(s) this PR fixes:

issue #16300

What this PR does / why we need it:

cherry-pick fix bvt test


PR Type

Tests


Description

  • Added validation queries for TPCH tables count results to ensure data consistency.
  • Removed lower_case_table_names variable checks and settings from DDL test cases.
  • Adjusted column names in CREATE TABLE and SELECT statements to lowercase in DDL test cases.

Changes walkthrough 📝

Relevant files
Tests
00_COUNT.result
Add validation queries for TPCH tables count results         

test/distributed/cases/benchmark/tpch/04_CLEANUP/00_COUNT.result

  • Added multiple SELECT COUNT(*) queries for various tables in the tpch
    schema.
  • Included repeated queries to validate consistency.
  • +210/-0 
    00_COUNT.sql
    Add validation queries for TPCH tables count                         

    test/distributed/cases/benchmark/tpch/04_CLEANUP/00_COUNT.sql

  • Added multiple SELECT COUNT(*) queries for various tables in the tpch
    schema.
  • Included repeated queries to validate consistency.
  • +84/-0   
    create_table_as_select.result
    Remove lower_case_table_names settings and adjust column names

    test/distributed/cases/ddl/create_table_as_select.result

  • Removed lower_case_table_names variable checks and settings.
  • Adjusted column names in CREATE TABLE and SELECT statements to
    lowercase.
  • +2/-7     
    create_table_as_select.sql
    Remove lower_case_table_names settings                                     

    test/distributed/cases/ddl/create_table_as_select.sql

    • Removed lower_case_table_names variable checks and settings.
    +0/-3     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and primarily involve adding repeated validation queries for database consistency checks. The PR is focused on testing and does not involve complex logic changes or new feature implementations.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Redundancy: The PR includes multiple identical queries for the same tables, which might be intentional for consistency checks but could be optimized if the intention is to verify the count only once.

    🔒 Security concerns

    No

    @mergify mergify bot requested a review from sukki37 June 3, 2024 02:02
    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jun 3, 2024
    @mergify mergify bot added the kind/test-ci label Jun 3, 2024
    Copy link

    PR-Agent was enabled for this repository. To continue using it, please link your git user with your CodiumAI identity here.

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure consistency in column naming conventions

    Ensure consistency in column naming conventions. The column NewCol should be renamed to
    newcol to match the other columns' naming style.

    test/distributed/cases/ddl/create_table_as_select.result [1597]

    -alias02    CREATE TABLE `alias02` (\n  `newcol` INT DEFAULT NULL,\n  `col1` INT DEFAULT NULL,\n  `col2` DECIMAL(38,0) DEFAULT NULL\n)
    +alias02    CREATE TABLE `alias02` (\n  `NewCol` INT DEFAULT NULL,\n  `col1` INT DEFAULT NULL,\n  `col2` DECIMAL(38,0) DEFAULT NULL\n)
     
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies an inconsistency in column naming conventions between the SQL creation script and the result output, which is important for maintaining code standards and readability. The suggestion aligns with best practices.

    8
    Maintainability
    Consolidate repeated SQL queries to improve readability and maintainability

    The repeated SELECT COUNT(*) queries for each table can be consolidated to improve
    readability and maintainability. Instead of repeating the same query multiple times,
    consider running each query once and storing the results.

    test/distributed/cases/benchmark/tpch/04_CLEANUP/00_COUNT.result [1-210]

     select count(*) from tpch.customer;
     count(*)
     150
    -select count(*) from tpch.customer;
    +select count(*) from tpch.lineitem;
     count(*)
    -150
    +6005
     ...
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies the redundancy in repeated SQL queries across multiple lines, which could be optimized for better maintainability. However, the 'improved_code' section does not accurately reflect a consolidated version, it just lists a subset of the existing queries.

    7

    @mergify mergify bot merged commit 095cfd3 into matrixorigin:1.2-dev Jun 3, 2024
    18 checks passed
    @aylei aylei mentioned this pull request Jun 5, 2024
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/test-ci Review effort [1-5]: 2 size/M Denotes a PR that changes [100,499] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants