Refactor value types #93

hendrikmuhs · 2018-08-22T09:46:51Z

Changes the template logic to give a dictionary type enum value instead of inner details
The need for a persistence choice is hidden in a lower layer.
Value Stores are separated by 3 different modes of writing: compile, merge, append merge.
Implement support for append merge of string dictionaries.

fixes #93

old:

todo:

separate value store for append merge and normal merge
fix string value store append merge (actually seems to be broken independent of this PR)

coveralls · 2018-08-22T10:46:28Z

Coverage decreased (-0.09%) to 95.212% when pulling 0fcf738 on hendrikmuhs:refactor-value-types into c1343d9 on KeyviDev:master.

hendrikmuhs · 2018-09-06T19:06:29Z

almost done, got bigger and bigger

would be good to get some 1st feedback on the changes, IMHO it simplifies usage and the code base despite the need of a bit more boiler plate code

I also see some more improvements, e.g. further separating writing and merging (requires some thought on the generator), but I prefer doing this in separate follow up PR's.

narekgharibyan · 2018-09-10T09:25:44Z

keyvi/bin/keyvicompiler/keyvicompiler.cpp

-  keyvi::dictionary::DictionaryCompiler<keyvi::dictionary::fsa::internal::SparseArrayPersistence<BucketT>,
-                                        keyvi::dictionary::fsa::internal::IntInnerWeightsValueStore>
-      compiler(value_store_params);
+  keyvi::dictionary::DictionaryCompiler<keyvi::dictionary::dictionary_type_t::INT_WITH_WEIGHTS> compiler(


Maybe here we can use types defined in dictionary_types.h ?

narekgharibyan · 2018-09-10T09:44:24Z

keyvi/include/keyvi/dictionary/fsa/internal/value_store_persistence.h

+ * Comparison for the string value store which does not use length prefixes
+ */
+template <class PersistenceT, class HashCodeTypeT = int32_t>
+struct RawPointerForCompareString final {


So this will be removed once current StringValueStore is deprecated, right ?

Yes, we have to keep the reader for some time, but the writing part can be removed as part of the reimplementation.

narekgharibyan · 2018-09-10T09:50:56Z

keyvi/include/keyvi/dictionary/fsa/internal/ivalue_store.h

-  JSON_VALUE_STORE_DEPRECATED = 4,    // !< JsonValueStoreDeprecated
-  JSON_VALUE_STORE = 5,               // !< JsonValueStore
-  INT_INNER_WEIGHTS_VALUE_STORE = 6,  // !< IntInnerWeightsValueStore
+  KEY_ONLY = 1,          //!< NullValueStore


Maybe this refactoring is a good time to use scoped enums here ?

Excellent Idea!

narekgharibyan · 2018-09-10T15:18:56Z

keyvi/include/keyvi/dictionary/fsa/internal/int_inner_weights_value_store.h

+class IntInnerWeightsValueStore final : public IntInnerWeightsValueStoreBase {
+ public:
+  explicit IntInnerWeightsValueStore(const keyvi::util::parameters_t& parameters = keyvi::util::parameters_t()) {}
+  IntInnerWeightsValueStore(const IntInnerWeightsValueStore& that) = delete;


If we delete these methods in base class, we don't need to delete them in derived ones.

narekgharibyan · 2018-09-10T15:19:46Z

keyvi/include/keyvi/dictionary/fsa/internal/int_inner_weights_value_store.h

+ */
+class IntInnerWeightsValueStore final : public IntInnerWeightsValueStoreBase {
+ public:
+  explicit IntInnerWeightsValueStore(const keyvi::util::parameters_t& parameters = keyvi::util::parameters_t()) {}


We can implement this constructor in base class, and later just make use of constructor inheritance.

narekgharibyan · 2018-09-10T15:20:32Z

keyvi/include/keyvi/dictionary/fsa/internal/int_inner_weights_value_store.h

+
+class IntInnerWeightsValueStoreAppendMerge final : public IntInnerWeightsValueStoreBase {
+ public:
+  explicit IntInnerWeightsValueStoreAppendMerge(


Is this constructor needed for append merger ?

yes, because the generator constructs ValueStore if it isn't passed into it, similar to the AddValue problem. We should look into this in 1 change.

narekgharibyan · 2018-09-10T15:21:38Z

keyvi/include/keyvi/dictionary/fsa/internal/int_inner_weights_value_store.h

@@ -99,6 +106,13 @@ class IntInnerWeightsValueStoreReader final : public IValueStoreReader {
  std::string GetValueAsString(uint64_t fsa_value) const override { return std::to_string(fsa_value); }
 };

+template <>
+struct Dict<value_store_t::INT_WITH_WEIGHTS> {
+  typedef IntInnerWeightsValueStore value_store_t;


I think modern C++ is encouraging use of using statement over typedef.

narekgharibyan · 2018-09-11T08:02:23Z

keyvi/include/keyvi/dictionary/fsa/generator_adapter.h

-GeneratorAdapterInterface<PersistenceT, ValueStoreT>::CreateGenerator(size_t size_of_keys,
-                                                                      const keyvi::util::parameters_t& params,
-                                                                      ValueStoreT* value_store) {
+typename GeneratorAdapterInterface<ValueT>::AdapterPtr GeneratorAdapterInterface<ValueT>::CreateGenerator(


Seems that we are passing 2 value store related template params, maybe we can eliminate one of them ?

this looks strange but note that ValueT is the template of the class, PersistenceT, ValueStoreT are templates of the method.

narekgharibyan · 2018-09-11T08:07:21Z

keyvi/include/keyvi/dictionary/dictionary_merger.h

-template <class PersistenceT, class ValueStoreT = fsa::internal::NullValueStore>
+template <int DictionaryType = fsa::internal::value_store_t::KEY_ONLY,
+          typename ValueStoreMergeT = typename fsa::internal::Dict<DictionaryType>::value_store_merge_t,
+          typename ValueStoreAppendMergeT = typename fsa::internal::Dict<DictionaryType>::value_store_append_merge_t>


Do we want to have this type deduction as a default behavior which is possible to override, or it should be enforced and be the only one ?

narekgharibyan · 2018-09-11T08:20:41Z

keyvi/include/keyvi/dictionary/fsa/generator.h

@@ -196,7 +196,7 @@ class Generator final {

    // get value and mark final state
    bool no_minimization = false;
-    uint64_t value_idx = value_store_->GetValue(value, &no_minimization);
+    uint64_t value_idx = value_store_->AddValue(value, &no_minimization);


@hendrikmuhs looks like this function is not used anymore, and can be deleted. What do you think ?
(this will also eliminate passing value_store_ to generator).

Note: Probably we can do that in a separate PR.

I agree, this is basically the reason why the value stores for merging still require the constructor and the Add(key, value) method.

Still, I would like to work on that in a separate PR.

narekgharibyan · 2018-09-11T08:23:31Z

keyvi/include/keyvi/dictionary/dictionary_merger.h

-        }
-
-      } else {
+      if (KeyDeleted(segment_it.segmentIndex(), top_key) == false) {


Maybe if ( ! KeyDeleted(...)) ?

narekgharibyan · 2018-09-11T08:26:07Z

keyvi/include/keyvi/dictionary/dictionary_merger.h

+    }
+  }
+
+  void Write(std::ostream& stream) {


I doubt if we need this function for merger.

It's afaik not used at the moment, but that's true for the compiler as well. Not added, but just some strange merge artifact.

hendrikmuhs · 2018-09-12T17:41:48Z

I did a couple of the suggested re-factorings.

Thanks for the review @narekgharibyan! Looks much better now. Would be nice if you can have another look. I also changed vector to use the new template typing system.

Separation is still not perfect, to be continued in #102

narekgharibyan · 2018-09-12T18:09:47Z

keyvi/include/keyvi/dictionary/fsa/internal/int_inner_weights_value_store.h

+/**
+ * An IntValue store which uses the values as inner weights.
+ */
+class IntInnerWeightsValueStore final : public IntInnerWeightsValueStoreBase {


Seems we don't need this class anymore. Maybe it's worth to delete it and just rename IntInnerWeightsValueStoreBase -> IntInnerWeightsValueStore.

With #102 the AddValue(...) methods will move out of the base class into here.

narekgharibyan · 2018-09-12T18:11:09Z

keyvi/include/keyvi/dictionary/fsa/internal/int_value_store.h


-  uint64_t GetValue(const char* p, uint64_t v, bool* no_minimization) { return v; }
+class IntValueStore final : public IntValueStoreBase {


Same as https://github.com/KeyviDev/keyvi/pull/93/files#r217135241

narekgharibyan · 2018-09-12T18:23:30Z

keyvi/include/keyvi/vector/vector_generator.h

@@ -37,10 +37,11 @@
 namespace keyvi {
 namespace vector {

-template <class ValueStoreT>
+template <keyvi::dictionary::fsa::internal::value_store_t DictionaryType>


Truth be told I'm not sure about this change, as this adds a requirement of Dict< ... > to be defined for value store, which theoretically can be missing for some special value store, implemented for keyvi::vector only. No strong feelings though.

sounds more like a naming issue to me: I haven't thought about vector when calling the type Dict<...> - thinking about it, it would make sense to rename. All this does is simplifying the selection of a value store.

Any suggestions for a better name?

Actually I think that this class should have a member for value store reader as well, which will ultimately eliminate a need of having ValueStoreFactory as a separate class/method. Also it will ensure that someone implementing new value store, implements all it's components.

So maybe value_store_components ?

sounds good, I doubt we can get rid of the factory as this is part of runtime abstraction but I think we can do some improvements

narekgharibyan · 2018-09-12T18:30:58Z

@hendrikmuhs This PR is really great, looking forward to have this merged!
Added a few more comments, apart from that LGTM!

And more idea to be discussed later on: In a long run I see weights to be something independent from value store itself, so for example we can have completion dictionary with json values, but this will require addition of new interfaces as well.

hendrikmuhs · 2018-09-13T16:01:27Z

I improved the naming, there were some overlaps, I hope this improves readability. The readers are also now part of the new helper type.

@narekgharibyan Can you create an issue for your idea about weights - not sure if it can really be independent or if we need a JsonValueStoreWithWeights but lets discuss it in the issue.

hendrikmuhs requested a review from narekgharibyan September 6, 2018 19:04

hendrikmuhs mentioned this pull request Sep 6, 2018

Refactor StringValueStore to use external memory to limit memory usage. #69

Closed

hendrikmuhs added the refactoring label Sep 9, 2018

narekgharibyan reviewed Sep 10, 2018

View reviewed changes

narekgharibyan reviewed Sep 11, 2018

View reviewed changes

hendrikmuhs mentioned this pull request Sep 12, 2018

Move out ValueStore from Automata and Generator classes #102

Open

3 tasks

narekgharibyan reviewed Sep 12, 2018

View reviewed changes

narekgharibyan approved these changes Sep 12, 2018

View reviewed changes

hendrikmuhs added 5 commits September 13, 2018 19:23

simplify type deduction

c768992

overhaul type system

9027919

format fixes

61c037b

fix friend class declaration

18ac3ae

fix type in friend forward declaration

f8ef7c4

hendrikmuhs added 17 commits September 13, 2018 19:23

missing friend forward fix

1739fcb

separate value stores for merge and append merge

ddc1c6b

remove IValueStoreWriter

7f073bc

split json value store implementations for writing and merging

1c5ce31

refactor stringvaluestore

dfa1fce

remove commented code

baad015

fix append merge for string value store

2c6b38b

remove dead code

af743c9

remove superfluous includes

fbce82a

use scoped enum for value types

79f404d

change typedef to using

af1e860

simplify template parameters

d90dad3

fix friend definition

d3ded9f

address review comments

e03dfe9

use dictionary compiler short types

5b36810

improve naming

bd53420

adapt test from upstream

0fcf738

hendrikmuhs force-pushed the refactor-value-types branch from 3727b53 to 0fcf738 Compare September 13, 2018 17:28

test appendmerge for key only dicts

0ebbc7f

hendrikmuhs merged commit 20c7895 into KeyviDev:master Sep 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor value types #93

Refactor value types #93

hendrikmuhs commented Aug 22, 2018 •

edited

Loading

coveralls commented Aug 22, 2018 •

edited

Loading

hendrikmuhs commented Sep 6, 2018 •

edited

Loading

narekgharibyan Sep 10, 2018

narekgharibyan Sep 10, 2018

hendrikmuhs Sep 10, 2018

narekgharibyan Sep 10, 2018

hendrikmuhs Sep 10, 2018

narekgharibyan Sep 10, 2018

narekgharibyan Sep 10, 2018

narekgharibyan Sep 10, 2018

hendrikmuhs Sep 11, 2018

narekgharibyan Sep 10, 2018

narekgharibyan Sep 11, 2018

hendrikmuhs Sep 11, 2018

narekgharibyan Sep 11, 2018

narekgharibyan Sep 11, 2018

hendrikmuhs Sep 11, 2018

narekgharibyan Sep 11, 2018 •

edited

Loading

narekgharibyan Sep 11, 2018

hendrikmuhs Sep 11, 2018

hendrikmuhs commented Sep 12, 2018 •

edited

Loading

narekgharibyan Sep 12, 2018

hendrikmuhs Sep 13, 2018

narekgharibyan Sep 12, 2018

narekgharibyan Sep 12, 2018

hendrikmuhs Sep 12, 2018

narekgharibyan Sep 12, 2018

hendrikmuhs Sep 13, 2018

narekgharibyan commented Sep 12, 2018

hendrikmuhs commented Sep 13, 2018


		uint64_t GetValue(const char* p, uint64_t v, bool* no_minimization) { return v; }
		class IntValueStore final : public IntValueStoreBase {

Refactor value types #93

Refactor value types #93

Conversation

hendrikmuhs commented Aug 22, 2018 • edited Loading

coveralls commented Aug 22, 2018 • edited Loading

hendrikmuhs commented Sep 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narekgharibyan Sep 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hendrikmuhs commented Sep 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

narekgharibyan commented Sep 12, 2018

hendrikmuhs commented Sep 13, 2018

hendrikmuhs commented Aug 22, 2018 •

edited

Loading

coveralls commented Aug 22, 2018 •

edited

Loading

hendrikmuhs commented Sep 6, 2018 •

edited

Loading

narekgharibyan Sep 11, 2018 •

edited

Loading

hendrikmuhs commented Sep 12, 2018 •

edited

Loading