eschutho commented on PR #35573: URL: https://github.com/apache/superset/pull/35573#issuecomment-3423957384
I had Claude Code review this PR and we have some follow-up questions: Overall Assessment The core approach looks sound - moving SQL expression sanitization from execution time to validation time is the right solution. The implementation shows good defensive programming with immutability, error handling, and comprehensive tests. I verified the call to _sanitize_sql_expressions() is properly integrated into the validate() method. Follow-up Questions Performance testing: Have you tested this with queries containing many adhoc metrics? Any noticeable performance impact from repeated datasource method calls? (Korbit AI flagged a potential concern about calling datasource.get_metrics_by_name() and processing methods N times without caching) Idempotency: Is _sanitize_sql_expressions() idempotent? If validate() is called multiple times on the same QueryObject, will it cause issues? Cache invalidation: Will this change invalidate existing cached queries? If so, should there be a migration note in UPDATING.md? Edge cases: What happens with: Datasources that don't implement processing methods? (I see tests cover this - good!) SQL expressions containing Jinja templates? (Also covered in tests - good!) Nested adhoc metrics that reference other adhoc metrics? Minor Observations The type ignore comment change (# type: ignore → # type: ignore[misc,index]) - does this address a real mypy error or should the underlying typing be fixed? Comprehensive test coverage looks great! Particularly like the mutation prevention tests. -- 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]
