Skip to content
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.

Sign up for GitHub

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

Jump to bottom

List size for large data.tables #6609

Merged
merged 7 commits into from
Nov 13, 2024
Merged

List size for large data.tables #6609

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
f08502c
List size for large data.tables
vlulla Nov 9, 2024
48ef7f9
Edits based on guidance from @MichaelChirico
vlulla Nov 9, 2024
2cf5d25
Explicitly list the size as a numeric instead of integer.
vlulla Nov 9, 2024
30b0ce7
Better phrasing of the bug fix.
vlulla Nov 10, 2024
e15ec87
Update inst/tests/other.Rraw
ben-schwen Nov 10, 2024
f8b389b
Merge branch 'master' into tables-NAsize
MichaelChirico Nov 13, 2024
40e4ac3
more specific about 2GiB size for "large"
MichaelChirico Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/tables.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ type_size = function(DT) {
# for speed and ram efficiency, a lower bound by not descending into character string lengths or list items
# if a more accurate and higher estimate is needed then user can pass object.size or alternative to mb=
# in case number of columns is very large (e.g. 1e6 columns) then we use a for() to avoid allocation of sapply()
ans = 0L
ans = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ans = 0
ans = 0.0

we like to really emphasize "this is not integer"

lookup = c("raw"=1L, "integer"=4L, "double"=8L, "complex"=16L)
for (i in seq_along(DT)) {
col = DT[[i]]
Expand Down
7 changes: 7 additions & 0 deletions inst/tests/other.Rraw
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -766,3 +766,10 @@ if (loaded[["nanotime"]]) {
# respect dec=',' for nanotime, related to #6446, corresponding to tests 2281.*
test(31, fwrite(data.table(as.nanotime(.POSIXct(0))), dec=',', sep=';'), output="1970-01-01T00:00:00,000000000Z")
}

# test for bug#6607
ben-schwen marked this conversation as resolved.
Show resolved Hide resolved
local({
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • no need for :::
  • you filter to one NAME, somewhere redundant to then test that value

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you're using local() to reduce the scope of what gets included in tables() right?

if so I'd either

  • emphasize that in a comment (using local is otherwise unusual in our suite)
  • assign to a specific new.env() for clarity

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good. I'll make these changes.

DT <- as.data.table(lapply(1:15,function(i) runif(20e6)))
res <- tables()
data.table:::test(32, res[NAME=='DT',.(NAME,NROW,NCOL,MB)], data.table(NAME="DT",NROW=20000000L,NCOL=15L,MB=2288))
})
Loading