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

handle null in convertRowsIntoBatch #16675

Merged
merged 2 commits into from
Jun 5, 2024
Merged

Conversation

ck89119
Copy link
Contributor

@ck89119 ck89119 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 #16662

What this PR does / why we need it:

handle null in convertRowsIntoBatch


PR Type

Bug fix


Description

This PR addresses issue #16662 by handling null values in the convertRowsIntoBatch function:

  • Added the nulls package to manage null values.
  • Updated the convertRowsIntoBatch function to check for null values in each column type.
  • Added null values to a nulls bitmap for each vector.
  • Set the nulls bitmap for each vector in the batch to ensure proper handling of null values.

Changes walkthrough 📝

Relevant files
Bug fix
util.go
Handle null values in `convertRowsIntoBatch` function.     

pkg/frontend/util.go

  • Added handling for null values in convertRowsIntoBatch function.
  • Introduced nulls package to manage null values.
  • Updated various type cases to check for null values and add them to
    nulls bitmap.
  • Set nulls bitmap for each vector in the batch.
  • +45/-0   

    💡 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 localized to a specific function with repetitive patterns across different data types. The logic is straightforward, involving null checks and bitmap manipulations, which are typical in database operations.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The PR does not handle the case where the input type does not match the expected type, which could lead to runtime panics. For example, if row[colIdx] is not of type int16 in the types.T_int16 case, the code will panic. It might be beneficial to add type assertion checks or handle the conversion errors gracefully.

    Inconsistency: The handling for types.Timestamp when the value is a string does not check for null before attempting to parse the string, which could lead to unnecessary processing or errors.

    🔒 Security concerns

    No

    @matrix-meow matrix-meow added the size/S Denotes a PR that changes [10,99] lines label Jun 5, 2024
    @mergify mergify bot requested a review from sukki37 June 5, 2024 07:39
    @mergify mergify bot added the kind/bug Something isn't working label 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
    Add error handling for type assertion in the T_varchar case to prevent runtime panics

    Add error handling for the type assertion in the T_varchar case to avoid potential runtime
    panics if the type assertion fails.

    pkg/frontend/util.go [775-777]

    -if val, ok := row[colIdx].(string); ok {
    -    vData[rowIdx] = val
    -} else {
    +val, ok := row[colIdx].(string)
    +if !ok {
    +    return nil, nil, moerr.NewInternalErrorNoCtx("expected string value at row %d, col %d", rowIdx, colIdx)
    +}
    +vData[rowIdx] = val
     
    Suggestion importance[1-10]: 8

    Why: The suggestion is valid as it addresses a potential runtime panic due to failed type assertion, which is a critical issue that could crash the application.

    8
    Add error handling for type assertion in the T_timestamp case to prevent runtime panics

    Add error handling for the type assertion in the T_timestamp case to avoid potential
    runtime panics if the type assertion fails.

    pkg/frontend/util.go [896-898]

    -switch val := row[colIdx].(type) {
    -case types.Timestamp:
    -    vData[rowIdx] = val
    +val, ok := row[colIdx].(types.Timestamp)
    +if !ok {
    +    return nil, nil, moerr.NewInternalErrorNoCtx("expected Timestamp value at row %d, col %d", rowIdx, colIdx)
    +}
    +vData[rowIdx] = val
     
    Suggestion importance[1-10]: 8

    Why: This suggestion is correct and important as it prevents a potential runtime panic due to type assertion failure in handling T_timestamp, which is crucial for application stability.

    8
    Possible issue
    Add a nil check before setting nulls to prevent potential nil pointer dereference

    Add a check to ensure that bat.Vecs[colIdx] is not nil before calling SetNulls to prevent
    potential nil pointer dereference.

    pkg/frontend/util.go [915]

    -bat.Vecs[colIdx].SetNulls(nsp)
    +if bat.Vecs[colIdx] != nil {
    +    bat.Vecs[colIdx].SetNulls(nsp)
    +}
     
    Suggestion importance[1-10]: 7

    Why: The suggestion to add a nil check before calling SetNulls is a good preventive measure against nil pointer dereference, enhancing the robustness of the code.

    7

    @mergify mergify bot merged commit f900631 into matrixorigin:1.2-dev Jun 5, 2024
    18 checks passed
    @ck89119 ck89119 deleted the fix/16662 branch June 5, 2024 14:15
    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]: 2 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