betodealmeida commented on PR #35573:
URL: https://github.com/apache/superset/pull/35573#issuecomment-3523303648
> 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)
The processing was already happening during query execution, this PR just
moves is to earlier validation time.
> _Idempotency_: Is _sanitize_sql_expressions() idempotent? If validate() is
called multiple times on the same QueryObject, will it cause issues?
It is idempotent.
> _Cache invalidation_: Will this change invalidate existing cached queries?
If so, should there be a migration note in UPDATING.md?
Yeah, it will invalidate existing cached queries, it's a one time cache
miss. Since this is a bug fix I don't think we need to add to `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]
