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

change LIMIT,OFFSET's data type from int64 to uint64 #16698

Merged
merged 7 commits into from
Jun 7, 2024

Conversation

daviszhen
Copy link
Contributor

@daviszhen daviszhen commented Jun 5, 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 ##16624

What this PR does / why we need it:

limit和offset 的数据类型改为uint64。


PR Type

Bug fix, Tests


Description

  • Changed limit and offset data types from int64 to uint64 across various modules.
  • Updated related functions and test cases to handle uint64.
  • Added checks for non-negative values in limit and offset.
  • Added new test cases to ensure proper handling of uint64 limit and offset.

Changes walkthrough 📝

Relevant files
Bug fix
13 files
limit.go
Update `limit` data type to `uint64`.                                       

pkg/sql/colexec/limit/limit.go

  • Changed limit data type from int64 to uint64.
+1/-1     
limit.go
Update `mergelimit` limit data type to `uint64`.                 

pkg/sql/colexec/mergelimit/limit.go

  • Changed limit data type from int64 to uint64.
+1/-1     
offset.go
Update `mergeoffset` offset data type to `uint64`.             

pkg/sql/colexec/mergeoffset/offset.go

  • Changed offset data type from int64 to uint64.
+1/-1     
top.go
Update `mergetop` limit data type to `uint64`.                     

