Support sql check #44
Merged
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
![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 6d04ff0 in 1 minute and 10 seconds
More details
- Looked at
187lines of code in5files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:108
- Draft comment:
Add a return type annotation to theparse_querymethod for clarity. It should returnOptional[sqlglot.Expression]or similar based on the actual return type ofparse_one. - Reason this comment was not posted:
Confidence changes required:50%
Theparse_querymethod is missing a return type annotation, which is a best practice for clarity.
2. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:13
- Draft comment:
Correct the import path forSqlInsight. It should be imported fromdatapilot.core.platforms.dbt.insights.sql.baseinstead ofdatapilot.core.insights.sql.base.insight. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests a change in the import path, but without additional context or file structure, it's difficult to verify if the suggested path is correct. The current path might be correct if the file structure supports it. Since the comment is speculative and lacks strong evidence, it should be removed unless there's clear evidence that the path is incorrect.
I might be missing the actual file structure or context that confirms the correct import path. The comment could be valid if the file structure supports the suggested path.
Without clear evidence or file structure context, it's safer to remove the comment. If the import path was incorrect, it would likely cause an error during the build or runtime.
Remove the comment as it lacks strong evidence and is speculative about the import path.
Workflow ID: wflow_WwhQQrVJFwZOSsNR
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.
| "quote_identifiers": False, | ||
| **kwargs, | ||
| } | ||
| raise "test" |
There was a problem hiding this comment.
Remove the raise "test" statement. It seems to be a placeholder for testing and will cause the function to raise an exception unnecessarily.
Suggested change
| raise "test" |
| from datapilot.core.platforms.dbt.insights.base import DBTInsight | ||
|
|
||
|
|
||
| class SqlInsight(DBTInsight): |
There was a problem hiding this comment.
The has_all_required_data method is a duplicate of the method in DBTGovernanceInsight. Consider using the existing method to avoid duplication.
- method
has_all_required_data(base.py)
mdesmet
force-pushed
the
feat/sql-check
branch
from
6d04ff0 to
a4c0ee1
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on a4c0ee1 in 51 seconds
More details
- Looked at
479lines of code in11files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:105
- Draft comment:
Theparse_querymethod is defined but never used. Consider removing it if it's not needed. - Reason this comment was not posted:
Confidence changes required:50%
Theparse_querymethod inSqlCheckis defined but never used. This might be an oversight or a leftover from previous code iterations.
2. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:58
- Draft comment:
The docstring incorrectly mentions test coverage. Update it to reflect that the method generates SQL optimization insights. - Reason this comment was not posted:
Marked as duplicate.
3. src/datapilot/core/platforms/dbt/insights/sql/base.py:11
- Draft comment:
Ensure that subclasses ofSqlInsightimplement thegeneratemethod, as it is abstract. - Reason this comment was not posted:
Confidence changes required:0%
Thegeneratemethod inSqlInsightclass inbase.pyis not implemented, which is expected for an abstract method. No issues here.
4. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:99
- Draft comment:
self.ALIASis used but not defined inSqlCheck. DefineALIASin the class to avoid AttributeError. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is incorrect becauseALIASis defined in theSqlCheckclass. The comment suggests a change that is not needed, as the code is already correct. Therefore, the comment should be removed.
I might be missing some context or misunderstanding the comment, but based on the provided code,ALIASis clearly defined, making the comment incorrect.
The code snippet provided is complete, andALIASis defined, so the comment is indeed incorrect.
The comment should be deleted because it incorrectly states thatALIASis not defined, while it is clearly defined in the class.
Workflow ID: wflow_iY627wgELqaJP6Db
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.
mdesmet
force-pushed
the
feat/sql-check
branch
from
a4c0ee1 to
f3f20ca
Compare
There was a problem hiding this comment.
❌ Changes requested. Incremental review on f3f20ca in 51 seconds
More details
- Looked at
483lines of code in11files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:59
- Draft comment:
The docstring is misleading. It mentions test coverage, which is unrelated to SQL optimization. Please update the docstring to accurately describe the purpose of thegeneratemethod. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_xRuhagdbhcfqeVnG
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.
suryaiyer95
reviewed
Nov 15, 2024
suryaiyer95
reviewed
Nov 15, 2024
suryaiyer95
reviewed
Nov 15, 2024
mdesmet
force-pushed
the
feat/sql-check
branch
from
f3f20ca to
b0147c9
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on b0147c9 in 21 seconds
More details
- Looked at
361lines of code in11files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. src/datapilot/core/insights/sql/base/insight.py:2
- Draft comment:
AddOptionalto the import statement for type hinting consistency.
from typing import Optional
- Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment is suggesting a change that is not necessary because the 'Optional' type is not used anywhere in the current file. The removal of 'Optional' seems intentional and consistent with the current code, which does not use it.
I might be missing some context about why 'Optional' was initially included, but based on the current file content, it seems unnecessary.
The absence of 'Optional' in the current code suggests that its removal was intentional and correct. Without its usage, the import is redundant.
The comment should be deleted because it suggests an unnecessary import that is not used in the current file.
Workflow ID: wflow_UPRXs3C0PXsGIPiF
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
mdesmet
force-pushed
the
feat/sql-check
branch
from
b0147c9 to
4863600
Compare
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 4863600 in 29 seconds
More details
- Looked at
356lines of code in11files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. src/datapilot/core/insights/sql/base/insight.py:3
- Draft comment:
The import statement forDialectis removed but still used in the code. This will cause a NameError whenDialectis referenced. Please re-importDialectif it's used elsewhere in the code. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests that 'Dialect' is still used in the code, but the provided file content shows no reference to 'Dialect'. The constructor that used 'Dialect' was removed, so the comment seems incorrect.
I might be missing references to 'Dialect' in other parts of the codebase, but the task is to focus only on the changes in this file.
Given the context of the file, there is no evidence that 'Dialect' is still needed. The comment seems to be based on outdated information.
The comment should be deleted as it is not relevant to the current state of the file.
2. src/datapilot/core/insights/sql/base/insight.py:10
- Draft comment:
Thegeneratemethod should specify the return type asdictin the method signature for clarity. - Reason this comment was not posted:
Confidence changes required:50%
Thegeneratemethod inSqlInsightclass should return a dictionary, but the return type is not specified in the method signature. This can lead to confusion about the expected return type.
3. src/datapilot/core/platforms/dbt/insights/sql/base.py:11
- Draft comment:
Thegeneratemethod should specify the return type asdictin the method signature for clarity. - Reason this comment was not posted:
Confidence changes required:50%
Thegeneratemethod inSqlInsightclass should return a dictionary, but the return type is not specified in the method signature. This can lead to confusion about the expected return type.
4. src/datapilot/core/platforms/dbt/insights/sql/sql_check.py:59
- Draft comment:
Thegeneratemethod should specify the return type asList[DBTModelInsightResponse]in the method signature for clarity. - Reason this comment was not posted:
Confidence changes required:50%
Thegeneratemethod inSqlCheckclass should return a list ofDBTModelInsightResponse, but the return type is not specified in the method signature. This can lead to confusion about the expected return type.
Workflow ID: wflow_NlJSQMZp6KdgxvX2
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
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
SqlCheckclass to identify SQL optimization issues in DBT models usingsqlglot, and updates dependencies insetup.py.SqlCheckclass insql_check.pyto identify SQL optimization issues in DBT models.SqlCheckintoINSIGHTSlist in__init__.py.sqlglotlibrary for SQL parsing and optimization.sqlglottoinstall_requiresinsetup.py.SqlCheckextendsSqlInsightinbase.py.generate()inSqlCheckto apply optimization rules and generate insights._build_failure_result()inSqlCheckto construct failure messages and recommendations.get_adapter_type()method inwrapper.pyand its versioned counterparts.This description was created by
for 4863600. It will automatically update as commits are pushed.