Abdulrehman-PIAIC80387 commented on code in PR #42463:
URL: https://github.com/apache/superset/pull/42463#discussion_r3663618158
##########
superset/commands/dataset/refresh.py:
##########
@@ -46,7 +49,23 @@ def __init__(self, model_id: int):
def run(self) -> Model:
self.validate()
assert self._model
- self._model.fetch_metadata()
+ try:
+ self._model.fetch_metadata()
+ except SupersetGenericDBErrorException as ex:
+ # Virtual datasets whose SQL contains Jinja templates (e.g.
+ # ``{% if from_dttm %}``) cannot be parsed at save time
+ # because the templates have no runtime context — sqlglot
+ # then rejects the empty ``{% if %}`` block. The dataset
+ # row itself has already been persisted by
+ # ``UpdateDatasetCommand`` before this refresh runs, so
+ # treat the metadata refresh as best-effort rather than
+ # surfacing an "Invalid SQL" toast for a save the user
+ # already committed. See #38012.
+ logger.warning(
+ "Dataset column refresh skipped for %s: %s",
+ self._model.table_name,
+ ex.message,
+ )
Review Comment:
Good catch — you're right that `get_columns_description` wraps genuine DB
errors (connection / permission / driver failures) in the same
`SupersetGenericDBErrorException` class. Narrowed the softening to only trigger
when `self._model.sql` contains Jinja markers (`{%` or `{{`); non-Jinja SQL
still hard-fails on DB errors. Added a new test
`test_refresh_still_raises_generic_db_error_for_non_jinja_sql` to lock that in.
Pushed in the amended commit.
--
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]