pkg/sql/colexec/mergetop/top.go

  • Changed limit data type from int64 to uint64.
  • Updated related functions to handle uint64.
  • +7/-7     
    types.go
    Update `mergetop` limit type definition to `uint64`.         

    pkg/sql/colexec/mergetop/types.go

    • Changed limit data type from int64 to uint64.
    +1/-1     
    offset.go
    Update `offset` data type to `uint64`.                                     

    pkg/sql/colexec/offset/offset.go

    • Changed offset data type from int64 to uint64.
    +1/-1     
    top.go
    Update `top` limit data type to `uint64`.                               

    pkg/sql/colexec/top/top.go

  • Changed limit data type from int64 to uint64.
  • Updated related functions to handle uint64.
  • +8/-8     
    types.go
    Update `top` limit type definition to `uint64`.                   

    pkg/sql/colexec/top/types.go

    • Changed limit data type from int64 to uint64.
    +1/-1     
    compile.go
    Update compile logic for limit and offset to use `uint64`.

    pkg/sql/compile/compile.go

  • Changed limit and offset data types from int64 to uint64.
  • Added overflow check for topN.
  • +7/-3     
    apply_indices_vector.go
    Update plan apply indices vector for uint64 limit.             

    pkg/sql/plan/apply_indices_vector.go

    • Changed limit data type from int64 to uint64.
    +2/-2     
    limit_binder.go
    Update limit binder for uint64 limit and offset.                 

    pkg/sql/plan/limit_binder.go

  • Changed limit and offset data types from int64 to uint64.
  • Added checks for non-negative values.
  • +21/-9   
    query_builder.go
    Update query builder for uint64 limit and offset.               

    pkg/sql/plan/query_builder.go

  • Changed limit and offset data types from int64 to uint64.
  • Updated related logic to handle uint64.
  • +6/-28   
    stats.go
    Update stats calculation for uint64 limit.                             

    pkg/sql/plan/stats.go

    • Changed limit data type from int64 to uint64.
    +4/-4     
    Tests
    10 files
    limit_test.go
    Update limit test cases to use `uint64`.                                 

    pkg/sql/colexec/limit/limit_test.go

  • Updated test cases to use uint64 for LimitExpr.
  • Added require import for assertions.
  • +5/-4     
    limit_test.go
    Update mergelimit test cases to use `uint64`.                       

    pkg/sql/colexec/mergelimit/limit_test.go

  • Updated test cases to use uint64 for Limit.
  • Added require import for assertions.
  • +3/-2     
    offset_test.go
    Update mergeoffset test cases to use `uint64`.                     

    pkg/sql/colexec/mergeoffset/offset_test.go

  • Updated test cases to use uint64 for OffsetExpr.
  • Added require import for assertions.
  • +3/-2     
    top_test.go
    Update mergetop test cases to use `uint64`.                           

    pkg/sql/colexec/mergetop/top_test.go

  • Updated test cases to use uint64 for Limit.
  • Added require import for assertions.
  • +3/-2     
    offset_test.go
    Update offset test cases to use `uint64`.                               

    pkg/sql/colexec/offset/offset_test.go

  • Updated test cases to use uint64 for OffsetExpr.
  • Added require import for assertions.
  • +5/-4     
    top_test.go
    Update top test cases to use `uint64`.                                     

    pkg/sql/colexec/top/top_test.go

  • Updated test cases to use uint64 for Limit.
  • Added require import for assertions.
  • +3/-2     
    compile_test.go
    Update compile test cases for uint64 limit and offset.     

    pkg/sql/compile/compile_test.go

  • Added require import for assertions.
  • Updated test cases to reflect changes in limit and offset data types.
  • +9/-3     
    build_test.go
    Add test cases for uint64 limit in plan build.                     

    pkg/sql/plan/build_test.go

  • Added test cases for uint64 limit.
  • Added assert import for assertions.
  • +20/-1   
    limit.result
    Add test cases for uint64 limit and offset.                           

    test/distributed/cases/dml/select/limit.result

    • Added test cases for uint64 limit and offset.
    +54/-0   
    limit.sql
    Add SQL test cases for uint64 limit and offset.                   

    test/distributed/cases/dml/select/limit.sql

    • Added SQL test cases for uint64 limit and offset.
    +19/-0   
    Enhancement
    2 files
    make.go
    Add function to create uint64 constant expressions.           

    pkg/sql/plan/make.go

    • Added function MakePlan2Uint64ConstExprWithType.
    +1/-0     
    compiler_context.go
    Implement GetSnapshot to return nil.                                         

    pkg/vm/engine/memoryengine/compiler_context.go

    • Implemented GetSnapshot to return nil.
    +2/-4     

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

    @daviszhen daviszhen changed the title 0605 fix limit 1.2 change LIMIT,OFFSET's data type from int64 to uint64 Jun 5, 2024
    @matrix-meow matrix-meow added the size/M Denotes a PR that changes [100,499] lines label Jun 5, 2024
    @mergify mergify bot requested a review from sukki37 June 5, 2024 13:00
    @mergify mergify bot added the kind/bug Something isn't working label Jun 5, 2024
    @codiumai-pr-agent-pro codiumai-pr-agent-pro bot changed the title change LIMIT,OFFSET's data type from int64 to uint64 0605 fix limit 1.2 Jun 5, 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 Review 🔍

    ⏱️ Estimated effort to review [1-5]

    3, because the PR involves multiple changes across various files, including data type changes, test updates, and logical adjustments. The changes are spread across core execution and planning components which require careful review to ensure correctness and compatibility.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The conversion from int64 to uint64 in various places assumes that the original int64 values were non-negative. This assumption should be explicitly checked to avoid unexpected behavior.

    Overflow Handling: The changes include handling for potential overflow scenarios when calculating limits and offsets. It's crucial to ensure that these checks are correctly implemented to prevent runtime errors.

    🔒 Security concerns

    No

    @daviszhen daviszhen changed the title 0605 fix limit 1.2 change LIMIT,OFFSET's data type from int64 to uint64 Jun 5, 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
    Possible bug
    Correct the SQL syntax for the drop table if exists statement

    The drop table if exists t1(a int); statement on line 1 is incorrect SQL syntax. The
    correct syntax should be drop table if exists t1;.

    test/distributed/cases/dml/select/limit.result [1]

    -drop table if exists t1(a int);
    +drop table if exists t1;
     
    Suggestion importance[1-10]: 10

    Why: The suggestion correctly identifies and fixes a syntax error in the SQL statement, which is crucial for the script to execute successfully.

    10
    Performance
    Move the testutil.NewProc() call outside the loop to avoid unnecessary reinitialization

    The testutil.NewProc() call inside the loop should be moved outside to avoid unnecessary
    reinitialization in each iteration.

    pkg/sql/plan/build_test.go [1186-1189]

    -testutil.NewProc()
    +proc := testutil.NewProc()
     mock := NewMockOptimizer(false)
     
     for _, sql := range sqls {
     
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a performance issue with unnecessary reinitialization within a loop, which can significantly improve the efficiency of the test code.

    8
    Maintainability
    Replace unprofessional comments with a meaningful TODO comment

    The commented-out code with the FIXME and !!!GOD!!! comments should be removed or replaced
    with a more meaningful error handling or logging mechanism. Leaving such comments in the
    codebase can be confusing and unprofessional.

    pkg/sql/compile/compile_test.go [161-165]

    -//FIXME:
    -//!!!GOD!!!
    -//Sometimes it is 0.
    -//Sometimes it is 24.
    -//require.Equal(t, int64(0), tc.proc.Mp().CurrNB())
    +// TODO: Investigate why tc.proc.Mp().CurrNB() sometimes returns 0 and sometimes 24
    +// require.Equal(t, int64(0), tc.proc.Mp().CurrNB())
     
    Suggestion importance[1-10]: 7

    Why: The suggestion correctly identifies unprofessional comments and proposes a more appropriate TODO comment, enhancing code professionalism and maintainability.

    7
    Enhancement
    Simplify the overflow check by incorporating it directly into the if condition

    Instead of using a separate overflow variable, you can directly check for overflow in the
    if condition to make the code more concise.

    pkg/sql/compile/compile.go [2798-2802]

    -overflow := false
    -if topN < limit || topN < offset {
    -    overflow = true
    -}
    -if !overflow && topN <= 8192*2 {
    +if (topN < limit || topN < offset) || topN <= 8192*2 {
     
    Suggestion importance[1-10]: 6

    Why: The suggestion to simplify the overflow check is valid and improves code conciseness, although it's not a critical change.

    6
    Combine nested if conditions to reduce nesting and improve readability

    The if condition checking for cExpr can be combined with the nested if condition checking
    for c to reduce nesting and improve readability.

    pkg/sql/plan/stats.go [878-882]

    +if cExpr, ok := limitExpr.Expr.(*plan.Expr_Lit); ok {
    +    if c, ok := cExpr.Lit.Value.(*plan.Literal_U64Val); ok {
    +        node.Stats.Outcnt = float64(c.U64Val)
    +        node.Stats.Selectivity = node.Stats.Outcnt / node.Stats.Cost
    +    }
    +}
     
    -
    Suggestion importance[1-10]: 5

    Why: Combining the nested if conditions is a good suggestion for improving readability, but it's a relatively minor enhancement in the context of the overall codebase.

    5
    Replace the redundant limit clause with a more meaningful limit

    The test case for select * from t1 limit 0,18446744073709551615; on line 5 might be
    redundant as it selects all rows without any limit. Consider removing or replacing it with
    a more meaningful limit.

    test/distributed/cases/dml/select/limit.result [5]

    -select * from t1 limit 0,18446744073709551615;
    +select * from t1 limit 0,5;
     
    Suggestion importance[1-10]: 5

    Why: While the suggestion to replace the limit clause is valid for enhancing the test's meaningfulness, the original limit might be intentionally testing edge cases or maximum values, so the suggestion might not be necessary.

    5

    @mergify mergify bot merged commit c848d1c into matrixorigin:1.2-dev Jun 7, 2024
    16 of 18 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix kind/bug Something isn't working Review effort [1-5]: 3 size/M Denotes a PR that changes [100,499] lines Tests
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    9 participants