fix(sql_query): resolve double dash issue in url path #1674
Conversation
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 5ad945f in 2 minutes and 20 seconds
More details
- Looked at
74lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. pandasai/data_loader/local_loader.py:60
- Draft comment:
The error message could be more concise. Consider: 'Unsafe SQL query detected.' to improve clarity. - Reason this comment was not posted:
Comment was on unchanged code.
2. tests/unit_tests/data_loader/test_loader.py:127
- Draft comment:
Similarly, use textwrap.dedent for the SQL strings in test_read_parquet_file_with_mock_query_validator to improve maintainability and avoid stray whitespace. - Reason this comment was not posted:
Marked as duplicate.
3. tests/unit_tests/data_loader/test_loader.py:141
- Draft comment:
The assertion using assert_called_once_with on a multiline string is fragile. Consider normalizing or verifying a substring match to avoid test failures due to formatting. - Reason this comment was not posted:
Marked as duplicate.
4. pandasai/data_loader/local_loader.py:43
- Draft comment:
Consider adding type hints (e.g., sql_query: str -> str) for _replace_readparquet_block_with_table to improve clarity. - Reason this comment was not posted:
Marked as duplicate.
5. pandasai/data_loader/local_loader.py:57
- Draft comment:
Document the rationale for validating a sanitized query while executing the original one. This design assumes READ_PARQUET blocks (with potential '--' in URLs) are excluded only from safety checks. - 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.
6. tests/unit_tests/data_loader/test_loader.py:140
- Draft comment:
The mock assertion is placed inside the pytest.raises block; if the exception is raised, the line may not execute. Consider moving the assert_called_once_with outside the with block. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_SthrGZMwlR7Uuk5i
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.
| path=self.dataset_path, | ||
| ) | ||
|
|
||
| def _replace_readparquet_block_with_table( |
There was a problem hiding this comment.
Add type hints and a docstring for _replace_readparquet_block_with_table. Consider annotating the sql_query parameter (e.g., sql_query: str) to clarify expected input.
| loader = LocalDatasetLoader(sample_schema, "test/test") | ||
| with pytest.raises(RuntimeError): | ||
| loader.execute_query( | ||
| """SELECT |
There was a problem hiding this comment.
Consider using textwrap.dedent to normalize the multiline SQL strings in test_read_parquet_file to avoid formatting issues.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1674 +/- ##
==========================================
+ Coverage 91.97% 91.99% +0.02%
==========================================
Files 72 72
Lines 2791 2799 +8
==========================================
+ Hits 2567 2575 +8
Misses 224 224
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Important
Add method to replace
READ_PARQUETblocks with a dummy table for SQL validation inLocalDatasetLoaderand update tests accordingly._replace_readparquet_block_with_table()inLocalDatasetLoaderto replaceREAD_PARQUETblocks withdummy_tablefor SQL validation.execute_query()inLocalDatasetLoaderto use the new method for query validation.test_read_parquet_fileandtest_read_parquet_file_with_mock_query_validatorintest_loader.pyto testREAD_PARQUETblock replacement and query validation.mock_is_query_safeis called with the modified query intest_read_parquet_file_with_mock_query_validator.This description was created by
for 5ad945f. It will automatically update as commits are pushed.