-
Notifications
You must be signed in to change notification settings - Fork 1k
Introduce datatable.unique.names policy for duplicate handling in setnames() #4044 #7674
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,6 +35,30 @@ check_duplicate_names = function(x, table_name=deparse(substitute(x))) { | |
| table_name, brackify(duplicate_names), domain=NA) | ||
| } | ||
|
|
||
| process_name_policy = function(names_vec) { | ||
| policy = getOption("datatable.unique.names") | ||
| if (is.null(policy) || policy == "off") return(names_vec) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. no ultra happy with this double option for "off". I would either eliminate |
||
|
|
||
| allowed = c("warn", "error", "rename") | ||
| if (!policy %in% allowed) { | ||
| warningf("Invalid value for 'datatable.unique.names': [%s]. Falling back to 'off'. Allowed values are: 'off', 'warn', 'error', 'rename'.", as.character(policy)) | ||
| return(names_vec) | ||
| } | ||
|
|
||
| if (anyDuplicated(names_vec)) { | ||
| dups = unique(names_vec[duplicated(names_vec)]) | ||
| # Use paste0 to avoid sprintf issues with column names containing '%' | ||
| msg = paste0("Duplicate column names created: ", brackify(dups), ". This may cause ambiguity.") | ||
|
|
||
| switch(policy, | ||
| warn = warningf(msg), | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see comment above from previous review. this will trip with DT = data.table("a%d" = 1, b = 2)
options(datatable.unique.names = "warn")
setnames(DT, "b", "a%d")
# Error in sprintf(gettext(fmt, domain = domain, trim = trim), ...) :
# too few argumentsHence, you need to pass format strings like |
||
| error = stopf(msg), | ||
| rename = return(make.unique(names_vec)) | ||
| ) | ||
| } | ||
| names_vec | ||
| } | ||
|
|
||
| duplicated_values = function(x) { | ||
| # fast anyDuplicated for the typical/non-error case; second duplicated() pass for (usually) error case | ||
| if (!anyDuplicated(x)) return(vector(typeof(x))) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21577,3 +21577,23 @@ close(con) | |
| file.create(f <- tempfile()) | ||
| test(2367.6, fread(file(f)), data.table(), warning="Connection has size 0.") | ||
| unlink(f) | ||
|
|
||
| #4044 | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please dont only write the issue number, but what issue is fixed. this provides information without needing to search for the issue on github. The tests seem ok but an example of |
||
| DT = as.data.table(iris) | ||
| test(2368.1, names(setnames(copy(DT), "Petal.Length", "Sepal.Length")), | ||
| c("Sepal.Length", "Sepal.Width", "Sepal.Length", "Petal.Width", "Species"), | ||
| options = list(datatable.unique.names = "off")) | ||
| test(2368.2, names(setnames(copy(DT), "Petal.Length", "Sepal.Length")), | ||
| c("Sepal.Length", "Sepal.Width", "Sepal.Length", "Petal.Width", "Species"), | ||
| warning = "Duplicate column names created", | ||
| options = list(datatable.unique.names = "warn")) | ||
| test(2368.3, setnames(copy(DT), "Petal.Length", "Sepal.Length"), | ||
| error = "Duplicate column names created", | ||
| options = list(datatable.unique.names = "error")) | ||
| test(2368.4, names(setnames(copy(DT), "Petal.Length", "Sepal.Length")), | ||
| c("Sepal.Length", "Sepal.Width", "Sepal.Length.1", "Petal.Width", "Species"), | ||
| options = list(datatable.unique.names = "rename")) | ||
| test(2368.5, names(setnames(copy(DT), "Petal.Length", "Sepal.Length")), | ||
| c("Sepal.Length", "Sepal.Width", "Sepal.Length", "Petal.Width", "Species"), | ||
| warning = "Invalid value for 'datatable.unique.names'", | ||
| options = list(datatable.unique.names = "invalid_option_name")) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -105,6 +105,9 @@ | |
| \item{\code{datatable.enlist}}{Experimental feature. Default is \code{NULL}. If set to a function | ||
| (e.g., \code{list}), the \code{j} expression can return a \code{list}, which will then | ||
| be "enlisted" into columns in the result.} | ||
| \item{\code{datatable.unique.names}}{A character string, default \code{NULL} (same as \code{"off"}). | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also document the other options, e.g. warn, error etc. |
||
| Controls the behavior when operations (\bold{currently only \code{setnames}}) | ||
| would result in duplicate column names.} | ||
| } | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think we need this part since its superfluous "This addresses long-standing ambiguity issues when duplicate names were created silently"