Skip to content

Conversation

Ariznawlll
Copy link
Contributor

@Ariznawlll Ariznawlll commented May 21, 2025

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:

modify case
issue ##14804

What this PR does / why we need it:

modify case


PR Type

Tests


Description

  • Update test case to use count(*) instead of selecting all rows

  • Adjust expected result to match new test output


Changes walkthrough 📝

Relevant files
Tests
create_table_as_select.result
Update expected output for unix_timestamp test case           

test/distributed/cases/ddl/create_table_as_select.result

  • Replace expected output of select * from time05 with count(*)
  • Update result to show row count (3) instead of individual values
  • +2/-4     
    create_table_as_select.sql
    Modify test query to aggregate row count                                 

    test/distributed/cases/ddl/create_table_as_select.sql

  • Change SQL test to select count(*) from time05 instead of all columns
  • Align test logic with updated expected results
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more 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.


    [email protected] 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 Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ✅

    14804 - PR Code Verified

    Compliant requirements:

    • The PR modifies test cases to accommodate the fix for UNIX_TIMESTAMP(date) compatibility

    Requires further human verification:

    • Verify that the actual implementation of UNIX_TIMESTAMP function has been fixed elsewhere, as this PR only updates test cases
    • Confirm that the new behavior matches MySQL's behavior for UNIX_TIMESTAMP(date)

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

    Test Coverage

    The test now only verifies the count of rows rather than the actual values returned by UNIX_TIMESTAMP(date). This may not fully validate that the function returns the correct values matching MySQL.

    select count(*) from time05;
    drop table time05;

    Copy link

    qodo-merge-pro bot commented May 21, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @matrix-meow matrix-meow added the size/XS Denotes a PR that changes [1, 9] lines label May 21, 2025
    @mergify mergify bot merged commit 0c092d1 into matrixorigin:main May 21, 2025
    18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    kind/bug Something isn't working Review effort 1/5 size/S Denotes a PR that changes [10,99] lines
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants