[cherry-pick-15957] : let preinsert can get outcnt from stats

[jensenojs]

cherry-pick #15957

What type of PR is this?

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

Which issue(s) this PR fixes:

issue #15933

What this PR does / why we need it:

实现 #15933 的子任务

[matrix-meow]

@jensenojs Thanks for your contributions!

Here are review comments for file pkg/sql/colexec/preinsert/types.go:

Pull Request Review:

Title and Body:

The title of the pull request is clear and indicates that it is related to cherry-picking commit 15957. The body provides some context by mentioning the issue number it addresses (#15933) and states that it implements a subtask of that issue. It would be beneficial to have a bit more detailed explanation of the changes made in the pull request to provide better context for reviewers.

Changes in types.go:

1. Addition of EstimatedRowCount field:

   • Problem: The addition of the EstimatedRowCount field without sufficient context or explanation in the pull request body can be confusing for reviewers. It is unclear why this field is necessary and how it relates to the overall functionality of the Argument struct.
   • Suggestion: Provide a clear explanation in the pull request body about the purpose of the EstimatedRowCount field and how it enhances the functionality of the Argument struct. Additionally, consider adding comments in the code to explain the usage and significance of this new field.

2. Optimization Potential:

   • Issue: The changes made in this pull request seem limited to adding a new field to the Argument struct. While this may be a valid improvement, it would be beneficial to include more comprehensive changes or optimizations to make the pull request more impactful.
   • Suggestion: Consider including additional improvements or optimizations in the same pull request to enhance the overall quality of the codebase. This could involve refactoring existing code, improving performance, or addressing other related issues.

Security Concerns:

Based on the provided diff, there are no apparent security concerns in the changes made to the types.go file.

General Suggestions:

• Provide more detailed explanations in the pull request body to help reviewers understand the purpose and impact of the changes.
• Consider including comprehensive changes or optimizations in each pull request to maximize the benefits to the codebase.
• Ensure that new fields or functionalities added to existing structs/classes are well-documented and have clear purposes to avoid confusion for other developers.

By addressing the suggestions mentioned above and providing more context and explanations, the quality and clarity of the pull request can be improved, leading to a more efficient review process and better overall codebase maintenance.

Here are review comments for file pkg/sql/compile/compile.go:

Pull Request Review:

Title and Body:

The title of the pull request is clear and indicates that it is related to cherry-picking commit 15957. The body of the pull request provides some context about the purpose of the changes and references the related issue #15933. It seems to be an improvement type of PR.

Changes in pkg/sql/compile/compile.go:

1. Issue with Variable Naming:

   • The change in the code modifies the assignment of preInsertArg by adding an additional parameter ns to the constructPreInsert function call.
   • The variable ns is not defined or initialized in the provided diff snippet, which could lead to compilation errors or unexpected behavior.
   • Suggestion: Ensure that ns is properly defined and initialized before using it in the constructPreInsert function call.

2. Potential Security Concern:

   • It's important to verify that the addition of the ns parameter to the constructPreInsert function call does not introduce any security vulnerabilities.
   • If ns is related to user input or external data, it should be properly validated and sanitized to prevent injection attacks.
   • Suggestion: Validate the source of ns and implement appropriate input validation mechanisms to mitigate security risks.

3. Code Optimization:

   • The change seems to be straightforward and related to passing an additional argument to a function.
   • Consider reviewing the context in which ns is used and ensure it aligns with the intended functionality.
   • Suggestion: Conduct thorough testing to verify that the addition of ns does not impact the existing logic negatively.

Overall Suggestions:

• Ensure all variables are properly defined and initialized before use to prevent runtime errors.
• Perform a thorough code review to check for any unintended consequences of the added parameter.
• Consider adding comments or documentation to explain the purpose of the ns parameter in the function call.
• Validate input sources and sanitize user inputs to enhance the security of the codebase.

By addressing the issues mentioned above and following the suggestions provided, the quality and security of the codebase can be improved. Conducting thorough testing after making these adjustments is crucial to ensure the stability and reliability of the changes.

Here are review comments for file pkg/sql/compile/operator.go:

Pull Request Review:

Title and Body:

The title of the pull request is clear and concise, indicating that it is a cherry-pick of a specific PR. The body provides some context by mentioning the related issue and the purpose of the PR. It would be beneficial to include more details about the improvements made in the PR to provide a better understanding of the changes.

Changes in pkg/sql/compile/operator.go:

1. Line 483:

   • Addition of arg.EstimatedRowCount = t.EstimatedRowCount in the dupInstruction function.
   • Issue: It seems like this change is related to copying instructions, but without more context, it's unclear why EstimatedRowCount needs to be duplicated. This could potentially lead to incorrect behavior if not handled properly.
   • Suggestion: Ensure that duplicating EstimatedRowCount is necessary and that it won't cause unexpected behavior. Add comments to explain the reason for this duplication.

2. Line 596:

   • Modification of the constructFuzzyFilter function.
   • No issues found in this change.

3. Line 626:

   • Modification of the constructPreInsert function by adding ns []*plan.Node as a parameter and setting arg.EstimatedRowCount based on statistics.
   • Issue: The addition of ns []*plan.Node as a parameter without clear context or explanation in the PR body can lead to confusion about its purpose and usage.
   • Suggestion: Provide a detailed comment explaining the purpose of the new parameter ns []*plan.Node and why EstimatedRowCount is being set based on statistics. Ensure that the statistics are accurate and reliable to avoid incorrect estimations.

Overall Suggestions for Optimization:

• Add detailed comments for each change explaining the rationale behind the modifications to improve code readability and maintainability.
• Consider providing more context in the PR body to help reviewers understand the significance of the changes and their impact on the codebase.
• Verify the accuracy and consistency of the added functionality related to EstimatedRowCount to prevent potential issues with data integrity.

By addressing the issues mentioned above and implementing the suggested optimizations, the quality and clarity of the codebase can be improved, ensuring a more robust and maintainable system.

Here are review comments for file pkg/sql/compile/scopeRemoteRun.go:

Pull Request Review:

Title and Body:

The title of the pull request is clear and indicates that it is a cherry-pick of a specific commit related to allowing preinsert to access outcnt from stats. The body of the pull request provides minimal information about the purpose of the changes and the related issue.

Changes in scopeRemoteRun.go:

1. Issue with convertToPipelineInstruction function:

   • The changes made in the convertToPipelineInstruction function involve adding the EstimatedRowCount field to the PreInsert struct. However, the conversion of t.EstimatedRowCount to int64 may lead to potential data loss if the value is too large for an int64.
   • Suggestion: Ensure that the EstimatedRowCount value is within the range of int64 to prevent data loss. Consider using a data type that can accommodate larger values if needed.

2. Issue with convertToVmInstruction function:

   • Similar to the previous change, the addition of EstimatedRowCount field in the convertToVmInstruction function may lead to data loss if the value is too large for an int64.
   • Suggestion: Validate the range of EstimatedRowCount before converting it to int64 to avoid potential data loss. Consider using a data type that can handle larger values if necessary.

General Suggestions for Optimization:

• Code Consistency: Ensure consistent naming conventions and formatting throughout the codebase to improve readability and maintainability.
• Error Handling: Implement proper error handling mechanisms to gracefully handle unexpected scenarios and improve the robustness of the code.
• Documentation: Add or update relevant comments and documentation to explain the purpose and functionality of the modified code for better understanding by other developers.

Security Concerns:

• Data Loss Risk: The conversion of EstimatedRowCount to int64 without proper validation may lead to data loss or unexpected behavior. It is crucial to handle data types and ranges carefully to prevent security vulnerabilities or data corruption.

Overall, the changes in the pull request aim to enhance the functionality related to preinsert operations. However, it is essential to address the identified issues and consider the suggestions provided to optimize the codebase and ensure its reliability and security.

Here are review comments for file proto/pipeline.proto:

Pull Request Review:

Title:

The title of the pull request indicates that it is a cherry-pick of a specific PR related to allowing preinsert to retrieve outcnt from stats.

Body:

The body of the pull request mentions that this PR is an improvement related to implementing a subtask of issue #15933.

Changes in proto/pipeline.proto:

1. Addition of int64 estimated_row_count = 7; in the PreInsert message:
   • Issue:
     • The addition of int64 estimated_row_count without context or explanation in the PR description can be confusing.
   • Suggestion:
     • Add a brief comment above the int64 estimated_row_count field explaining its purpose and usage.
     • Consider updating the PR description to provide more details on why this field is necessary and how it relates to the overall improvement.

Overall Suggestions:

1. Documentation:

   • Ensure that the purpose and usage of the new estimated_row_count field are clearly documented within the codebase.
   • Update the PR description to include a more detailed explanation of the changes made and their significance in addressing the improvement or subtask related to issue [Tech Request]: use stats of external scan to set auto incr cache #15933.

2. Code Optimization:

   • Consider optimizing the changes by providing more context in the code comments to help future developers understand the rationale behind the modifications.
   • If there are any additional changes or dependencies related to issue [Tech Request]: use stats of external scan to set auto incr cache #15933, ensure they are included and documented appropriately.

3. Security Concerns:

   • As per the provided diff, there are no apparent security concerns identified. However, it is essential to conduct a thorough review of the entire codebase to ensure there are no vulnerabilities introduced by the changes.

By addressing the suggestions mentioned above, the clarity, maintainability, and overall quality of the codebase can be improved. Additionally, providing comprehensive documentation and context for changes will assist in better understanding and maintaining the code in the future.