Closed annie closed 1 year ago
hm looks like CI is failing because String.replaceAll
(which i added in the unit test) isn't available in Node 14. i'll update it to just use .replace
.
oh interesting! in my first pass i had added names
to select
as a new property, but that ended up feeling unwieldy – your suggestion seems cleaner though. i'll take a shot at updating it in the monorepo PR and see if i run into any issues.
@mbostock i ran into one major issue while trying to implement your suggestion, which is that it doesn't allow us to preserve the renamed name when a user hides the column. we could potentially get around this by adding a hidden
flag to each columns
object and toggling that when we hide a column, instead of removing the object from the array, but that would require a fairly significant set of changes and i'm not sure it's worth doing.
given that, i think i'd prefer to stick with this operations.names
approach, unless you have strong objections and/or other ideas!
@annie does that mean we’re still selecting and returning columns that are hidden when the query is run?
no we don't actually select hidden columns, we just keep the name information for that column in operations.names
. that allows us to still show the renamed name in the columns dropdown (where hidden columns are listed in a deselected state), and the column will preserve its renamed name if it's reselected.
@annie Got it. Sorry, still waking up! ☕ 👍
No further suggestions from me. I’ll leave the reviewing to @libbey-observable and @mkfreeman! Thanks y’all. 🙏
Implement column renaming in stdlib. Specifically, we now set column name overrides based on the
names
property innode.data.operations
, which is added in this PR.For SQL data sources, we do this by modifying the
SELECT
clause. For in-memory data sources, we create a copy of the source data and modify it to use the new names.