VanessaGiannoni commented on PR #37140: URL: https://github.com/apache/superset/pull/37140#issuecomment-3811124548
> Just noticed the new `columnDisplayNames` prop. I'm trying to understand the design decision behind this approach, as I'm concerned it might introduce some maintainability challenges. > > Since `useResultsPane` fetches a query directly from the backend, requiring an external `columnDisplayNames` prop creates two separate sources of truth for what is essentially the same data. Without explicit synchronization logic, this seems like it could easily lead to subtle display bugs down the line. > > I'm also concerned about the prop drilling that this introduces, having to pass display labels through multiple layers feels like it adds complexity without clear benefit. Also this is not a required prop which may further introduce inconsistencies depending on usage. > > Would it be cleaner to have the backend include display labels as part of the query response? This would keep the component more self-contained and eliminate the risk of data mismatches entirely. > > I'd appreciate understanding the reasoning behind this approach, as I'm still getting up to speed with this part of the codebase and have just seen the PR merged while trying to resolve conflicts in #37396. > > As a side note, `superset-frontend/src/explore/components/DataTablesPane/DataTablesPane.tsx` is missing the `columnDisplayNames` entirely atm. This already introduces an inconsistency as the tables in different parts of the application show different labels even thought they should show the same data! Thanks for raising these points. I honestly hadn't fully considered. I'm also still getting up to speed with this part of the codebase, and your suggestion to have the backend include display labels in the query response actually makes a lot more sense architecturally. My approach was more of a "minimal change" solution at the time — I needed to get display labels working across different result panes, and adding a prop seemed like the path of least resistance — even tho I agree prop drilling here feels like a smell. But you're spot on that it creates the synchronization risks and complexity you mentioned. The missing `columnDisplayNames` in `DataTablesPane` is definitely an oversight on my part and proves your point about inconsistencies. I think your backend approach is the right long-term solution. It would eliminate the prop drilling, reduce the risk of mismatches, and make the components more self-contained. The query response already has the data structure, so including display metadata there makes perfect sense. cc: @msyavuz, @geido, since you have more knowledge about the codebase, it'd be great to hear your opinions on these concerns. -- 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]
