codeant-ai-for-open-source[bot] commented on PR #36747: URL: https://github.com/apache/superset/pull/36747#issuecomment-3671899626
## Nitpicks 🔍 <table> <tr><td>🔒 <strong>No security issues identified</strong></td></tr> <tr><td>⚡ <strong>Recommended areas for review</strong><br><br> - [ ] <a href='https://github.com/apache/superset/pull/36747/files#diff-262aba4d8ec94601953bb91e7d86568f6c195458a56bee5e43db098f81fb9087R58-R63'><strong>Dialect availability</strong></a><br>Mapping `awsathena` to `Dialects.ATHENA` assumes the installed `sqlglot` version exposes `Dialects.ATHENA`. If a runtime environment uses an older `sqlglot` release that doesn't provide ATHENA, importing/using this mapping may cause runtime failures or unexpected fallbacks. Validate CI/test environments and consider a safe fallback.<br> - [ ] <a href='https://github.com/apache/superset/pull/36747/files#diff-262aba4d8ec94601953bb91e7d86568f6c195458a56bee5e43db098f81fb9087R58-R66'><strong>Parsing behavior changes</strong></a><br>Switching from PRESTO to ATHENA will change sqlglot parsing/AST for some queries. Ensure the unit tests cover edge cases (e.g., constructs that were previously accepted under PRESTO but not under ATHENA) and that downstream codepaths (formatting, transpile, optimize) behave correctly.<br> - [ ] <a href='https://github.com/apache/superset/pull/36747/files#diff-eb80e4a4b5268d6a56c2d04fc89a9a569bc9583de52bd1dc365d6338959f3827R2894-R2905'><strong>Missing Assertion</strong></a><br>The new test `test_awsathena_engine_mapping` calls `statement.format()` but does not assert any outcome. This makes the test weaker — it will only fail on exceptions during parsing, and won't catch regressions where the wrong dialect is used but parsing still succeeds. Consider adding explicit assertions that validate the mapping or the formatted output.<br> - [ ] <a href='https://github.com/apache/superset/pull/36747/files#diff-eb80e4a4b5268d6a56c2d04fc89a9a569bc9583de52bd1dc365d6338959f3827R2894-R2905'><strong>Dialect Mapping Verification</strong></a><br>The goal of the change is to ensure `awsathena` maps to the ATHENA SQLGlot dialect. The test should explicitly verify the mapping (e.g., `SQLGLOT_DIALECTS["awsathena"] == Dialects.ATHENA`) or assert properties of the formatted SQL that demonstrate Athena-specific syntax was handled. Otherwise the test may pass without actually validating the intended configuration.<br> - [ ] <a href='https://github.com/apache/superset/pull/36747/files#diff-262aba4d8ec94601953bb91e7d86568f6c195458a56bee5e43db098f81fb9087R58-R66'><strong>Engine name variants</strong></a><br>The key used is `awsathena`. Database engine identifiers sometimes vary (e.g. `athena`, `aws_athena`, `aws-athena`). If other parts of the codebase or user configs use a different identifier the mapping won't apply and parsing will revert to base behavior. Consider adding common aliases or normalizing engine names before lookup.<br> </td></tr> </table> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
