Fix index printing by adding index info to header (#6806)

[Mukulyadav2004]

Problem:
Currently, when options(datatable.show.indices = TRUE), print.data.table() tries to add index info to toprint. However, toprint may have a different number of columns, leading to the error:

Error in rbind(abbs, toprint) : number of columns of result is not a multiple of vector length (arg 1)

Fix:
Instead of modifying toprint, this PR adds the index information directly to header, ensuring a cleaner and safer display.

Changes:
Extracts index names from the "index" attribute.
Formats them (removes __ and replaces _ with , ).
Creates a "Indices: ..." header string.
Appends this to header.

File changed: print.data.table R

[aitap]

The guide that you and your colleagues are following has some advice that is counter-productive. Where does it come from? Perhaps it should be changed?

Putting the issue number that you would like to fix into the pull request title doesn't help: GitHub ignores it. Instead you need to put the number into the text (i.e. the comment) of the pull request. Then GitHub will link the two together.

Let's try to stick to the code formatting style used by this project: when closing an if block and following it with an else block, put both braces on the same line as the word else:

if (whatever) {
  # some code
} else { # <-- like here
  # some more code
}

It's usually best to minimise the changes you're introducing to a code base. This is both easier to review and has less chance of introducing bugs. Since R is mostly ambivalent about spaces, let's not introduce extra space at the end of some but not all of the lines. On the Files tab of this pull request, you can see "lint-r" complaining about extra spaces. Could you please remove them?

But the main problem is the suggested solution. print(x) shouldn't have to change variables above it in the function call stack. The main issue here is that abbs is constructed from classes1(x) without taking into account that cbind(x[...,], index_dt) may be printed instead of just a subset of x. There must be a simpler way of either (1) extracting the classes from both data.tables or (2) padding abbs with a string for each index.

[aitap]

gsub with _ as a complete subexpression will match _ anywhere, including in the middle of the column name. Why does the code match it here?

[aitap]

There's probably a way to fix the code above so that it doesn't create a separator at the end of the string in the first place.

[aitap]

What is this code intended to do? Where is the header variable used? Use of parent.frame() in this context is almost certainly a mistake because parent.frame() is the environment of the function that calls print(x). Why is the exists() check needed? Why assign()? In R, if doesn't create a new lexical scope (unlike a function call), so your changes to the variables will be retained outside the block.

[Mukulyadav2004]

Thank you, @aitap, for your valuable feedback.
I now understand the issues you pointed out and will try to make the necessary changes accordingly. Initially, I declared header because an error occurred when an index existed, and since the index is metadata, I created header to store it. My intent was to update header dynamically when printing a data.table, ensuring index information is included.
However after you tell that if statements do not create a new lexical scope , I came to know about it. This led to header being modified in the parent environment instead of being managed locally within print.data.table().

[aitap]

Is there a place in the code where the header variable is used after being created by this block?

[Mukulyadav2004]

Hi @aitap
As you mentioned, in the modified data.table, where we add columns from both x and index_dt via cbind, abbs was initially constructed only from x and not index_dt. I have made the necessary changes to address this.

However, after implementing these modifications, three tests from tests.Rraw are failing because the expected output differs from the observed output. Specifically, in these tests, the expected output does not include an extra column for index_dt. The failing tests are as follows:

DT2 <- data.table(a = 1:3, b = 4:6)
setindexv(DT2, c("b","a"))
test(1775.2, print(DT2, print.keys = TRUE),
     output=c("Index: <b__a>", "   a b", "1: 1 4", "2: 2 5", "3: 3 6"))

setindexv(DT2, "b")
test(1775.3, print(DT2, print.keys = TRUE),
     output=c("Indices: <b__a>, <b>", "   a b", "1: 1 4", "2: 2 5", "3: 3 6"))

setkey(DT2, a)
setindexv(DT2, c("b","a"))
test(1775.4, print(DT2, print.keys = TRUE),
     output=c("Key: <a>", "Indices: <b__a>, <b>", "   a b", "1: 1 4", "2: 2 5", "3: 3 6"))

Could you please clarify this.

[aitap]

Hi @Mukulyadav2004 Could you please push your changes to this pull request? It's very hard to diagnose code only knowing that it produces unwanted output but without seeing the changes to the output code. The expected output in tests 1775.2, 1775.3, 1775.4 does not contain the index columns because show.indices= is not set, only print.keys= is.

[aitap]

Thank you for making the improvements! You are approaching a good solution.

[aitap]

Is there a place in the code where the header variable is used after being created by this block?

[aitap]

This check is not very reliable. A user can create a column whose name starts with index:: data.table(`index:foo`='bar'). Moreover, grepl('^static-string') can usually be replaced by startsWith(), which avoids the cost of having to compile a regular expression.

[Mukulyadav2004]

Thank you for your feedback. I have reviewed the code, and currently, the header variable is being assigned within this block. However, I do not see any explicit usage of header after its creation. I believe the header might be printed due to the following code snippet:

if (print.keys) {
    if (!is.null(ky <- key(x)))
    catf("Key: <%s>\n", toString(ky))
    if (!is.null(ixs <- indices(x)))
    cat(sprintf(
      ngettext(length(ixs), "Index: %s\n", "Indices: %s\n"),
      paste0("<", ixs, ">", collapse = ", ")
    ))
  }

However, I am not entirely certain, and I would appreciate any clarification if I have misunderstood

[aitap]

Let me rephrase that. When you wrote the code that introduced the header variable, where did you intend it to be used? I'm asking this question because there is no other use of this variable in R/print.data.table.R, and no uses of variables named header in other *.R files are applicable.

If you are using a machine learning model to write the code or the comments for you, please stop doing that, because it's doing us both a disservice. Relying on machine output deprives you from learning opportunities and prevents you from accumulating skill. (If machine learning models start writing good code by themselves, what use will there be for human programmers like you and I?) Having to review code that superficially looks like it would work but ultimately proves useless wastes maintainer time and morale.

[aitap]

What if an R package provides an S3 class called "index"? There's more than 20000 CRAN packages, plus some more Bioconductor packages, plus a lot of packages only published on GitHub or even not published at all.

[Mukulyadav2004]

To avoid such conflicts, I will ensure that only actual index columns are modified by explicitly tracking them using indices(x). This way, we only modify columns that are confirmed to be indices, rather than relying on a generic "index" class check.

[aitap]

Instead of overwriting classes by re-creating it anew, can you append the index markers to the variable classes created two lines above?

[aitap]

Defensive programming is good, but it's better to add a test to inst/tests/tests.Rraw to verify that printing a data.table with indices does not raise any warnings due to invalid rbind().

[Mukulyadav2004]

Hi @aitap,
I have incorporated all the changes you suggested, but I am still encountering 6 test failures out of 11,760 tests. The failing tests are: 2264.1, 2264.2, 2264.3, 2264.4, 2264.7, and 2264.8, all from inst/tests/tests.Rraw.
The issue seems to be that the observed output does not match the expected output.

Expected Output:

  grp1 grp2 grp3 index1:grp1__grp3 index2:grp3__grp1
1:   77   61   53                 3                 5
2:   80   66   37                 8                 4
3:   27   42    8                 5                 3
4:   66   37    7                 4                 7
5:   38   69    5                 6                 2
6:   72   89   69                 1                10
7:   86   52   16                 2                 1
8:   28   35   62                10                 8
9:   95   82   80                 7                 6
10:   83   64   41                 9                 9

Observed Output:

    grp1 grp2 grp3
 1:   77   61   53
 2:   80   66   37
 3:   27   42    8
 4:   66   37    7
 5:   38   69    5
 6:   72   89   69
 7:   86   52   16
 8:   28   35   62
 9:   95   82   80
10:   83   64   41

All failures appear to be of the same nature—the index columns are missing in the observed output. Could you please provide guidance on what might be causing
this and how best to resolve it?

[aitap]

Instead of suppressing warnings, test that they don't occur. Try capturing the output from a print(DT, show.indices=TRUE) with the warning fixed and call test(2264.9, print(DT, show.indices=TRUE), output = c(...example output goes here...)).

[aitap]

The current patch both removes the code that creates index_dt and then seems to set show.indices to FALSE, which prevents an error due to the index_dt being undefined. Try providing the variables that are needed by the show.indices == TRUE branches in print.data.table and then preserving the show.indices argument being true.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.60%. Comparing base (c0469f4) to head (9b83983).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #6816   +/-   ##
=======================================
  Coverage   98.60%   98.60%           
=======================================
  Files          79       79           
  Lines       14661    14661           
=======================================
  Hits        14457    14457           
  Misses        204      204           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Mukulyadav2004]

