-
Notifications
You must be signed in to change notification settings - Fork 1.9k
serialization of columns added into the definition of the table #1715
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
Open
matteocacciola
wants to merge
2
commits into
sinaptik-ai:main
Choose a base branch
from
matteocacciola:fix/serialize-dataframe-columns
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
serialization of columns added into the definition of the table #1715
matteocacciola
wants to merge
2
commits into
sinaptik-ai:main
from
matteocacciola:fix/serialize-dataframe-columns
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
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 2d97886 in 43 seconds
More details
- Looked at
107
lines of code in4
files - Skipped
1
files when reviewing. - Skipped posting
10
drafted comments based on config settings.
1. CONTRIBUTING.md:65
- Draft comment:
Updated test commands (make test_all, test_core, test_extensions, test-coverage) are clearly documented. Ensure future updates maintain consistent naming conventions throughout. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
2. pandasai/data_loader/semantic_layer_schema.py:64
- Draft comment:
In the is_expression_valid validator, adding an explicit check for None is good. Consider if empty strings should be allowed or handled similarly. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
3. pandasai/helpers/dataframe_serializer.py:31
- Draft comment:
Serialization now includes a 'columns' attribute with JSON output. This implementation is clear; ensure that any future changes to Column model are reflected in the serializer tests. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
4. tests/unit_tests/helpers/test_dataframe_serializer.py:9
- Draft comment:
Test expectations updated with the new columns JSON attribute. Confirm the nested double quotes are rendered as expected in different environments. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
5. CONTRIBUTING.md:65
- Draft comment:
Good update of test commands. Ensure that any command-line updates in documentation are synced with the makefile targets. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
6. pandasai/data_loader/semantic_layer_schema.py:64
- Draft comment:
The validator now returns None if expression is None. Consider updating the docstring (or adding a comment) to clarify that a missing expression bypasses SQL parsing. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
7. pandasai/helpers/dataframe_serializer.py:31
- Draft comment:
New logic adds a 'columns' attribute with JSON-dumped column definitions. Verify that HTML attribute escaping is robust if column descriptions may include quotes or special characters. - Reason this comment was not posted:
Confidence changes required:0%
<= threshold50%
None
8. tests/unit_tests/helpers/test_dataframe_serializer.py:8
- Draft comment:
The expected output embeds JSON inside an HTML attribute. This literal comparison may be fragile if key ordering changes. Consider parsing the JSON portion and comparing as objects to make tests more robust. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold50%
None
9. CONTRIBUTING.md:56
- Draft comment:
Typo found: On line 56, "We useecodespell
..." should be corrected to "We usecodespell
...". - 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.
10. pandasai/helpers/dataframe_serializer.py:8
- Draft comment:
Minor naming inconsistency: The class is named 'DataframeSerializer' (with a lowercase 'f'), whereas the type hints and commonly used naming conventions (e.g., DataFrame) suggest it might be more consistent to name it 'DataFrameSerializer'. This is a trivial cosmetic issue. - 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_9zWS9G4iItB46wo8
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Hello @ArslanSaleem @gdcsinaptik any chance to consider this PR? |
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
Adds column serialization to
DataframeSerializer.serialize()
and updates tests and documentation accordingly.DataframeSerializer.serialize()
indataframe_serializer.py
now includes column serialization in the table definition.is_expression_valid()
insemantic_layer_schema.py
now returnsOptional[str]
to handleNone
values.test_dataframe_serializer.py
to test new column serialization format.make test_core
,make test_extensions
, andmake test-coverage
inCONTRIBUTING.md
.This description was created by
for 2d97886. It will automatically update as commits are pushed.