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(schema): make columns mandatory for view #1599

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

scaliseraoul
Copy link
Contributor

@scaliseraoul scaliseraoul commented Feb 11, 2025


Important

Make columns mandatory for views in SemanticLayerSchema, updating validation logic in check_columns_relations() and handling unspecified column types in is_column_type_supported().

  • Behavior:
    • In check_columns_relations() in semantic_layer_schema.py, columns are now mandatory for views, removing the previous allowance for either columns or relations.
    • Updated is_column_type_supported() to handle cases where column type is not specified.
  • Validation:
    • Ensures that at least one column is defined for a view in SemanticLayerSchema.
    • Removes the requirement for at least one relation or column, focusing solely on columns for views.

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

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 fcbfcda in 1 minute and 47 seconds

More details
  • Looked at 26 lines of code in 1 files
  • Skipped 0 files when reviewing.
  • Skipped posting 3 drafted comments based on config settings.
1. pandasai/data_loader/semantic_layer_schema.py:50
  • Draft comment:
    Consider clarifying the validator logic. Now it skips validation when 'type' is None; update the field docs to reflect that 'None' is allowed or adjust validation if not.
  • 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 documenting or changing behavior that is actually already well-defined. The field is explicitly Optional[str] in the model definition, and the validator's behavior aligns with this by allowing None values. The field docs already reflect this by having description="Data type of the column." on an Optional field. This is standard Pydantic behavior and doesn't need additional clarification.
    I might be overlooking the value of explicit documentation even for standard behavior. Some developers might not be familiar with Pydantic's Optional field handling.
    While explicit documentation can be helpful, in this case the code follows standard Pydantic patterns and the optionality is already clearly indicated in the type annotation and field definition.
    The comment should be deleted as it suggests documenting behavior that is already clearly defined through standard Pydantic patterns and type annotations.
2. pandasai/data_loader/semantic_layer_schema.py:50
  • Draft comment:
    Consider using an explicit check (e.g. 'if type is not None and type not in VALID_COLUMN_TYPES:') instead of 'if type and ...' to avoid skipping validation for falsy values like an empty string.
  • Reason this comment was not posted:
    Marked as duplicate.
3. pandasai/data_loader/semantic_layer_schema.py:312
  • Draft comment:
    The error message could be improved for clarity. Consider changing it to: 'At least one column must be defined for a view.' Also, ensure unit tests cover this validation scenario.
  • Reason this comment was not posted:
    Marked as duplicate.

Workflow ID: wflow_8a8zFSbP0LHUgrlh


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.

scaliseraoul and others added 2 commits February 11, 2025 12:13
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
@gventuri gventuri merged commit 1071749 into sinaptik-ai:main Feb 11, 2025
12 checks passed
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.

2 participants