Hi @aitap,

I have incorporated all the changes as per your suggestions, and this time, all tests from inst/tests/tests.Rraw have passed successfully. However, the following checks have failed:
atime performance tests / comment (pull_request)
code-quality / lint-r (pull_request)

Could you please advise on how to proceed with these.

[MichaelChirico]

• for lintr, scroll through the Files tab, you will see line annotations telling you what's wrong
• same for codecov. you need to add new test(s) which will execute the highlighted line(s).
• for atime, that's a known issue, you can ignore

[Mukulyadav2004]

Hi @MichaelChirico
Apologies for the interruption. Could you kindly review the test I’ve written. Despite making changes to include printing, it still fails at the same checks. I would greatly appreciate your guidance on how to correct it.

[aitap]

Better, but the changes to unrelated code need to be reverted, and it's better to reuse the classes1 logic instead of reimplementing it.

[aitap]

Here's an idea. The code uses classes1(x) to obtain the class names of the column. It's already mostly the right answer, and it handles things like non-length-1 class vectors. The problem here is that toprint, which is what will get printed, may be produced from not just x, but cbind(x, indices(x)), and so may have a different number of columns.

Instead of manually reimplementing classes1 below, it should be enough to either append <index> to classes (length(indices(x)) times) if show.indices is true, or capture classes1(toprint) before it is formatted.

