-
Notifications
You must be signed in to change notification settings - Fork 978
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
Improved Error Message for Assigning NULL to List Column Items #6336
Conversation
Generated via commit c5947a0 Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 11 minutes and 26 seconds Time taken to run |
src/assign.c
Outdated
@@ -458,7 +458,7 @@ SEXP assign(SEXP dt, SEXP rows, SEXP cols, SEXP newcolnames, SEXP values) | |||
coln--; | |||
SEXP thisvalue = RHS_list_of_columns ? VECTOR_ELT(values, i) : values; | |||
vlen = length(thisvalue); | |||
if (isNull(thisvalue) && !isNull(rows)) error(_("When deleting columns, i should not be provided")); // #1082, #3089 | |||
if (isNull(thisvalue) && !isNull(rows)) error(_("Direct assignment of NULL when i is provided is not supported. If you intend to assign NULL to a column, use NULL without i. For list columns, if you intend to assign NULL to a specific item, use list(list(NULL)) in your query.")); // #1082, #3089 |
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.
Not sure I'm a fan of the wording chosen here:
Direct assignment of NULL when i is provided is not supported.
Well, it's never supported to assign NULL
to anything but a list column -- regardless of whether i
is provided.
If you intend to assign NULL to a column, use NULL without i
Is that right? Assigning NULL
without i
just deletes the column? And again, this only applies to list
columns.
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.
Would it be ideal if we errored differently depending on whether LHS is a list column or not?
Something like:
DT = data.table(A=1:3, B=list())
DT[2, A:=NULL] # ERROR: When deleting columns, i should not be provided
DT[2, B:=NULL} # ERROR: When deleting columns, i should not be provided. If you are trying to assign NULL to a list column, please use list(list(NULL)) in your query instead.
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.
yes that's exactly what I'm thinking. I ran out of time to check earlier -- IINM at that point in the code we haven't checked if the current column exists or not yet.
e.g. in your example
DT[2, C := NULL]
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.
WDYT about:
if (isNull(thisvalue) && !isNull(rows)) {
if (TYPEOF(VECTOR_ELT(dt, coln)) == VECSXP) {
error(_("When deleting columns, i should not be provided. If you are trying to assign NULL to a list column, please use list(list(NULL)) instead."));
} else {
error(_("When deleting columns, i should not be provided."));
}
}
For the case where the column doesn't exist, I believe the error is the same as current master, ERROR: When deleting columns, i should not be provided
.
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.
We also need to be sure coln < length(dt)
, but that's roughly the idea. PTAL.
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.
Looks good!!
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.
Thanks all!
Description:
This PR improves the error which occurs when attempting to assign
NULL
to a list column item usingset()
or:=
. which is not supported, The new message guides users to uselist(list(NULL))
instead for row-specific assignments, enhancing clarity and preventing common mistakes.Changes:
assign.c
to provide a clearer instruction.New Error Message:
Issue Reference:
This PR closes #5526