Keep result-metadata of native queries if possible #39184
Conversation
metamben
added
the
backport
| :type} | ||
| :non-nil #{:dataset_query :display :name :visualization_settings :archived | ||
| :enable_embedding :parameters :parameter_mappings :embedding_params | ||
| :result_metadata :collection_preview :verified-result-metadata?})))) |
There was a problem hiding this comment.
In this hunk, only this line contains interesting an change.
|
| ;; remove all the unchanged keys from the map, except for `:id`, so the functions below can do the right thing since | ||
| ;; they were written pre-Toucan 2 and don't know about [[t2/changes]]... | ||
| ;; | ||
| ;; We have to convert this to a plain map rather than a Toucan 2 instance at this point to work around upstream bug | ||
| ;; https://github.com/camsaul/toucan2/issues/145 . | ||
| (-> (into {:id (:id card)} (t2/changes card)) | ||
| (-> (into {:id (:id card)} (t2/changes (dissoc card :verified-result-metadata?))) |
There was a problem hiding this comment.
Having to add and then remove this data seems a bit noisy. How about update-meta on the card instead of adding a new key to the card itself?
There was a problem hiding this comment.
I'm a bit vary of relying on meta being preserved. Just naively putting metadata at the point where I now assoc :verified-result-metadata? is not enough, that metadata doesn't reach the before update hook (i.e., card doesn't have it).
There was a problem hiding this comment.
Looks good from FE perspective 👍
I have a PR based on this one coming: #39201
|
Since this is for an escalation, I'm merging this as is, we can revisit using meta to pass in the flag. |
|
@metamben Did you forget to add a milestone to the issue for this PR? When and where should I add a milestone? |
Part of #37009.
Related FE PR: #39201
The problem this PR is fixing is that for native queries the before update hook of the card doesn't populate
result_metadata, but sets it tonilinstead (to make sure we have no stale metadata, which we consider worse than no metadata at all). When a native question is updated and the result metadata doesn't change, thet2/changesfunction omitsresult_metadataand the above behavior is triggered.To solve the problem, when a question is updated via the API and we are able to determine the result metadata, we set a flag that forces the use of that metadata (which might mean that the field is left untouched, if there's no change).
This PR doesn't fully solve the problem, because we only set the result metadata if the query finishes in 1.5 s. In other cases, the behavior is unchanged, and the FE will get
nilas metadata.