-
Notifications
You must be signed in to change notification settings - Fork 2.3k
fix(expression): modulo operation issues #1637
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
Merged
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Looks good to me! Reviewed everything up to de0af91 in 1 minute and 8 seconds
More details
- Looked at
26lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. pandasai/data_loader/sql_loader.py:51
- Draft comment:
Duplicated logic: Instead of replacing " % " with " %% " directly here, consider extracting this escaping into a helper function. This will avoid code duplication with view_loader and handle edge cases more robustly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50%
While the suggestion to extract helper functions is generally good practice, I cannot verify the claimed duplication without seeing the view_loader code. The comment makes assumptions about code I cannot see. Additionally, the current implementation is simple and localized - it's not clear that extracting a one-line string replacement would provide significant benefit.
I might be undervaluing the benefits of consistency across the codebase. The reviewer may have valid knowledge about similar code elsewhere.
Without being able to verify the claimed duplication, I cannot be confident this comment is correct or actionable. The current code is simple enough that extraction may not be warranted.
Delete this comment since I cannot verify the claimed code duplication and the current implementation is straightforward enough that extraction is not clearly beneficial.
2. pandasai/data_loader/view_loader.py:107
- Draft comment:
Duplicated modulo escaping logic detected. Consider refactoring this into a shared helper function to improve maintainability and handle edge cases. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment claims there is duplicated logic, but I can only see one instance in this file. The comment requires knowledge of code outside this file to verify the claim. Following our rules, we should not make assumptions about code we cannot see. The comment also doesn't provide specific guidance about where the duplication exists.
I might be missing other files in the codebase where this logic is duplicated. The suggestion to create a helper function could be valid if there really is duplication elsewhere.
However, our rules explicitly state we should ignore cross-file issues and only think about the file we are reviewing. We also need strong evidence to keep a comment.
Without concrete evidence of duplication within this file, and following our rule to ignore cross-file issues, this comment should be removed.
3. pandasai/data_loader/sql_loader.py:51
- Draft comment:
Consider extracting the modulo-sanitization (query.replace(" % ", " %% ") block) into a shared helper to avoid duplication across loaders. Also, a regex-based solution might handle edge cases more robustly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
- The comment suggests avoiding duplication, but I don't see any duplication in the codebase - this is the only instance. 2. The suggestion for regex is speculative - there's no clear evidence of edge cases that would require regex. 3. The current simple string replacement is clear and straightforward. 4. Creating a helper function for a single-use, single-line operation would add unnecessary complexity.
I might be missing other files where this sanitization occurs. The edge cases mentioned could be real problems that I'm not seeing.
Without concrete evidence of duplication or specific edge cases, the suggestions are speculative and would add complexity without clear benefit.
Delete the comment as it suggests solutions to problems (duplication and edge cases) that aren't evidently present in the code.
4. pandasai/data_loader/view_loader.py:106
- Draft comment:
The same modulo operator replacement appears here. Consider refactoring this logic into a common helper, and review the regex pattern for better edge-case handling. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50%
The comment mentions "the same" modulo operator replacement, implying there's another similar replacement elsewhere, but I can't see the other occurrences in this file. Without seeing the other occurrences, I can't verify if refactoring is truly needed. The comment about regex patterns seems speculative since the code uses simple string replacement, not regex.
I might be missing other files where this pattern occurs. The suggestion about regex handling could be valid if there are edge cases not handled by simple string replacement.
However, without seeing concrete evidence of duplication or actual edge cases, this comment is speculative. The current simple solution may be sufficient for the known use cases.
The comment should be deleted as it makes assumptions about code outside this file and makes speculative suggestions without clear evidence of problems.
5. pandasai/data_loader/view_loader.py:68
- Draft comment:
There appears to be a typographical issue in the error message: 'Sources in this schemas {self.schema} are compatible for a view.' It might be intended to say something like 'Sources in this schema {self.schema} are not compatible for a view.' Please review and correct for clarity. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_ThqFFmpr8BQXvlXS
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Fix modulo operation in
execute_query()insql_loader.pyandview_loader.pyby replacing%with%%when parameters are present.execute_query()insql_loader.pyandview_loader.pyby replacing%with%%whenparamsare present.sql_loader.pyview_loader.pyThis description was created by
for de0af91. It will automatically update as commits are pushed.