cherry-pick bug fixes to 1.1-dev

[aunjgr]

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #15196

What this PR does / why we need it:

PrefixCompare truncates the left operand if it's longer than the right operand, but not vice versa. So we MUST always put the composite key at the left side, and the matching prefix at the right side.

[matrix-meow]

@aunjgr Thanks for your contributions!

Here are review comments for file pkg/container/types/bytes.go:

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that bug fixes are being cherry-picked to the 1.1-dev branch.

Body:

The body of the pull request provides useful information about the type of PR (BUG), the specific issue it fixes (issue #15196), and the reason for the changes made. It explains the issue related to PrefixCompare function and the necessary adjustment to ensure correct behavior.

Changes Made:

1. Added Import Statement:

   • An import statement for the "bytes" package was added to the file pkg/container/types/bytes.go. This change seems necessary for the bytes.Compare function used later in the code.

2. Added PrefixCompare Function:

   • A new function PrefixCompare was added to compare byte slices. The function ensures that the comparison is done correctly by truncating the longer slice to match the length of the shorter slice before comparison.

Suggestions for Improvement:

1. Error Handling:

   • It's important to handle potential errors when working with slices and comparing bytes. Ensure that appropriate error handling mechanisms are in place, such as checking for nil slices or unexpected lengths.

2. Unit Tests:

   • It would be beneficial to add unit tests for the newly added PrefixCompare function to validate its correctness and ensure it behaves as expected in different scenarios.

3. Documentation:

   • Consider adding comments/documentation to the PrefixCompare function to explain its purpose, expected inputs, and return values. This will help other developers understand the function's behavior without diving into the implementation details.

4. Optimization:

   • Since the function modifies the input slice lhs by truncating it, consider whether creating a copy of the slice before truncating would be more appropriate to avoid unintended side effects.

5. Consistency:

   • Ensure consistency in coding style and formatting throughout the file to maintain readability and make the codebase more maintainable.

6. Security Concerns:

   • While the changes made in this PR seem focused on functionality, it's essential to always consider potential security vulnerabilities, especially when working with byte manipulation and comparisons. Perform a thorough review to identify and address any security risks.

By addressing the suggestions mentioned above, the codebase can be improved in terms of functionality, maintainability, and security. It's crucial to maintain a high standard of code quality to ensure the stability and security of the application.

Here are review comments for file pkg/container/vector/search.go:

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that bug fixes are being cherry-picked to the 1.1-dev branch.

Body:

The body of the pull request provides relevant information about the type of PR (BUG), the specific issue it fixes (issue #15196), and the reason for the changes made in the PR. It explains the issue related to PrefixCompare and the necessity of placing the composite key on the left side and the matching prefix on the right side.

Changes:

1. Addition of New Functions:

   • Three new functions have been added to the search.go file: CollectOffsetsByPrefixEqFactory, CollectOffsetsByPrefixBetweenFactory, and CollectOffsetsByPrefixInFactory.

2. Functionality:

   • These functions are designed to collect offsets based on prefix comparisons for vectors.
   • They handle scenarios where specific prefixes need to be matched within the vectors.

3. Code Optimization:

   • The functions utilize efficient methods like bytes.Compare, bytes.HasPrefix, and sort.Search for comparison and searching operations.
   • The use of make to pre-allocate slices and append to add elements incrementally is appropriate for performance.

Suggestions for Improvement:

1. Error Handling:

   • Ensure proper error handling mechanisms are in place, especially when dealing with byte slices and comparisons to prevent potential panics or unexpected behavior.

2. Consistency in Naming:

   • Maintain consistency in naming conventions across functions and variables to enhance code readability and maintainability.

3. Code Duplication:

   • Check for any code duplication within the new functions and consider refactoring common logic into separate reusable functions to promote code reusability.

4. Unit Testing:

   • Write comprehensive unit tests for the newly added functions to validate their functionality and ensure they work as expected in different scenarios.

5. Security Concerns:

   • Validate user input to prevent any potential injection attacks, especially when dealing with byte slices and comparisons.

6. Documentation:

   • Add inline comments or documentation to explain the purpose of each function, parameters, and expected behavior for better code understanding.

Overall, the changes made in the pull request seem to address a specific bug and enhance the functionality of the vector search operations. By incorporating the suggested improvements, the codebase can be further optimized for performance, maintainability, and security.

Here are review comments for file pkg/container/vector/utils_test.go:

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that bug fixes are being cherry-picked to the 1.1-dev branch.

Body:

The body of the pull request provides relevant information about the type of PR (BUG), the specific issue it fixes (issue #15196), and the reason for the changes made in the PR. The explanation about PrefixCompare and the necessity of placing the composite key on the left side and the matching prefix on the right side is helpful in understanding the context of the changes.

Changes:

1. Addition of Test Functions:

   • Two new test functions TestCollectOffsetsByPrefixEqFactory and TestCollectOffsetsByPrefixBetweenFactory have been added to the utils_test.go file.
   • These test functions are testing the behavior of functions related to collecting offsets based on prefix equality and between two prefixes.

2. Code Changes in Test Functions:

   • The test functions involve creating a vector, appending bytes to it, defining prefixes, and then testing the functions that collect offsets based on these prefixes.
   • Assertions using require.Equal are made to validate the expected results of the offset collections.

Suggestions for Improvement:

1. Code Optimization:

   • Consider refactoring the test functions to reduce code duplication. Since both test functions have similar setup steps, extracting common setup code into a helper function can improve maintainability.
   • Utilize table-driven tests to cover different scenarios with varying inputs and expected outputs, enhancing test coverage and readability.

2. Error Handling:

   • Ensure proper error handling in the test functions. If any of the assertions fail, provide clear error messages to aid in debugging.

3. Documentation:

   • Add comments to the test functions to explain the purpose of each test case and any specific edge cases being covered.

4. Security Concerns:

   • Ensure that sensitive data or hardcoded values are not exposed in the test cases, especially when dealing with real-world data scenarios.

5. Consistency:

   • Maintain consistency in naming conventions and coding style throughout the test functions to enhance code readability and maintainability.

6. Review Test Coverage:

   • Verify that the new test functions cover all relevant scenarios and edge cases to ensure comprehensive testing of the functionality.

By addressing the above suggestions, the code quality, maintainability, and reliability of the test suite can be improved, leading to a more robust codebase.

Here are review comments for file pkg/sql/plan/function/func_prefix.go:

Pull Request Review:

Title and Body:

The title of the pull request is clear and concise, indicating that bug fixes are being cherry-picked to the 1.1-dev branch. The body of the pull request provides relevant information about the type of PR, the specific issue it fixes, and the reason for the changes made. It explains the issue with PrefixCompare and the necessary adjustment to ensure correct behavior. The body could be improved by providing more context on the impact of the bug and the importance of the fix.

Changes Made:

1. Import Statement Change:

   • The import statement for index has been removed, and types has been added. This change seems appropriate if types now contains the necessary functions for PrefixCompare.

2. Function Changes:

   • The function PrefixBetween and PrefixIn have been modified to use types.PrefixCompare instead of index.PrefixCompare. This change is consistent with the import modification and aligns the function calls with the updated implementation.

Issues and Suggestions:

1. Unused Import Removal:

   • The import statement for index has been removed, but it might be a good practice to verify if there are any other references to index in the file and remove them to keep the code clean and avoid confusion.

2. Consistency in Function Calls:

   • Ensure that all occurrences of index.PrefixCompare have been replaced with types.PrefixCompare throughout the file to maintain consistency and avoid potential bugs due to mixed function calls.

3. Code Optimization:

   • Consider refactoring the functions PrefixBetween and PrefixIn to reduce code duplication and improve readability. Extracting common logic into helper functions could make the code more maintainable.

4. Security Concerns:

   • While the changes seem focused on bug fixes, it's essential to ensure that the new implementation using types.PrefixCompare does not introduce any security vulnerabilities. Perform thorough testing to validate the correctness and security of the updated code.

5. Documentation Update:

   • It would be beneficial to update any relevant documentation or comments in the code to reflect the changes made, especially regarding the switch from index.PrefixCompare to types.PrefixCompare.

Overall, the pull request addresses the bug related to PrefixCompare effectively, but attention to the mentioned suggestions can further enhance the code quality and maintainability. Conducting thorough testing, especially focusing on the security implications of the changes, is crucial before merging the pull request.

Here are review comments for file pkg/vm/engine/disttae/util.go:

Pull Request Review:

Title:

The title of the pull request "cherry-pick bug fixes to 1.1-dev" is clear and indicates that bug fixes are being applied to the 1.1-dev branch.

Body:

The body of the pull request provides relevant information about the type of PR (BUG), the specific issue it fixes (issue #15196), and the reason for the changes made in the PR. It explains the issue with PrefixCompare and the necessary adjustments to ensure correct behavior.

Changes:

1. In the getPkExpr function in pkg/vm/engine/disttae/util.go, several changes have been made to handle cases where the left operand is truncated if longer than the right operand. The changes involve using GetCol() method to get the column, and ensuring that the composite key is always on the left side and the matching prefix on the right side.

2. Additional cases have been added for "prefix_eq", "prefix_between", and "prefix_in" in the getPkExpr function to handle prefix comparisons. These cases check if the column name matches the provided key name and return the appropriate expression.

3. In the getNonCompositePKSearchFuncByExpr function, changes have been made to handle cases related to prefix comparisons. Functions like CollectOffsetsByPrefixEqFactory, CollectOffsetsByPrefixBetweenFactory, and CollectOffsetsByPrefixInFactory are used based on the function object name.

Suggestions for Improvement:

1. Consistency in Error Handling: Ensure consistent error handling throughout the codebase. It's important to handle errors consistently and provide meaningful error messages or logging where necessary.

2. Code Optimization: Consider refactoring repetitive code segments to improve readability and maintainability. Look for opportunities to extract common functionality into separate functions to reduce duplication and improve code organization.

3. Security Considerations: When dealing with user input or external data, ensure proper validation and sanitization to prevent security vulnerabilities like SQL injection or code injection attacks. Validate inputs before processing them to avoid potential security risks.

4. Documentation: Add or update comments and documentation to explain the purpose of functions, variables, and logic to make the code more understandable for future developers and maintainers.

5. Testing: Ensure that the changes made in the PR are covered by appropriate unit tests to validate the functionality and prevent regressions. Test edge cases and error scenarios to ensure robustness.

6. Performance Optimization: Analyze the performance impact of the changes and consider optimizations if necessary. Look for opportunities to improve efficiency without sacrificing readability or maintainability.

By addressing these suggestions, the codebase can be enhanced in terms of security, maintainability, and performance.

Overall, the changes made in the pull request seem to address the identified bug and improve the handling of prefix comparisons. However, the suggestions provided above can further enhance the quality and robustness of the codebase.

Here are review comments for file pkg/vm/engine/tae/blockio/read.go:

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that bug fixes are being cherry-picked to the 1.1-dev branch.

Body:

1. The PR type is specified as a bug fix.
2. The PR is linked to a specific issue ([Bug]: Problem with RuntimeFilter of SEMI join, when we use 2 conditions in OnList #15196).
3. The explanation provided in the body clarifies the reason for the changes made in the PR, highlighting the issue with PrefixCompare and the required adjustment.

Changes:

1. Imports Reorganization:

   • Removed unused imports (github.com/matrixorigin/matrixone/pkg/container/vector, github.com/matrixorigin/matrixone/pkg/util/metric/v2, go.uber.org/zap).
   • Added missing import for github.com/matrixorigin/matrixone/pkg/container/vector.

2. Code Optimization:

   • Reordered the import statements for better readability.
   • Removed redundant imports to declutter the code.

Suggestions for Improvement:

1. Unused Imports:

   • It's good that unused imports were removed, but ensure that all unused imports are eliminated throughout the codebase to maintain cleanliness and reduce potential conflicts or confusion.

2. Consistent Formatting:

   • Ensure consistent formatting of import statements (alphabetical order, grouping by standard library, third-party, and local imports) for better code organization and readability.

3. Code Review:

   • Consider reviewing the entire file for any other unused code, redundant functions, or outdated comments that can be cleaned up to enhance code quality.

4. Security Concerns:

   • While the changes in this PR do not introduce security vulnerabilities, it's crucial to conduct regular security audits and ensure that sensitive data handling and external dependencies are secure.

5. Testing:

   • Verify that the bug fixes applied in this PR are accompanied by appropriate test cases to prevent regression and ensure the stability of the codebase.

By addressing the suggestions above, the codebase can be improved in terms of readability, maintainability, and overall quality. Additionally, incorporating automated tools for code analysis and testing can further enhance the development process.

Here are review comments for file pkg/vm/engine/tae/blockio/reader.go:

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that bug fixes are being cherry-picked to the 1.1-dev branch.

Body:

The body of the pull request provides essential information about the type of PR, the specific issue it fixes, and the reason for the changes. It also highlights the specific file where the changes are made and the nature of the modifications.

Changes:

The changes made in the pull request involve adding an import statement in the reader.go file of the pkg/vm/engine/tae/blockio directory. The added import statement is for the context package.

Feedback and Suggestions:

1. Code Modification:

   • The added import statement for the context package seems unrelated to the mentioned bug fix in the body of the PR. It's important to ensure that the changes made align with the purpose of the PR. If the import is necessary for the bug fix, it should be explicitly mentioned in the body of the PR.

2. Security Concern:

   • While the changes provided do not indicate any direct security issues, it's crucial to ensure that the modifications made do not introduce vulnerabilities. Reviewing the entire context of the changes and considering any potential security implications is recommended.

3. Optimization:

   • Since the changes in the pull request seem minimal, it might be beneficial to include all relevant modifications related to the bug fix in a single commit. This can help in maintaining a clean commit history and make it easier to track changes.

4. Documentation:

   • It would be helpful to provide more detailed information in the body of the PR regarding the bug that is being fixed and the impact of the changes. Clear documentation can aid in understanding the rationale behind the modifications.

5. Code Consistency:

   • Ensure that the coding style and formatting of the added import statement align with the existing codebase to maintain consistency and readability.

6. Testing:

   • It's essential to include relevant tests to validate the bug fixes and ensure that the changes do not introduce new issues. Adding test cases can enhance the reliability of the codebase.

Overall:

The pull request addresses bug fixes and includes necessary changes in the reader.go file. However, it's important to ensure that the modifications are directly related to the bug fix and do not introduce any security vulnerabilities. Providing more detailed information in the PR body and optimizing the changes for clarity and consistency can further enhance the quality of the codebase.

Here are review comments for file pkg/vm/engine/tae/index/zm.go:

Pull Request Review:

Title:

The title of the pull request is clear and concise, indicating that bug fixes are being cherry-picked to the 1.1-dev branch.

Body:

The body of the pull request provides essential information about the type of PR, the specific issue it fixes, and the reason for the changes made. It explains the issue related to PrefixCompare function and the changes made in the zm.go file.

Changes:

1. The PrefixCompare function has been removed from the zm.go file, and instead, calls to types.PrefixCompare have been added in the PrefixEq, PrefixBetween, and PrefixIn functions.

2. The comparison logic in the PrefixEq, PrefixBetween, and PrefixIn functions has been updated to use types.PrefixCompare consistently for comparing byte slices.

Feedback and Suggestions:

1. Security Issue:

   • The removal of the PrefixCompare function and the introduction of types.PrefixCompare without any context raises a security concern. It's crucial to ensure that the types.PrefixCompare function is properly implemented and secure to prevent any vulnerabilities related to byte slice comparison. Verify that the types.PrefixCompare function handles edge cases and potential security risks such as buffer overflows or incorrect comparisons.

2. Code Optimization:

   • Instead of directly replacing PrefixCompare with types.PrefixCompare in multiple places, consider refactoring the code to reduce redundancy. You could create a helper function within the zm struct or a utility package to handle prefix comparisons consistently.

3. Consistency:

   • Ensure consistent use of types.PrefixCompare throughout the codebase to maintain clarity and avoid confusion. Make sure that all similar comparisons follow the same pattern to improve code readability and maintainability.

4. Testing:

   • After making changes related to comparison functions, it's essential to update or add relevant test cases to cover the new logic thoroughly. Test different scenarios to validate the correctness and robustness of the comparison operations.

5. Documentation:

   • Consider adding comments or updating existing documentation to explain the rationale behind the changes made, especially regarding the decision to switch from PrefixCompare to types.PrefixCompare. Clear documentation can help future developers understand the code more easily.

6. Review and Collaboration:

   • Encourage collaboration and review among team members to ensure that the changes align with the project's standards and best practices. Peer reviews can help catch potential issues early and improve the overall quality of the codebase.

By addressing the security concern, optimizing the code for better efficiency, ensuring consistency, updating documentation, and thorough testing, the pull request can be enhanced to contribute positively to the codebase.