Conversation
There was a problem hiding this comment.
I've completed my review and didn't find any issues.
Files scanned
| File Path | Reviewed |
|---|---|
| superset/sql_lab.py | ✅ |
| superset/sql_parse.py | ✅ |
| superset/sql/parse.py | ✅ |
Explore our documentation to understand the languages and file types we support and the files we ignore.
Check out our docs on how you can make Korbit work best for you and your team.
|
Thanks for creating a feature branch for this @betodealmeida. It will help with testing. |
| return SQLStatement(ast=self._parsed.copy(), engine=self.engine) | ||
|
|
||
| optimized = pushdown_predicates(self._parsed, dialect=self._dialect) | ||
| sql = optimized.sql(dialect=self._dialect) |
There was a problem hiding this comment.
Is there a reason we're removing this? Is it redundant?
There was a problem hiding this comment.
Yeah, this is redundant. Before, to create a new statement we had to pass the SQL (a string) and the AST, so here we were generating the SQL. Now we just pass the AST directly.
7c78a62 to
6185eac
Compare
Vitor-Avila
left a comment
There was a problem hiding this comment.
lgtm! did some manual testing as well and didn't notice any issues
6185eac to
616010f
Compare
SUMMARY
This PR removes the
_sqlattribute fromSQLStatementandKustoKQLStatement, keeping only the AST in the class. This is important because in #33473 we modify the AST inplace, which would require callingformat()to keep_sqlin sync. Since the attribute is no longer used in any meaningful way it's better to just get rid of it.Part of #26786, stacked on #33457, #33456, and #33473.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TESTING INSTRUCTIONS
ADDITIONAL INFORMATION