rusackas commented on PR #42474: URL: https://github.com/apache/superset/pull/42474#issuecomment-5185838087
Nice fix, and good job chasing the bot findings down with real regression tests. If you can deal with one more detail though, the comment on `_extract_query_context_columns` says `columns` takes precedence over `groupby` when both are present, matching `QueryObjectFactory`. But `QueryObject._rename_deprecated_fields` actually does the opposite: it overwrites `columns` with `groupby`'s value whenever `groupby` is present and truthy, regardless of whether `columns` was already set. So for a legacy query_context that somehow has both keys, this helper and `get_query_context()` would disagree on which columns win. Is that edge case actually reachable in practice, or should this fall back to the deprecated field when it's present, same as `QueryObject` does? -- 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]
