Improved Error Message for Assigning NULL to List Column Items

[Nj221102]

Description:

This PR improves the error which occurs when attempting to assign NULL to a list column item using set() or :=. which is not supported, The new message guides users to use list(list(NULL)) instead for row-specific assignments, enhancing clarity and preventing common mistakes.

Changes:

• Updated the error message in assign.c to provide a clearer instruction.
• Updated tests to cover the new error message.

New Error Message:

Direct assignment of NULL to a list column item is not allowed. To set a list column item to NULL, use list(list(NULL)) in your query.

Issue Reference:

This PR closes #5526

[github-actions]

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 atime::atime_pkg on the tests: 6 minutes and 43 seconds

[MichaelChirico]

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.

[joshhwuu]

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.

[MichaelChirico]

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]

[joshhwuu]

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.

[MichaelChirico]

We also need to be sure coln < length(dt), but that's roughly the idea. PTAL.

[joshhwuu]

Looks good!!

[MichaelChirico]

Thanks all!