Conversation
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to 85efa36 in 56 seconds
More details
- Looked at
368lines of code in12files - Skipped
1files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. README.md:125
- Draft comment:
LGTM. The new SQL metrics are well integrated and documented. - Reason this comment was not posted:
Confidence changes required:0%
The PR adds new metrics for SQL code evaluation, specifically SQLSyntaxMatch and SQLASTSimilarity. The implementation seems to correctly handle the formatting and AST comparison for SQL queries. The documentation and tests are updated accordingly to reflect these new metrics. The PR also includes necessary dependency updates in pyproject.toml for sqlparse and sqlglot, which are used in the SQL metrics. Overall, the PR appears to be well-structured and addresses the intended functionality of adding SQL metrics.
Workflow ID: wflow_c4y8tPTwiBNh7Dyg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
I would drop the sql from the name, the file is already in the sql folder making it "obvious"
| @@ -0,0 +1,77 @@ | |||
| from typing import List, Union | |||
|
|
|||
| import sqlparse | |||
There was a problem hiding this comment.
Can avoid sqlparse to format the query? Can we use sqlglot only. It seems sqlglot can format but also optimize in a common format https://github.com/tobymao/sqlglot?tab=readme-ov-file#sql-optimizer
| if isinstance(change, Keep): | ||
| return 0 | ||
| elif isinstance(change, Update): | ||
| return 1.5 # Updates are significant as they imply a modification in function or value. | ||
| elif isinstance(change, Insert) or isinstance(change, Remove): | ||
| return 1 # Inserts and Removes affect the structure and content but are simpler than updates. | ||
| elif isinstance(change, Move): | ||
| return 0.5 # Moves are generally less impactful as they simply change the order. | ||
| return 1 # Default weight for other types of changes |
There was a problem hiding this comment.
Weights should be modifiable through some configuration parameters (see
)
pyproject.toml
Outdated
| sqlparse = "^0.5.0" | ||
| sqlglot = "^23.17.0" |
There was a problem hiding this comment.
I think these packages should be optional/extra
| return {"SQL_Syntax_Match": max_match_score} | ||
|
|
||
|
|
||
| class SQLASTSimilarity(Metric): |
There was a problem hiding this comment.
How do these two metrics handle comments?
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on da2d38b in 4 minutes and 0 seconds
More details
- Looked at
329lines of code in8files - Skipped
1files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_wXck8lQebKerzqVB
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6eb410a in 2 minutes and 54 seconds
More details
- Looked at
264lines of code in5files - Skipped
1files when reviewing. - Skipped posting
0drafted comments based on config settings.
Workflow ID: wflow_cNrQQ3jGozSZ5BIb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Added an AST comparison for SQL based on the weighted changes in diff tree.
Summary:
Enhanced SQL code evaluation with new metrics, updated documentation, and added tests, including specific file paths and a new dependency.
Key points:
SQLSyntaxMatchandSQLASTSimilaritymetrics for SQL code evaluation.README.mdto include new SQL metrics in the metrics table.SQLSyntaxMatchandSQLASTSimilaritymetrics incontinuous_eval/metrics/code/sql/deterministic.py.README.mdand documentation under/docs/src/content/docs/metrics/Code/Deterministic/for new SQL metrics.tests/code_metrics_test.py.pyproject.tomlto includesqlglotas an optional dependency.Generated with ❤️ by ellipsis.dev