-
Notifications
You must be signed in to change notification settings - Fork 967
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
Teach forder to re-use existing key and index attributes instead of sorting from scratch #4386
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #4386 +/- ##
==========================================
+ Coverage 97.47% 97.50% +0.02%
==========================================
Files 80 80
Lines 14873 15030 +157
==========================================
+ Hits 14498 14655 +157
Misses 375 375 ☔ View full report in Codecov by Sentry. |
forder C set index directly smart opt for index retGrp=T/F no tests updated yet
if (length(o)) { | ||
setattr(x,"index",NULL) # TO DO: reorder existing indexes likely faster than rebuilding again. Allow optionally. Simpler for now to clear. Only when order changes. |
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.
there is a related comment next to test 1419.62
# checks also that the prior index a is dropped (because y is keyed with no index)
but we should not drop that index, as long as order does not change then index is still valid, and it can even be useful now, because it can store groups info.
added NEWS entry. |
@jangorecki I was trying to write an atime example, I saw DT = data.table(a = sample(letters, 1e8, TRUE), key='a')
options(datatable.forder.reuse.sorting=TRUE, datatable.verbose=TRUE)
key(DT)
# [1] "a"
invisible(uniqueN(DT))
# forder.c received 100000000 rows and 1 columns
# forderReuseSorting: opt=-1, took 0.407s
setindex(DT, a)
invisible(uniqueN(DT))
# forderReuseSorting: using existing index: __a
# forderReuseSorting: opt=2, took 0.000s Why doesn't the |
Most likely it does not do retGrp=T which is needed by uniqueN. |
Got it. It does leave us in an awkward place where now This is really unfortunate: DT=data.table(a=sample(letters))
setindex(DT, a)
options(datatable.verbose=TRUE)
uniqueN(DT)
# forderReuseSorting: using existing index: __a
# forderReuseSorting: opt=2, took 0.000s
# [1] 26
setkey(DT, a)
# forderReuseSorting: using existing index: __a
# forderReuseSorting: opt=2, took 0.000s
# forder took 0.000 sec
# reorder took 0.002s elapsed (0.003s cpu)
uniqueN(DT)
# forder.c received 26 rows and 1 columns
# forderReuseSorting: opt=-1, took 0.000s
# [1] 26 Can |
It can but then carry performance penalties more broadly that initially planned change. Why not follow up PR? |
Added as a follow-up task: #6320 |
Thanks @jangorecki !!! 🚀 |
Closes #4380
Closes #4387
Partially address #2947 for index, not for keys. I don't think we can do much better there so I would also close that one.
Towards #2879
Resolves #1321 (a test should be added)
Examples below.
Note that output below is missing newly added attributes populated by new option
retStats=TRUE
. I am not adding them below to make output easier to read.retStats=TRUE
will just return more attributes (all comes for free, unlikeretGrp=T
): any-not-ascii, any-not-utf8, any-na, any-inf-nan.setindex[v]
will now compute groups (retGrp=T
) as well, and store it together with index attribute. Note that it only applies tosetindex
interface, any internal index computation will useforder
and will not compute groups unnecessarily (explained in computing index could find groups as well #4387).i
index #4380). Verbose codes: keyopt=1
, indicesopt=2
.forderv
could already set the index. It is disabled by default, can be enabled by optiondatatable.forder.auto.index=TRUE
. This option is only for debugging/development, not going to be exported. Ultimately currentdatatable.auto.index
option will control that.It is disabled by default because of the issue caused by #507.
forderv
writes an attribute by reference, when invoked for example viauniqueN
by group, it writes the index attribute to the first group.SD
, the same index attribute is later re-used for further groups, making results incorrect:If we will manage to detect that a call has been invoked from
j
+by group, then we can setlazy=FALSE
to escape all optimizations. At the same time we could setDTthreads to 1 in such case #4294.