FDB-303 added decoupling between Key and fully typed 'CanonicalKey' #26

danovaro · 2024-06-17T12:05:09Z

No description provided.

codecov-commenter · 2024-06-17T12:13:19Z

Codecov Report

Attention: Patch coverage is 72.89377% with 148 lines in your changes missing coverage. Please review.

Project coverage is 64.12%. Comparing base (7f8e64c) to head (3bbb824).

Files with missing lines	Patch %	Lines
src/fdb5/database/Key.cc	76.77%	36 Missing ⚠️
src/fdb5/database/Catalogue.h	19.35%	25 Missing ⚠️
src/fdb5/database/Index.cc	52.94%	16 Missing ⚠️
src/fdb5/database/RetrieveVisitor.cc	0.00%	12 Missing ⚠️
src/fdb5/remote/RemoteFieldLocation.cc	0.00%	7 Missing ⚠️
src/fdb5/toc/AdoptVisitor.cc	0.00%	5 Missing ⚠️
src/fdb5/toc/TocStats.h	54.54%	5 Missing ⚠️
src/fdb5/tools/fdb-hide.cc	0.00%	4 Missing ⚠️
src/fdb5/tools/fdb-root.cc	0.00%	4 Missing ⚠️
src/fdb5/toc/TocCatalogueWriter.cc	80.00%	3 Missing ⚠️
... and 26 more

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #26      +/-   ##
===========================================
+ Coverage    63.84%   64.12%   +0.27%     
===========================================
  Files          237      238       +1     
  Lines        13583    13610      +27     
  Branches      1325     1313      -12     
===========================================
+ Hits          8672     8727      +55     
+ Misses        4911     4883      -28

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/fdb5/database/Catalogue.h

simondsmart · 2024-06-17T12:17:27Z

src/fdb5/rules/Schema.cc

-void Schema::expand(const Key &field, WriteVisitor &visitor) const {
-    Key full(registry());
-    std::vector<Key> keys(3);
+void Schema::expand(const CanonicalKey& field, WriteVisitor &visitor) const {


This feels the wrong way around - we are expanding a canonicalised key, and producing a non-canonicalised key?

simondsmart · 2024-06-17T12:18:07Z

src/fdb5/api/FDB.h

@@ -75,7 +75,7 @@ class FDB {
    // warning: not high-perf API - makes sure that all the requested fields are archived and there are no data exceeding the request
    void archive(const metkit::mars::MarsRequest& request, eckit::DataHandle& handle);
    // disclaimer: this is a low-level API. The provided key and the corresponding data are not checked for consistency
-    void archive(const Key& key, const void* data, size_t length);
+    void archive(const CanonicalKey& key, const void* data, size_t length);


I was expecting the interface to use ApiKey?

(Or just "Key" to make it nice and clean)

simondsmart · 2024-06-17T12:20:36Z

src/fdb5/database/Key.h

+public: // methods
+
+    explicit CanonicalKey(const std::shared_ptr<TypesRegistry> reg = nullptr);
+    explicit CanonicalKey(eckit::Stream &, const std::shared_ptr<TypesRegistry> reg = nullptr);


I don't understand (a) why CanonicalKey and ApiKey have the same constructors, (b) why they both accept knowledge of the TypesRegistry.

Ideally neither of them will have the TypesRegistry. We should have an uncanonicalised key, then a component (the schema?) which has the TypesRegistry and produces a canonicalised key.

…alues

tbkr

Left some minor comment, mostly things of which I'm sure I'm just lacking information, as well as some commented code which is probably not necessary anymore.

tbkr · 2024-09-03T13:51:31Z

src/fdb5/api/FDB.cc

@@ -112,7 +112,7 @@ void FDB::archive(const Key& key, const void* data, size_t length) {
        if (stepunit->second.size()>0 && static_cast<char>(tolower(stepunit->second[0])) != 'h') {
            auto step = keyInternal.find("step");
            if (step != keyInternal.end()) {
-                std::string canonicalStep = config().schema().registry()->lookupType("step").toKey("step", step->second + static_cast<char>(tolower(stepunit->second[0])));


the toKey method now is the same as the tidy method. We should get rid of one

tbkr · 2024-09-03T13:52:26Z

src/fdb5/types/Type.cc

@@ -35,13 +35,11 @@ const std::string &Type::type() const {
    return type_;
 }

-std::string Type::tidy(const std::string&,
-                       const std::string &value) const  {
+std::string Type::tidy(const std::string &value) const  {
    return value;


tbkr · 2024-09-04T14:23:19Z

src/fdb5/database/Key.cc

@@ -380,27 +380,15 @@ Key::Key() :
 // }


There are quite a lot of comments in the class. Is the code still needed? If not, remove it.

tbkr · 2024-09-04T14:32:57Z

src/fdb5/toc/TocCatalogueWriter.cc

@@ -62,7 +62,7 @@ bool TocCatalogueWriter::selectIndex(const Key& idxKey) {
            fdb5LustreapiFileCreate(indexPath.localPath(), stripeIndexLustreSettings());
        }

-        indexes_[idxKey] = Index(new TocIndex(idxKey, indexPath, 0, TocIndex::WRITE));


The whole Catalouge is inserted here although only the schema (and maybe the key functionality/ dbKey) is needed. Is there any reason to insert the whole thing?

How are we treating raw pointer? Is this, per convention, interpreted as a transferred ownership? If so, we need a shared_pointer or whatever the convention is here.

tbkr · 2024-09-04T14:34:05Z

src/fdb5/toc/TocHandler.cc

@@ -1035,7 +1035,7 @@ std::vector<Index> TocHandler::loadIndexes(bool sorted,
            s >> offset;
            s >> type;
            LOG_DEBUG(debug, LibFdb5) << "TocRecord TOC_INDEX " << path << " - " << offset << std::endl;
-            indexes.push_back( new TocIndex(s, r->header_.serialisationVersion_, currentDirectory(),


The remark above would probably get rid of this dynamic_cast, as well? Not entirely sure, whether this can be done like suggested.

tbkr · 2024-09-04T14:36:36Z

src/fdb5/types/TypeClimateMonthly.cc

@@ -34,7 +34,11 @@ TypeClimateMonthly::~TypeClimateMonthly() {

 static int month(const std::string &value) {
    if (isdigit(value[0])) {
-        eckit::Date date(value);


Where is this coming from? Could you add a comment highlighting why this differentiation is necessary?

mcakircali · 2024-09-10T08:50:38Z

tests/regressions/CMakeLists.txt

@@ -19,6 +19,7 @@ if (HAVE_FDB_BUILD_TOOLS) # test scripts use the fdb tools
        add_subdirectory(FDB-282)
        add_subdirectory(FDB-291)
        add_subdirectory(FDB-292)
+        add_subdirectory(FDB-303)


there's no FDB-303 directory, forgot to commit ?

ChrisspyB · 2024-09-10T09:01:20Z

src/fdb5/types/TypeClimateMonthly.cc

@@ -48,8 +52,7 @@ static int month(const std::string &value) {
    }


Indentation is weird here (predates this PR)

src/fdb5/database/Key.h

mcakircali · 2024-09-10T09:16:47Z

src/fdb5/database/Key.cc


-    return ret;
-}
+BaseKey::~BaseKey() {}


prefer = default;

mcakircali · 2024-09-10T09:21:45Z

src/fdb5/database/Key.cc

    for (eckit::StringDict::const_iterator i = keys_.begin(); i != keys_.end(); ++i) {
-        s << i->first << (registry ? canonicalise(i->first, i->second) : i->second);
+        s << i->first << canonicalise(i->first, i->second);
    }



readable ?
for (const auto& [keyword, value] : keys_) { s << keyword << canonicalise(keyword, value); }

mcakircali · 2024-09-10T09:27:53Z

src/fdb5/database/Key.cc

-    std::vector<std::string>::const_iterator k = p.begin();
-    std::vector<std::string>::const_iterator kend = p.end();
-    for (; k != kend; ++k) {
+    for (const auto& k : request.params()) {


naming; k => keyword

mcakircali · 2024-09-10T09:28:46Z

src/fdb5/database/Key.cc

+        bool found=false;
+        auto values = request.values(k);
+        std::string can = canonicalise(k, j->second);
+        for (auto it = values.begin(); !found && it != values.end(); it++) {


cbegin() would be better

it++ intentional ?

mcakircali · 2024-09-10T09:32:37Z

src/fdb5/database/Key.cc

 //----------------------------------------------------------------------------------------------------------------------

+Key::Key() :
+    BaseKey() {}


redundant BaseKey() init

mcakircali · 2024-09-10T09:33:16Z

src/fdb5/database/Key.cc

+    BaseKey(keys) {}
+
+Key::Key(eckit::Stream& s) :
+    BaseKey() {


redundant BaseKey() init

mcakircali · 2024-09-10T12:07:27Z

src/fdb5/database/RetrieveVisitor.h

@@ -42,11 +42,11 @@ class RetrieveVisitor : public ReadVisitor {

    // From Visitor

-    virtual bool selectDatabase(const Key &key, const Key &full) override;
+    virtual bool selectDatabase(const Key& dbKey, const TypedKey& fullComputedKey) override;


can remove virtual, also below

mcakircali · 2024-09-10T12:09:29Z

src/fdb5/database/Store.h

@@ -36,7 +36,7 @@ class Store {
    virtual ~Store() {}


prefer = default;

mcakircali · 2024-09-10T12:09:42Z

src/fdb5/database/Store.h

@@ -36,7 +36,7 @@ class Store {
    virtual ~Store() {}

    virtual eckit::DataHandle* retrieve(Field& field) const = 0;
-    virtual std::unique_ptr<FieldLocation> archive(const Key &key, const void *data, eckit::Length length) = 0;
+    virtual std::unique_ptr<FieldLocation> archive(const Key& idxKey, const void *data, eckit::Length length) = 0;


missing include memory

mcakircali · 2024-09-10T12:12:00Z

src/fdb5/message/MessageArchiver.cc

@@ -85,7 +85,7 @@ static std::vector<metkit::mars::MarsRequest> make_filter_requests(const std::st

    if(str.empty()) return std::vector<metkit::mars::MarsRequest>();


could be return {};

mcakircali · 2024-09-10T12:13:18Z

src/fdb5/message/MessageArchiver.cc

@@ -85,7 +85,7 @@ static std::vector<metkit::mars::MarsRequest> make_filter_requests(const std::st



can move make_filter_requests to anonym namespace and remove static

mcakircali · 2024-09-10T12:50:00Z

src/fdb5/rules/Predicate.h

@@ -26,6 +26,7 @@ namespace metkit { class MarsRequest; }

 namespace fdb5 {

+class Key;
 class Key;


this should be TypedKey

mcakircali · 2024-09-10T13:16:57Z

src/fdb5/rules/Rule.cc


-        if ((*cur)->match(k))
-            expand(request, next, depth, keys, full, visitor);
+        if ((*cur)->match(k.canonical()))


attention, do we need k.canonical here, since visitor::values are canonical ?

I assume that match() takes an argument of type BaseKey. We should either:
i. Call canonical() inside the match() function, or
ii. Be incrementally canonicalising such that we don't fully canonicalise k (with lots of work and allocations) for each iteration of the for loop.

tbkr · 2024-09-10T12:50:58Z

src/fdb5/daos/DaosIndex.cc

@@ -34,8 +34,8 @@ namespace fdb5 {

 //----------------------------------------------------------------------------------------------------------------------

-DaosIndex::DaosIndex(const Key& key, const fdb5::DaosName& name) : 
-    IndexBase(key, "daosKeyValue"), 
+DaosIndex::DaosIndex(const Key& key, const Catalogue* catalogue, const fdb5::DaosName& name) : 


What is the policy on handling raw pointers? Is this owner ship transferal? Would a shared pointer be ok, otherwise?

tbkr · 2024-09-10T12:51:22Z

src/fdb5/daos/DaosIndex.h

@@ -27,9 +27,9 @@ class DaosIndex : public IndexBase {
 public: // methods

    /// @note: creates a new index in DAOS, in the container pointed to by 'name'
-    DaosIndex(const Key& key, const fdb5::DaosName& name);
+    DaosIndex(const Key& key, const Catalogue* catalogue, const fdb5::DaosName& name);


See raw pointer comment above.

tbkr · 2024-09-10T12:59:27Z

src/fdb5/database/Index.h

@@ -121,6 +123,9 @@ class IndexBase : public eckit::Counted {

    Indexer   indexer_;

+    const Catalogue* catalogue_;
+    mutable std::shared_ptr<TypesRegistry> registry_;


Also consistency between the raw pointer and shared pointer concept.

tbkr · 2024-09-10T13:00:18Z

src/fdb5/database/Key.cc

-Key::Key(const std::shared_ptr<TypesRegistry> reg, bool canonical) :
-    keys_(),
-    registry_(reg), canonical_(canonical) {}
+// BaseKey::BaseKey(const std::shared_ptr<TypesRegistry> reg) :


Comments can be deleted.

tbkr · 2024-09-10T13:00:46Z

src/fdb5/database/Key.cc

-void Key::encode(eckit::Stream& s) const {
-    const TypesRegistry* registry = canonical_ ? nullptr : &this->registry();
+void BaseKey::encode(eckit::Stream& s) const {
+    // const TypesRegistry* registry = (registry_ ? registry_.get() : nullptr);


tbkr · 2024-09-10T13:13:18Z

src/fdb5/rules/Schema.cc


-    for (std::vector<Rule *>::const_iterator i = rules_.begin(); i != rules_.end(); ++i ) {
+    for (Rule* r : rules_) {
 		// eckit::Log::info() << "Rule " << **i <<  std::endl;


tbkr · 2024-09-10T13:14:23Z

src/fdb5/toc/RootManager.cc

-std::vector<PathName> RootManager::allRoots(const Key& key)
-{
-    eckit::StringSet roots;
+// std::vector<PathName> RootManager::allRoots(const Key& key)


tbkr · 2024-09-10T13:14:33Z

src/fdb5/toc/RootManager.h


    /// Lists the roots that can be visited given a DB key
-    std::vector<eckit::PathName> allRoots(const Key& key);
+    // std::vector<eckit::PathName> allRoots(const Key& key);


Implementation has been removed. So remove this.

tbkr · 2024-09-10T13:15:22Z

src/fdb5/toc/TocCatalogueWriter.cc

@@ -74,20 +74,20 @@ bool TocCatalogueWriter::selectIndex(const Key& key) {

    if (useSubToc()) {

-        if (fullIndexes_.find(key) == fullIndexes_.end()) {
+        if (fullIndexes_.find(idxKey) == fullIndexes_.end()) {

            // TODO TODO TODO .master.index


This seems to be rather important, if it is stated thrice. Or simply delete it.

tbkr · 2024-09-10T13:15:44Z

src/fdb5/toc/TocCatalogueWriter.cc

@@ -151,11 +151,11 @@ void TocCatalogueWriter::reconsolidateIndexesAndTocs() {
            writer_(writer) {}
        ~ConsolidateIndexVisitor() override {}
    private:
-        void visitDatum(const Field& field, const Key& key) override {
+        void visitDatum(const Field& field, const TypedKey& datumKey) override {
            // TODO: Do a sneaky schema.expand() here, prepopulated with the current DB/index/Rule,


Is this todo still valid?

No. Remove it.

mcakircali · 2024-09-10T15:07:55Z

src/fdb5/database/MultiRetrieveVisitor.cc

@@ -129,7 +129,7 @@ void MultiRetrieveVisitor::values(const metkit::mars::MarsRequest &request,
    }

    for(auto l: list) {


avoid copy; for (const auto& l : list) {
naming; l -> value

mcakircali · 2024-09-10T15:15:29Z

src/fdb5/api/local/ListVisitor.h

@@ -21,6 +21,7 @@

 #include "fdb5/database/DB.h"


unused include
(in general, the DB-Catalogue-Store trio has include issues among each other)

mcakircali · 2024-09-10T15:31:13Z

src/fdb5/api/FDB.cc

            auto step = keyInternal.find("step");
            if (step != keyInternal.end()) {
-                std::string canonicalStep = keyInternal.registry().lookupType("step").toKey("step", step->second+stepunit->second);
+                std::string canonicalStep = config().schema().registry()->lookupType("step").toKey(step->second + static_cast<char>(tolower(stepunit->second[0])));
+                keyInternal.set("step", canonicalStep);


elegant to move registry out of the key
can we pin the registry above and reuse it in the loop, such as;

Key keyInternal(key); const auto registry = config().schema().registry();

Which loop are you referring to here? I don't see a loop.

mcakircali · 2024-09-10T15:39:28Z

src/fdb5/api/FDB.cc

+#include "eckit/utils/StringTools.h"

 #include "eckit/system/Plugin.h"
 #include "eckit/system/LibraryManager.h"


unused includes

mcakircali · 2024-09-10T15:56:54Z

src/fdb5/database/EntryVisitMechanism.h

@@ -26,7 +26,6 @@ class Catalogue;
 class Store;
 class FDBToolRequest;
 class Index;
-class Key;



TypedKey ?

mcakircali · 2024-09-10T15:59:13Z

src/fdb5/database/Index.cc

@@ -125,28 +128,38 @@ void IndexBase::encodeLegacy(eckit::Stream& s, const int version) const {
    s << type_;
 }

-void IndexBase::put(const Key &key, const Field &field) {
+void IndexBase::put(const Key& key, const Field &field) {


maybe align both refs to left for consistency

mcakircali · 2024-09-10T18:28:35Z

src/fdb5/toc/AdoptVisitor.h

@@ -33,14 +33,14 @@ class AdoptVisitor : public BaseArchiveVisitor {
 public: // methods

    AdoptVisitor(Archiver &owner,


align all refs to left ?

mcakircali · 2024-09-10T18:29:07Z

src/fdb5/toc/AdoptVisitor.h


-    virtual bool selectDatum(const Key &key, const Key &full) override;
+    virtual bool selectDatum(const TypedKey& datumKey, const TypedKey& fullComputedKey) override;

    virtual void print( std::ostream &out ) const override;



missing (forward) decl -> TypedKey
redundant virtuals

mcakircali · 2024-09-10T18:34:47Z

src/fdb5/toc/AdoptVisitor.cc

@@ -20,23 +20,23 @@ namespace fdb5 {

 //----------------------------------------------------------------------------------------------------------------------

-AdoptVisitor::AdoptVisitor(Archiver &owner, const Key &field, const PathName &path, Offset offset, Length length) :
-    BaseArchiveVisitor(owner, field),
+AdoptVisitor::AdoptVisitor(Archiver &owner, const Key& initialFieldKey, const PathName &path, Offset offset, Length length) :


good practice -> add const to offset and length

mcakircali · 2024-09-10T18:39:23Z

src/fdb5/database/DB.cc

@@ -173,7 +173,7 @@ void DB::reconsolidate() {
    cat->reconsolidate();
 }

-void DB::index(const Key &key, const eckit::PathName &path, eckit::Offset offset, eckit::Length length) {
+void DB::index(const Key& key, const eckit::PathName &path, eckit::Offset offset, eckit::Length length) {


good practice -> const offset and length

mcakircali · 2024-09-10T19:34:32Z

src/fdb5/toc/TocCatalogueReader.cc

+    for (const auto& m : matching_) {
+        const Index& idx(m->first);
+        Key remapKey = m->second;



safer:

for (const auto* match : matching_) { *if (!match) { continue; }* const auto& [idx, remapKey] = *match;

better, matching_ -> replace with std::forward_list<const pair<>&> or use std::vector<std::reference_wrapper<>>

attention, matching_ contains ptr (not ref); guard as in my comment above or ASSERT()

mcakircali · 2024-09-10T19:51:35Z

src/fdb5/toc/TocCatalogueWriter.cc

@@ -151,11 +151,11 @@ void TocCatalogueWriter::reconsolidateIndexesAndTocs() {
            writer_(writer) {}
        ~ConsolidateIndexVisitor() override {}


off topic:
redundant dtor, avoid it! no use, on top it disables move semantics (if any) on inheritance

mcakircali · 2024-09-10T20:09:03Z

src/fdb5/database/EntryVisitMechanism.h

@@ -52,7 +51,7 @@ class EntryVisitor : public eckit::NonCopyable {

 private: // methods

-    virtual void visitDatum(const Field& field, const Key& key) = 0;
+    virtual void visitDatum(const Field& field, const TypedKey& datumKey) = 0;


critical decision to choose TypeKey here..
if all visitDatum implementations need Key (hence each calls canocinal), then leave it as Key and let parent do it ..

No, using TypedKey is correct.
This defers the decision on whether we need to do canonicalisation (which is expensive) to the class which does the using.

src/fdb5/toc/TocCatalogueWriter.cc

mcakircali · 2024-09-10T20:12:34Z

src/fdb5/toc/TocCatalogueWriter.h

+    virtual bool selectIndex(const Key& idxKey) override;
    virtual void deselectIndex() override;



chance to remove virtual

mcakircali · 2024-09-10T20:24:45Z

src/fdb5/database/Index.h

@@ -53,8 +53,8 @@ class IndexBase : public eckit::Counted {

 public: // methods

-    IndexBase(const Key& key, const std::string& type);
-    IndexBase(eckit::Stream& s, const int version);
+    IndexBase(const Key& key, const std::string& type, const Catalogue* catalogue);


not this PR but mismatched ordering

mcakircali · 2024-09-11T06:39:40Z

src/fdb5/toc/TocStats.h

@@ -161,7 +161,7 @@ class TocStatsReportVisitor : public virtual StatsReportVisitor {

    bool visitDatabase(const Catalogue& catalogue, const Store& store) override;
    void visitDatum(const Field& field, const std::string& keyFingerprint) override;
-    void visitDatum(const Field& field, const Key& key) override { NOTIMP; }
+    void visitDatum(const Field& field, const TypedKey& datumKey) override { NOTIMP; }


attention #26 (comment)

mcakircali · 2024-09-11T10:03:13Z

src/fdb5/toc/TocStore.cc

@@ -128,7 +128,7 @@ std::unique_ptr<FieldLocation> TocStore::archive(const Key &key, const void *dat

    ASSERT(len == length);

-    return std::unique_ptr<TocFieldLocation>(new TocFieldLocation(dataPath, position, length, Key(nullptr, true)));
+    return std::unique_ptr<TocFieldLocation>(new TocFieldLocation(dataPath, position, length, Key()));


while at it, fix -> std::make_unique(dataPath, position, length, Key())

You can do. It doesn't really matter (the only case where std::make_unique makes a difference is if you have a constructor to an object which owns multiple dynamically allocated objects, where the constructor of one of those objects throws after the other has been constructed, and you want to ensure there is no memory leak.

mcakircali · 2024-09-11T10:16:41Z

src/fdb5/tools/fdb-hide.cc

    ASSERT(schema.expandFirstLevel(dbrequest.request(), dbkey));

-    std::unique_ptr<Catalogue> db = CatalogueFactory::instance().build(dbkey, conf, true);
+    std::unique_ptr<Catalogue> db = CatalogueFactory::instance().build(dbkey.canonical(), conf, true);


db and dbWrited (below) could share a const auto dbCanonicalKey = dbkey.canonical();

mcakircali · 2024-09-11T10:19:52Z

src/fdb5/tools/fdb-root.cc

@@ -65,16 +65,16 @@ void FdbRoot::execute(const eckit::option::CmdArgs& args) {

            Config conf = config(args);
            const Schema& schema = conf.schema();
-            Key result;
+            TypedKey result{conf.schema().registry()};


dbReader and dbWriter (below) could share a const auto key = result.canonical();

mcakircali · 2024-09-11T14:34:41Z

src/fdb5/database/Key.cc

+    for (BaseKey::const_iterator j = other.begin(); j != other.end(); ++j) {
+        const std::string& keyword = (*j).first;
+        BaseKey::const_iterator k = find(keyword);
+        if (k == keys_.end()) {
+            missing.insert(keyword);


nice catch!
auto ?
keys_.end() -> end()

small thing but keys_.end() -> end()

mcakircali

Very well done. There's opportunity to fix some non-pr code quality issues. Couple of things need attention, but there's a major issue of segfault, which's why I cannot approve it. Please see the attention/critical comments.

mcakircali

there's an object slicing, pls see my comment above.

mcakircali · 2024-09-19T07:40:05Z

src/fdb5/database/BaseArchiveVisitor.h

-protected: // members
-
-    const Key& field_;
+    const Key& initialFieldKey() { return initialFieldKey_; } 



missing const -> const Key& initialFieldKey() const { return initialFieldKey_; }

mcakircali · 2024-09-19T07:53:42Z

src/fdb5/database/Key.cc

+    for (BaseKey::const_iterator j = other.begin(); j != other.end(); ++j) {
+        const std::string& keyword = (*j).first;
+        BaseKey::const_iterator k = find(keyword);
+        if (k == keys_.end()) {
+            missing.insert(keyword);


small thing but keys_.end() -> end()

mcakircali · 2024-09-19T08:26:07Z

src/fdb5/database/FieldLocation.cc

+    const std::string& keyStr = uri.query("remapKey");
    if (!keyStr.empty()) {


simply const std::string, no ref

mcakircali · 2024-09-19T09:15:52Z

src/fdb5/database/Key.cc

+    BaseKey(key), registry_(reg) {}
+


attention, object slicing will cause issues later
we can reduce contention as registry_(std::move(req))

mcakircali · 2024-09-19T09:16:08Z

src/fdb5/database/Key.cc

+    BaseKey(key), registry_(reg) {}
+
+TypedKey::TypedKey(std::shared_ptr<const TypesRegistry> reg) :
+    BaseKey({}), registry_(reg) {}


we can remove BaseKey({}) since we have default ctor
we can reduce contention as registry_(std::move(req))

mcakircali · 2024-09-19T10:16:05Z

src/fdb5/types/TypeInteger.h

    virtual void getValues(const metkit::mars::MarsRequest &request,
                           const std::string &keyword,


could remove virtual

mcakircali · 2024-09-19T10:16:27Z

src/fdb5/types/TypeMonth.h


-    virtual std::string toKey(const std::string& keyword,
-                              const std::string& value) const override;
+    std::string toKey(const std::string& value) const override;

    virtual void getValues(const metkit::mars::MarsRequest &request,


could remove virtual

mcakircali · 2024-09-19T10:16:55Z

src/fdb5/types/TypeParam.h

@@ -28,7 +28,7 @@ class TypeParam : public Type {

    TypeParam(const std::string &name, const std::string &type);

-    virtual ~TypeParam() override;
+    ~TypeParam() override;

    virtual void getValues(const metkit::mars::MarsRequest &request,


more virtual

mcakircali · 2024-09-19T10:17:12Z

src/fdb5/types/TypeStep.h

@@ -28,22 +28,21 @@ class TypeStep : public Type {

    TypeStep(const std::string &name, const std::string &type);

-    virtual ~TypeStep() override;
+    ~TypeStep() override;

    virtual void getValues(const metkit::mars::MarsRequest &request,


can remove virtual

mcakircali · 2024-09-19T10:22:21Z

src/fdb5/toc/TocCatalogueReader.cc

+    for (const auto& m : matching_) {
+        const Index& idx(m->first);
+        Key remapKey = m->second;



attention, matching_ contains ptr (not ref); guard as in my comment above or ASSERT()

simondsmart

Please look in Schema.h/cc at the ownership of the TypesRegistry. It is currently a shared_ptr - this was because the types registry could be accessed by the Key object, and we could not guarantee that the lifetime of Key objects was less than the lifetime of the schema, as they were returned by the API.

I believe that now all items returned over the API are 'clean' in this regard, and do not require extension of the life of the TypesRegistry - so this should be able to belong to the Schema (and be accessed by reference from elsewhere). If we can fix this, it is a strong indication that we have removed a code smell.

Overall I am really happy with this PR. Once the comments have been addressed, I'm happy for Emanuele to merge without further review from me.

simondsmart · 2024-06-27T15:48:37Z

src/fdb5/database/Key.h


    //TODO add unit test for each type
-    std::string canonicalise(const std::string& keyword, const std::string& value) const;
+    virtual std::string canonicalise(const std::string& keyword, const std::string& value) const = 0;


Why do canonicalise() and type() belong to the base class - surely these are functions that can only be called on one of the derived classes?

simondsmart · 2024-09-19T09:57:39Z

src/fdb5/database/Key.h

+
+private: // members
+
+    std::string canonicalise(const std::string& keyword, const std::string& value) const override;


Can we add a comment to effect that this method is used in the comparison function when passed a BaseKey (otherwise it is odd that this method exists)

simondsmart · 2024-09-19T09:58:02Z

src/fdb5/database/Key.h

+private: // members
+
+    std::string canonicalise(const std::string& keyword, const std::string& value) const override;
+    std::string type(const std::string& keyword) const override;


This function should never be called on Key (only ever on TypedKey). Please remove it from here and from the base class.

simondsmart · 2024-09-19T10:08:42Z

src/fdb5/api/FDB.cc

            auto step = keyInternal.find("step");
            if (step != keyInternal.end()) {
-                std::string canonicalStep = keyInternal.registry().lookupType("step").toKey("step", step->second+stepunit->second);
+                std::string canonicalStep = config().schema().registry()->lookupType("step").toKey(step->second + static_cast<char>(tolower(stepunit->second[0])));
+                keyInternal.set("step", canonicalStep);


Which loop are you referring to here? I don't see a loop.

simondsmart · 2024-09-19T10:22:42Z

src/fdb5/database/EntryVisitMechanism.h

@@ -52,7 +51,7 @@ class EntryVisitor : public eckit::NonCopyable {

 private: // methods

-    virtual void visitDatum(const Field& field, const Key& key) = 0;
+    virtual void visitDatum(const Field& field, const TypedKey& datumKey) = 0;


No, using TypedKey is correct.
This defers the decision on whether we need to do canonicalisation (which is expensive) to the class which does the using.

simondsmart · 2024-09-19T10:53:57Z

src/fdb5/toc/TocCatalogueWriter.cc

@@ -151,11 +151,11 @@ void TocCatalogueWriter::reconsolidateIndexesAndTocs() {
            writer_(writer) {}
        ~ConsolidateIndexVisitor() override {}
    private:
-        void visitDatum(const Field& field, const Key& key) override {
+        void visitDatum(const Field& field, const TypedKey& datumKey) override {
            // TODO: Do a sneaky schema.expand() here, prepopulated with the current DB/index/Rule,


No. Remove it.

simondsmart · 2024-09-19T11:01:11Z

src/fdb5/toc/TocHandler.cc

@@ -1035,7 +1035,7 @@ std::vector<Index> TocHandler::loadIndexes(bool sorted,
            s >> offset;
            s >> type;
            LOG_DEBUG(debug, LibFdb5) << "TocRecord TOC_INDEX " << path << " - " << offset << std::endl;
-            indexes.push_back( new TocIndex(s, r->header_.serialisationVersion_, currentDirectory(),
+            indexes.push_back( new TocIndex(s, *(dynamic_cast<const TocCatalogue*>(this)), r->header_.serialisationVersion_, currentDirectory(),


I don't like this at all.

The purpose of dynamic_cast is that it uses RTTI to check that the cast is valid - and returns null if not. Here we are assuming that the return value is non-null without checking - either negating the point (which is that it should be used where we cannot assume this), or creating a possible segfault location.

One alternative would be to use dynamic_cast<const TocCatalogue&> which would throw an exception in the case that this was wrong.

Another would be to use a static_cast. Or (at the top of the function) we could ASSERT that the dynamic cast is non-null, and then use the returned value elsewhere in the function.

But this is telling us that something is not right with the type hierarchy. Either this function ought to move to TocCatalogue, or it ought to accept an argument of type "const Catalogue&". Probably the latter.

This code is smelly.

simondsmart · 2024-09-19T11:03:44Z

src/fdb5/toc/TocStore.cc

@@ -128,7 +128,7 @@ std::unique_ptr<FieldLocation> TocStore::archive(const Key &key, const void *dat

    ASSERT(len == length);

-    return std::unique_ptr<TocFieldLocation>(new TocFieldLocation(dataPath, position, length, Key(nullptr, true)));
+    return std::unique_ptr<TocFieldLocation>(new TocFieldLocation(dataPath, position, length, Key()));


You can do. It doesn't really matter (the only case where std::make_unique makes a difference is if you have a constructor to an object which owns multiple dynamically allocated objects, where the constructor of one of those objects throws after the other has been constructed, and you want to ensure there is no memory leak.

simondsmart · 2024-09-19T11:06:44Z

src/fdb5/types/TypeClimateMonthly.cc

@@ -34,7 +34,11 @@ TypeClimateMonthly::~TypeClimateMonthly() {

 static int month(const std::string &value) {
    if (isdigit(value[0])) {
-        eckit::Date date(value);
+        int n = stoi(value);


Can we check that the string length <= 2 here. Otherwise we will be doing a lot of extra string work in most cases.

simondsmart · 2024-09-19T11:10:07Z

tests/regressions/FDB-276/FDB-276.sh.in

-set +e
-grep "Invalid expver value xxxx" out.1 # should fail
-set -e
+! grep "Invalid expver value xxxx" out.1 # should fail


Nice trick. I didn't know this one.

related to PR #26

FDB-303 added decoupling between CanonicalKey and fully typed 'ApiKey'

d0be656

danovaro requested review from simondsmart and nicolau-manubens June 17, 2024 12:05

simondsmart requested changes Jun 17, 2024

View reviewed changes

danovaro added 6 commits June 21, 2024 12:15

refactored to address PR comments

6eb2f84

refactored to address PR comments

cb08170

cleanup

63309c1

refactored to address PR comments

12042d0

cleanup

9ecac11

FDB-303 fix Key changes & daos + fdb-list axis check with canonical v…

c1d46e0

…alues

danovaro changed the title ~~FDB-303 added decoupling between CanonicalKey and fully typed 'ApiKey'~~ FDB-303 added decoupling between Key and fully typed 'TypedKey' Jun 25, 2024

danovaro added 6 commits June 28, 2024 19:23

removed keyword from Type::toKey

77fadb3

Merge branch 'develop' into feature/canonicalKey

0eee718

fix merge

f41e792

fix regression test

3e41e96

Merge branch 'develop' into feature/canonicalKey

afded53

Merge branch 'develop' into feature/canonicalKey

fd7ab56

tbkr reviewed Sep 4, 2024

View reviewed changes