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

add fulltext index test case #20363

Open
wants to merge 1 commit into
base: 2.0-dev
Choose a base branch
from

Conversation

tom-csf
Copy link

@tom-csf tom-csf commented Nov 26, 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 #18869

What this PR does / why we need it:


add fulltest index test case

PR Type

Tests


Description

  • Added comprehensive test cases for fulltext index creation, deletion, and search functionality.
  • Included tests for fulltext search using MATCH() AGAINST() in natural language mode.
  • Tested fulltext index operations on both regular and JSON data types.
  • Added error handling tests for unsupported fulltext index operations and syntax errors.

Changes walkthrough 📝

Relevant files
Tests
fulltext.result
Add fulltext index test cases and scenarios                           

test/distributed/cases/fulltext/fulltext.result

  • Added test cases for creating and dropping fulltext indexes on tables.
  • Included tests for fulltext search using MATCH() AGAINST() in natural
    language mode.
  • Added tests for handling JSON data with fulltext indexes.
  • Included error handling scenarios for unsupported fulltext index
    operations.
  • +165/-0 
    fulltext.sql
    Add SQL commands for fulltext index testing                           

    test/distributed/cases/fulltext/fulltext.sql

  • Added SQL commands to create and drop fulltext indexes on various
    tables.
  • Included insert statements for sample data to test fulltext search.
  • Added SQL commands to test fulltext search functionality with
    different parsers.
  • Included SQL commands to test error scenarios with fulltext indexes.
  • +88/-0   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @CLAassistant
    Copy link

    CLA assistant check
    Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


    tom seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
    You have signed the CLA already but the status is still pending? Let us recheck it.

    Copy link

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

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Error Handling
    Verify that error messages and handling for invalid fulltext index operations are appropriate and consistent

    Character Encoding
    Some Chinese characters appear corrupted in the test data which may affect fulltext search accuracy

    Performance Testing
    Large dataset test with 10000 rows should include additional performance validation scenarios

    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 ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Prevent data corruption by dropping fulltext index before removing indexed column

    Add validation before attempting to alter table with fulltext index to prevent data
    corruption. The current code tries to drop a column that has a fulltext index
    without first dropping the index.

    test/distributed/cases/fulltext/fulltext.sql [395-397]

     create fulltext index ftidx1 on src(json1) with parser json;
     show create table src;
    +drop index ftidx1 on src;
     alter table src drop column json1;
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Critical suggestion that prevents potential data corruption by ensuring proper cleanup of fulltext indexes before dropping the indexed column, which is essential for maintaining data integrity.

    9
    General
    Add defensive error handling when dropping potentially non-existent indexes

    Add error handling for the case when trying to create a fulltext index on a
    non-existent column. The current code attempts to drop index fdx_04 which was never
    created.

    test/distributed/cases/fulltext/fulltext.sql [390-391]

    -drop  index fdx_04 on articles(title, body) with PARSER ngram;
    -drop  index fdx_04 on articles;
    +drop  index if exists fdx_04 on articles(title, body) with PARSER ngram;
    +drop  index if exists fdx_04 on articles;
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Adding IF EXISTS clause is a good defensive programming practice that prevents errors when dropping non-existent indexes, improving error handling and script reliability.

    7

    💡 Need additional feedback ? start a PR chat

    @mergify mergify bot requested a review from sukki37 November 26, 2024 02:13
    @mergify mergify bot added the kind/feature label Nov 26, 2024
    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Nov 26, 2024
    @tom-csf tom-csf changed the title add fulltest index test case add fulltext index test case Nov 26, 2024
    add fulltext index case
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    5 participants