MartinHaeusler / chronos

The Chronos versioning project aims to provide easy-to-use and reliable versioned data storage.
52 stars 7 forks source link

Delete as you go #3

Closed broneill closed 7 years ago

broneill commented 7 years ago

Out of curiosity, I wanted to see how Tupl was being used by Chronos. I noticed in a few places that delete actions over a range of records first store the keys into a collection, and then perform a second pass over them. This comment caught my attention too:

// in this case, we have to do a linear scan of both the key-time and the time-key index, remember // the keys to delete, and then delete them afterwards. In Tuple, there is no "delete as you go" mode // of operation for cursors, i.e. the cursor equivalent of "iterator.remove()" is missing.

Tupl Cursors do support this -- you just call store(null) on the current entry. The key remains in the same place until you move it. This more of a Tupl question than anything else. I'm wondering how can the documentation be improved so that this feature isn't overlooked?

MartinHaeusler commented 7 years ago

Hi, thanks for checking out the project. I hope you did not encounter any "oh my god what on earth are they doing here" moments. I am not entirely sure what you mean by

The key remains in the same place until you move it.

As far as I can remember, the entire point of the operation you commented on was to remove the entire key/value pair. Setting only the value to null and keeping the key intact is not what I was looking for. At least, that is how I would interpret store(null).

In general, I always get a bad feeling when an explicit null literal is passed to the public API of a library. Instead of trying to solve this API issue via JavaDoc, my suggestion would be to introduce another method:

/**
* Deletes the key/value pair at the current cursor position. [Needs to specify the cursor
* position after completing the operation].
*/
public void delete() {
    this.store(null);
}

That way, the old API is still valid (in fact the new method merely forwards the call), but users of the new API don't need to pass a null literal anymore. In my opinion, delete() is much more intention-revealing to the reader of the code than store(null).

broneill commented 7 years ago

Thanks, I'll add the delete method. I'll have it return a boolean for more utility, and to be consistent with the pattern of methods in the View interface. Perhaps insert/update/etc can be added later.

MartinHaeusler commented 7 years ago

Alright! I'll keep track of your repo; we will update to the latest TUPL version soon anyways, then I can rewrite this code in ChronoDB to use the new delete API.

broneill commented 7 years ago

Other than bug fixes, all changes are now being made to version 1.4, which breaks compatibility only due to a license change.