[aitap]

Instead of using capture.output(...) in tests, put the expected output into the output= argument of the test() function. This will automatically skip the output test when running with translation enabled instead of failing it. You also don't have to manually call print() when using the output= argument; the test() function calls print() for you when you ask it to test the output. There are other useful arguments described in help(test, data.table).

[aitap]

I don't think R will allow making the class vector a list. Also, class() will return the name of the primitive type of the value if the class attribute is set empty or removed altogether, so both tests here are impossible.

[aitap]

This is likely to be impossible due to the way toprint is constructed from x.

[Mukulyadav2004]

Hi @aitap,
Could you please review the changes when you have some time?
To summarize, I’ve made the following modifications:
1: Ensured that toprint correctly inherits from "data.table" and "data.frame" if it does not already.
2: Adjusted how column classes are determined, appending "" when show.indices is enabled and indices are present.
Currently, two checks are failing: atime performance and lint -r. The lint check suggests replacing explicit returns with implicit returns across multiple R files. Would you recommend making these changes as part of this PR, or should they be handled separately?

[aitap]

Almost there. Here's a test:

library(data.table)
DT <- data.table(a = 1:2, b = 2:1)
setindex(DT, b)
data.table:::test(
 0,
 { print(DT); TRUE },
 options = c(datatable.show.indices = TRUE)
)

Currently it fails with "produced 1 warnings but expected 0". In order to get it to pass, you need to produce the correct classes variable. Currently, you add the indices to the column headers twice. Once when you call classes <- classes1(toprint):

debug: classes <- classes1(toprint)
# ...
Browse[1]> print(classes)
[1] "integer" "integer" "integer"

...and for the second time when you append rep("<index>", length(indices(x))):

debug: classes <- c(classes, rep("<index>", length(indices(x))))
# <...>
Browse[1]> print(classes)
[1] "integer" "integer" "integer" "<index>"

Use one technique but not the other.

[aitap]

I think this variable is not needed now.

[aitap]

Could you please explain why this is needed?

[Mukulyadav2004]

Thank you for your review and feedback.
The reason for its need is that, when running the tests without this patch, I encountered the following error:

Running test id 2222         Test 2222 produced 1 errors but expected 0  
Expected:   
Observed: data.table inherits from data.frame (from v1.5), but this data.table does not. Has it been created manually (e.g. by using 'structure' rather than 'data.table') or saved to disk using a prior version of data.table?  
Output captured before unexpected warning/error/message:  
<simpleError in dimnames.data.table(x): data.table inherits from data.frame (from v1.5), but this data.table does not. Has it been created manually (e.g. by using 'structure' rather than 'data.table') or saved to disk using a prior version of data.table?>

To address this, I added a check to ensure that toprint correctly inherits from data.table and, consequently, data.frame. This prevents potential issues where toprint might lose its expected class during processing.

[aitap]

This error is signalled by colnames(toprint). Since the col_names variables isn't used anywhere, it's better to remove both it and the inherits(toprint, ...) check.

[aitap]

Thank you very much for your persistence. The two failing CI checks are currently expected. The test fails on master (checked manually) but passes here.