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

fix(view_loader): add optional parameters to execute_local_query method #1663

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

maidacundo
Copy link

@maidacundo maidacundo commented Mar 7, 2025


Important

Add optional parameters to execute_local_query in view_loader.py for parameterized queries.

  • Functionality:
    • Add params: Optional[List[Any]] to execute_local_query() in view_loader.py for parameterized queries.
    • Update execute_query() to pass params to execute_local_query().
  • Imports:
    • Add Any and List to imports in view_loader.py.

This description was created by Ellipsis for 8433952. It will automatically update as commits are pushed.

@maidacundo maidacundo requested a review from gventuri March 7, 2025 12:02
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ Changes requested. Reviewed everything up to 2e091b0 in 1 minute and 47 seconds

More details
  • Looked at 33 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 6 drafted comments based on config settings.
1. pandasai/data_loader/view_loader.py:80
  • Draft comment:
    Consider using a more explicit type for 'params' (e.g., Optional[List[Any]]) to clarify expected parameter types.
  • 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%
    The suggestion would make the type hint more precise by specifying that it's a list that can contain any type of elements. However, this is a relatively minor improvement to type hints that doesn't affect functionality. The current hint Optional[list] is technically correct, just less specific. The code will work the same either way.
    The comment points out a valid way to make the type hints more precise. Type hints are important for code maintainability and IDE support.
    While technically correct, this suggestion is more about code style than a critical issue. The existing type hint is functional and clear enough.
    This comment should be removed as it suggests a minor type hint improvement that doesn't materially impact code quality or functionality.
2. pandasai/data_loader/view_loader.py:90
  • Draft comment:
    Ensure that duckdb.sql(query, params) correctly handles parameter binding. If duckdb expects parameters in a different format, adjust accordingly.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 30% <= threshold 50%
    The comment is asking the author to ensure that parameter binding is handled correctly, which is a form of asking the author to double-check their work. This violates the rule against asking the author to ensure behavior is intended. However, it does hint at a potential issue with parameter binding, which could be useful if rephrased to suggest checking for SQL injection vulnerabilities or ensuring correct parameter usage.
3. pandasai/data_loader/view_loader.py:109
  • Draft comment:
    The replacement of ' % ' with ' %% ' for queries that have parameters is error-prone. Consider handling parameterized queries without altering the raw SQL string.
  • 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.
4. pandasai/data_loader/view_loader.py:90
  • Draft comment:
    Ensure that db_manager.sql can handle a 'None' value for the 'params' argument. If not, consider adding a fallback to call sql(query).df() when params is None.
  • Reason this comment was not posted:
    Comment looked like it was already resolved.
5. pandasai/data_loader/view_loader.py:99
  • Draft comment:
    Passing 'params' to execute_local_query in execute_query is consistent. Ensure that unit tests cover parameterized queries to validate this new functionality.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50%
    The comment violates the rule "Do NOT ask the PR author to confirm their intention, to double-check things, to ensure the behavior is intended, to make sure their change is tested, or similar." Comments starting with "Ensure that..." are explicitly called out as likely not useful. However, one of the additional rules is "Make sure new code has corresponding unit tests". This creates a conflict between rules.
    The comment could be rephrased to be more actionable and direct rather than using "ensure that". Also, we don't know if tests already exist.
    While the phrasing could be better, the underlying request for test coverage of new functionality is valid and matches our additional rules about requiring tests.
    The comment should be rephrased to be more direct, like "Add unit tests to cover the new parameterized query functionality" rather than deleted entirely.
6. pandasai/data_loader/view_loader.py:67
  • Draft comment:
    There appears to be a typographical issue in the error message on line 67: "Sources in this schemas {self.schema} are compatible for a view." The phrasing seems off – consider rephrasing it (perhaps to "Sources in the provided schema {self.schema} are not compatible for a view.") to improve 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_hzfdmoTAyB2oqYDZ


Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link

codecov bot commented Mar 7, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.80%. Comparing base (869f048) to head (8433952).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1663   +/-   ##
=======================================
  Coverage   91.80%   91.80%           
=======================================
  Files          72       72           
  Lines        2795     2795           
=======================================
  Hits         2566     2566           
  Misses        229      229           
Flag Coverage Δ
unittests 91.80% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@gventuri
Copy link
Collaborator

gventuri commented Mar 7, 2025

@maidacundo can we enforce it with tests?

maidacundo and others added 2 commits March 7, 2025 14:31
typing

Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on 43e92b7 in 54 seconds

More details
  • Looked at 23 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. pandasai/data_loader/view_loader.py:1
  • Draft comment:
    Good update of the import for better type clarity.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. pandasai/data_loader/view_loader.py:29
  • Draft comment:
    Refactored dictionary type annotation is clear. Ensure consistency of formatting with other parts of the project.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
3. pandasai/data_loader/view_loader.py:1
  • Draft comment:
    Good update adding 'Any' and 'List' to the typing import. This supports the type annotations for the new optional query parameters in execute_local_query.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. pandasai/data_loader/view_loader.py:29
  • Draft comment:
    The new inline type annotation using dict[str, DatasetLoader] improves readability. If the parentheses aren’t needed for multi-line formatting, consider removing them for brevity.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. pandasai/data_loader/view_loader.py:68
  • Draft comment:
    There is likely a typo in the error message on line 68. The message 'Sources in this schemas {self.schema} are compatible for a view.' seems to be incorrect. It probably should read something like 'Sources in these schemas {self.schema} are not compatible for a view.' Please fix the wording so it accurately reflects the error condition.
  • 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_N6MXXVxBerG745qX


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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! Incremental review on 8433952 in 37 seconds

More details
  • Looked at 17 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. pandasai/data_loader/view_loader.py:29
  • Draft comment:
    The multiline type annotation enhances readability. Ensure consistency with other parts of the codebase and add tests for the optional query parameters functionality in execute_local_query if not already covered.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. pandasai/data_loader/view_loader.py:29
  • Draft comment:
    The reformatting of the type annotation for 'schema_dependencies_dict' improves readability. However, the PR title mentions adding optional parameters to 'execute_local_query', which isn't reflected in the diff. Please update the commit message or include those changes for consistency.
  • Reason this comment was not posted:
    Comment did not seem useful. Confidence is useful = 0% <= threshold 50%
    The comment is asking the PR author to update the commit message or include changes that match the PR title. This violates the rule against asking the PR author to update the PR description or related metadata. The comment does not provide a specific code suggestion or ask for a test to be written, so it should be removed.
3. pandasai/data_loader/view_loader.py:67
  • Draft comment:
    Typographical error: The error message on line 68 reads "Sources in this schemas {self.schema} are compatible for a view." Consider changing 'this schemas' to 'these schemas' for grammatical correctness.
  • 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_CtBoqIkDYMwlco7x


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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants