Re: [PR] feat: Add slice_id to query object [superset]

["MDeadman (via GitHub)" <gi...@apache.org>]

MDeadman commented on PR #25192:
URL: https://github.com/apache/superset/pull/25192#issuecomment-1745753884

   Hi folks! @eschutho @villebro Thank you for your feedback on this thread!
   
   I have been looking into the suggested approach you mentioned with @hwmarkcheng, about pulling the `slice_id` from the `form_data` in `query_context` instead of injecting the `slice_id` into the `query_object`. 
   
   While it makes sense that currently `slice_id` lives only in `query_context`/`form_data` as right now its not a part of a query, with this PR we are in essence changing that, as we now need the slice_id to build that comment for the final query. 
   
   The problem is however, that the places in which the final query string is constructed (`get_query_str` methods), do not have access to the `query_context`, as only the `query_object` is passed into that point. 
   
   This is the reason why we injected the value into the object as opposed to just pulling from `query_context`, as there would be what looks like a lot of functions and calls we would need to update to now pass along that `query_context` (or just `form_data`) everywhere, and that feels like it would be making a big shift away from why I am guessing the distinction between the `query_context` and `query_object` was made in the first place. This approach of injecting means there is a lot less broad changes we need to make for this to happen.
   
   Should we be looking to make those changes and pass them around anyways? The other approach of modifying the SQL_QUERY_MUTATOR to use something like request args seems to brittle...
   
   If you have any further guidance on what approach would be best, we would love to hear your input. Thank you so much!!
   


-- 
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