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

test: integration tests for error rate #1542

Merged
merged 15 commits into from
Jan 23, 2025

Conversation

scaliseraoul
Copy link
Contributor

@scaliseraoul scaliseraoul commented Jan 22, 2025


Important

Introduces ViewQueryBuilder for handling view-based queries in DatasetLoader and adds related tests and schema validations.

  • Behavior:
    • DatasetLoader now uses ViewQueryBuilder if schema.source.view is true, otherwise uses QueryBuilder.
    • ViewQueryBuilder formats queries with a WITH statement for views.
    • SemanticLayerSchema raises error if no relations are defined for views.
  • Classes:
    • Adds ViewQueryBuilder extending QueryBuilder to handle view-specific query logic.
  • Tests:
    • Adds test_view_query_builder.py to test ViewQueryBuilder functionality.
    • Adds tests in test_semantic_layer_schema.py for view relation validation.
    • Adds test_agent_chat.py for integration tests with various data scenarios.

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

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jan 22, 2025
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 1b20e4c in 2 minutes and 19 seconds

More details
  • Looked at 595 lines of code in 8 files
  • Skipped 0 files when reviewing.
  • Skipped posting 5 drafted comments based on config settings.
1. pandasai/data_loader/loader.py:148
  • Draft comment:
    Ensure self.query_builder is initialized before using it. Consider raising an exception if it's None.
  • Reason this comment was not posted:
    Comment did not seem useful.
2. pandasai/data_loader/loader.py:152
  • Draft comment:
    Ensure self.query_builder is initialized before using it. Consider raising an exception if it's None.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pandasai/data_loader/view_query_builder.py:11
  • Draft comment:
    Check if the query already contains a WITH clause before adding another one.
  • Reason this comment was not posted:
    Decided after close inspection that this draft comment was likely wrong and/or not actionable:
    This is a valid concern - if format_query() is called with a query that already has a WITH clause, concatenating another WITH statement would create invalid SQL. However, I need to consider if this is actually possible in the codebase's usage. The class seems designed to build views specifically, and the WITH clause is core to its functionality. The parent QueryBuilder likely doesn't handle WITH clauses.
    I don't have visibility into how format_query() is used in the broader codebase. The query parameter could come from anywhere.
    While I don't have full context, the class name ViewQueryBuilder and its purpose strongly suggest it's specifically designed to wrap queries in WITH clauses to create views. Any code using this class would expect this behavior.
    The comment should be deleted. While technically correct, it goes against the class's core purpose of building view queries with WITH clauses.
4. pandasai/data_loader/semantic_layer_schema.py:171
  • Draft comment:
    Check if self.relations is None before iterating over it to avoid TypeError.
  • Reason this comment was not posted:
    Comment was on unchanged code.
5. pandasai/data_loader/semantic_layer_schema.py:172
  • Draft comment:
    The error message can be improved for clarity and conciseness.
                raise ValueError("At least one relation must be defined for a view.")
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_dxDXsxM8XjwKyCUW


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.

tests/unit_tests/agent/test_agent_chat.py Outdated Show resolved Hide resolved
pandasai/data_loader/loader.py Outdated Show resolved Hide resolved
scaliseraoul and others added 4 commits January 22, 2025 16:18
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
… into fix/SIN-310

* 'fix/SIN-310' of https://github.com/scaliseraoul/pandas-ai:
  Update pandasai/data_loader/loader.py
@gventuri gventuri changed the title Fix/sin 310 test: integration tests for error rate Jan 23, 2025
@gventuri gventuri merged commit e5f87cc into sinaptik-ai:release/v3 Jan 23, 2025
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants