You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@superset.apache.org by "betodealmeida (via GitHub)" <gi...@apache.org> on 2025/10/08 17:16:37 UTC
[PR] Fix query cache [superset]
betodealmeida opened a new pull request, #35573:
URL: https://github.com/apache/superset/pull/35573
<!---
Please write the PR title following the conventions at https://www.conventionalcommits.org/en/v1.0.0/
Example:
fix(dashboard): load charts correctly
-->
### SUMMARY
<!--- Describe the change below, including rationale and design decisions -->
### BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
<!--- Skip this if not applicable -->
### TESTING INSTRUCTIONS
<!--- Required! What steps can be taken to manually verify the changes? -->
### ADDITIONAL INFORMATION
<!--- Check any relevant boxes with "x" -->
<!--- HINT: Include "Fixes #nnn" if you are fixing an existing issue -->
- [ ] Has associated issue:
- [ ] Required feature flags:
- [ ] Changes UI
- [ ] Includes DB Migration (follow approval process in [SIP-59](https://github.com/apache/superset/issues/13351))
- [ ] Migration is atomic, supports rollback & is backwards-compatible
- [ ] Confirm DB migration upgrade and downgrade tested
- [ ] Runtime estimates and downtime expectations provided
- [ ] Introduces new feature or API
- [ ] Removes existing feature or API
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "rusackas (via GitHub)" <gi...@apache.org>.
rusackas commented on PR #35573:
URL: https://github.com/apache/superset/pull/35573#issuecomment-3462818863
Just needs a PR description :D
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on code in PR #35573:
URL: https://github.com/apache/superset/pull/35573#discussion_r2445340105
##########
superset/common/query_object.py:
##########
@@ -359,6 +360,104 @@ def _sanitize_filters(self) -> None:
except QueryClauseValidationException as ex:
raise QueryObjectValidationError(ex.message) from ex
+ def _sanitize_sql_expressions(self) -> None:
+ """
+ Sanitize SQL expressions in adhoc metrics and orderby for consistent cache keys.
+
+ This processes SQL expressions before cache key generation, preventing cache
+ mismatches due to later processing during query execution.
+ """
+ if not self.datasource or not hasattr(
+ self.datasource,
+ "_process_sql_expression",
+ ):
+ return
+
+ # Process adhoc metrics
+ if self.metrics:
+ self._sanitize_metrics_expressions()
+
+ # Process orderby - these may contain adhoc metrics
+ if self.orderby:
+ self._sanitize_orderby_expressions()
+
+ def _sanitize_metrics_expressions(self) -> None:
+ """
+ Process SQL expressions in adhoc metrics.
+ Creates new metric dictionaries to avoid mutating shared references.
+ """
+ # datasource is checked in parent method, assert for type checking
+ assert self.datasource is not None
+
+ if not self.metrics:
+ return
+
+ sanitized_metrics = []
+ for metric in self.metrics:
+ if not (is_adhoc_metric(metric) and isinstance(metric, dict)):
+ sanitized_metrics.append(metric)
+ continue
+ if sql_expr := metric.get("sqlExpression"):
+ try:
+ processed = self.datasource._process_select_expression(
+ expression=sql_expr,
+ database_id=self.datasource.database_id,
+ engine=self.datasource.database.backend,
+ schema=self.datasource.schema,
+ template_processor=None,
+ )
+ if processed and processed != sql_expr:
+ # Create new dict to avoid mutating shared references
+ sanitized_metrics.append({**metric, "sqlExpression": processed})
+ else:
+ sanitized_metrics.append(metric)
+ except Exception as ex: # pylint: disable=broad-except
+ # If processing fails, leave as-is and let execution handle it
+ logger.debug("Failed to sanitize metric SQL expression: %s", ex)
+ sanitized_metrics.append(metric)
+ else:
+ sanitized_metrics.append(metric)
Review Comment:
### Inefficient metric processing without caching <sub></sub>
<details>
<summary>Tell me more</summary>
###### What is the issue?
The method creates a new list and appends items one by one instead of using list comprehension, and performs expensive SQL processing operations for each metric without any caching mechanism.
###### Why this matters
This approach is inefficient for large metric lists and may cause repeated expensive SQL processing operations. The lack of caching means identical SQL expressions will be processed multiple times across different query objects.
###### Suggested change ∙ *Feature Preview*
Use list comprehension where possible and implement caching for processed SQL expressions. Consider using `functools.lru_cache` or a class-level cache to store processed expressions:
```python
from functools import lru_cache
@lru_cache(maxsize=128)
def _cached_process_select_expression(self, sql_expr, database_id, engine, schema):
return self.datasource._process_select_expression(
expression=sql_expr,
database_id=database_id,
engine=engine,
schema=schema,
template_processor=None,
)
```
###### Provide feedback to improve future suggestions
[](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c265a0d-ac46-4508-888a-907e07e56e40/upvote) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c265a0d-ac46-4508-888a-907e07e56e40?what_not_true=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c265a0d-ac46-4508-888a-907e07e56e40?what_out_of_scope=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c265a0d-ac46-4508-888a-907e07e56e40?what_not_in_standard=true) [](https://app.korbit.ai/feedback/aa91ff46-6083-4491-9416-b83dd1994b51/5c265a0d-ac46-4508-888a-907e07e56e40)
</details>
<sub>
💬 Looking for more details? Reply to this comment to chat with Korbit.
</sub>
<!--- korbi internal id:e7175c76-f3ca-4ccd-b96d-53f71d8bf15a -->
[](e7175c76-f3ca-4ccd-b96d-53f71d8bf15a)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #35573:
URL: https://github.com/apache/superset/pull/35573#issuecomment-3564810037
Better fix in https://github.com/apache/superset/pull/36225
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "sadpandajoe (via GitHub)" <gi...@apache.org>.
sadpandajoe commented on PR #35573:
URL: https://github.com/apache/superset/pull/35573#issuecomment-3433439019
@betodealmeida can you add a PR description?
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Fix query cache [superset]
Posted by "korbit-ai[bot] (via GitHub)" <gi...@apache.org>.
korbit-ai[bot] commented on PR #35573:
URL: https://github.com/apache/superset/pull/35573#issuecomment-3382510973
Based on your review schedule, I'll hold off on reviewing this PR until it's marked as ready for review. If you'd like me to take a look now, comment `/korbit-review`.
> Your admin can change your review schedule in the [Korbit Console](https://app.korbit.ai/settings)
[](208e990e-957d-41fe-be07-022a9f82466d)
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida closed pull request #35573: fix: prevent cache key mismatch
URL: https://github.com/apache/superset/pull/35573
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] Fix query cache [superset]
Posted by "bito-code-review[bot] (via GitHub)" <gi...@apache.org>.
bito-code-review[bot] commented on PR #35573:
URL: https://github.com/apache/superset/pull/35573#issuecomment-3382512389
**Bito Automatic Review Skipped - Draft PR**<br/><br/>Bito didn't auto-review because this pull request is in draft status.<br/>No action is needed if you didn't intend for the agent to review it. Otherwise, to manually trigger a review, type `/review` in a comment and save.<br/>You can change draft PR review settings [here](https://alpha.bito.ai/home/ai-agents/code-review-agent), or contact your Bito workspace admin at [evan@preset.io](mailto:evan@preset.io).
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "betodealmeida (via GitHub)" <gi...@apache.org>.
betodealmeida commented on PR #35573:
URL: https://github.com/apache/superset/pull/35573#issuecomment-3382515103
/korbit-review
/review
--
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org
Re: [PR] fix: prevent cache key mismatch [superset]
Posted by "eschutho (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@superset.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org