Make all modification methods respect prune_previous_versions #1556
Conversation
phoebusm
requested review from
alexowens90,
willdealtry and
poodlewars
as code owners
| @@ -194,7 +194,7 @@ TEST_F(VersionStoreTest, SortMerge) { | |||
| test_store_->append_incomplete_frame(symbol, std::move(frame.input_frame_)); | |||
| } | |||
|
|
|||
| test_store_->sort_merge_internal(symbol, std::nullopt, true, false, false, false); | |||
| test_store_->sort_merge_internal(symbol, std::nullopt, true, false, false, false, false); | |||
There was a problem hiding this comment.
5 bools in a row isn't a good API (nor was 4 before your change!). Refactor by pulling out a struct for the options?
| @@ -81,7 +81,8 @@ class LocalVersionedEngine : public VersionedEngine { | |||
| VersionedItem delete_range_internal( | |||
| const StreamId& stream_id, | |||
| const UpdateQuery& query, | |||
| bool dynamic_schema) override; | |||
| bool dynamic_schema, | |||
| bool prune_previous_versions) override; | |||
There was a problem hiding this comment.
Too many bools - pull out a struct for the options?
| @@ -238,6 +238,26 @@ def arctic_library(arctic_client, lib_name): | |||
| return arctic_client.create_library(lib_name) | |||
|
|
|||
|
|
|||
| @pytest.fixture( | |||
| scope="function", | |||
| params=[ | |||
There was a problem hiding this comment.
Why is this parameterized? Isn't just LMDB fine? Isn't there a similar fixture already?
There was a problem hiding this comment.
I was trying to create a library fixture similar to existing def basic_store()
It is used by test_write_parallel_sort_merge which tests parallelism. So I think it's safer to test multiple storages.
| @@ -540,6 +540,37 @@ def test_delete_date_range_with_strings(version_store_factory, index_start): | |||
| assert_frame_equal(vit.data, df) | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("prune_previous_versions", [True, False]) | |||
There was a problem hiding this comment.
Most of this test is the same as the one above it?
There was a problem hiding this comment.
Yes but I don't want to make def test_delete_date_range_with_strings more complicated.
And I only need to test against lmdb
| @@ -49,6 +49,14 @@ struct KeySizesInfo { | |||
| size_t uncompressed_size; // bytes | |||
| }; | |||
|
|
|||
| struct SortMergeOptiona { | |||
There was a problem hiding this comment.
typo - should be SortMergeOptions
phoebusm
force-pushed
the
feature/add_prune_previous
branch
from
39b402b
to
c4f0e4f
Compare
Reference Issues/PRs
#711
What does this implement or fix?
Add prune_previous option to APIs which still don't support it
Any other comments?
Checklist
Checklist for code changes...