Feature/metk 133 obstype #98

danovaro · 2025-07-15T14:47:56Z

Description

Contributor Declaration

By opening this pull request, I affirm the following:

All authors agree to the Contributor License Agreement.
The code follows the project's coding standards.
I have performed self-review and added comments where needed.
I have added or updated tests to verify that my changes are effective and functional.
I have run all existing tests and confirmed they pass.

codecov-commenter · 2025-07-15T14:53:02Z

Codecov Report

Attention: Patch coverage is 94.14414% with 13 lines in your changes missing coverage. Please review.

Project coverage is 67.21%. Comparing base (d08a04d) to head (aeac1b2).

Files with missing lines	Patch %	Lines
src/metkit/mars/MarsLanguage.cc	33.33%	4 Missing ⚠️
src/metkit/mars/TypeEnum.cc	95.94%	3 Missing ⚠️
src/metkit/mars/Type.cc	83.33%	2 Missing ⚠️
src/metkit/mars/MarsRequest.cc	0.00%	1 Missing ⚠️
src/metkit/mars/Parameter.cc	0.00%	1 Missing ⚠️
src/metkit/mars/Type.h	50.00%	1 Missing ⚠️
tests/test_filter.cc	80.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #98      +/-   ##
===========================================
+ Coverage    66.89%   67.21%   +0.31%     
===========================================
  Files          123      124       +1     
  Lines         7274     7165     -109     
  Branches       715      721       +6     
===========================================
- Hits          4866     4816      -50     
+ Misses        2408     2349      -59

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

ChrisspyB

Thanks Emanuele.

The amount of return {a} and ASSERT(x.size() == 1) this introduces makes me wonder if it would have been better to treat obstype differently from the rest of the language (assuming it is the only place where this hierarchical structure will exist). It seems to have introduced quite a lot of complexity into the type expansion.

ChrisspyB · 2025-07-15T17:55:59Z

share/metkit/language.yaml

+      - name: all
+        group:
+          - name: [nsd, conventional, non satellite data all]
+            group:
+              - name: [lsd, land surface data]
+                group:
+                  - [1, s, synop land]


I strongly think we should include some comments above obstype explaining why it as this complex hierarchical structure.

ChrisspyB · 2025-07-15T18:07:29Z

src/metkit/mars/Type.cc

-            ASSERT(r.contains("vals"));
-            eckit::Value vv = r["vals"];
+            ASSERT(r.contains("values"));
+            eckit::Value vv = r["values"];


I wish you renamed vals -> values in a separate PR to reduce noise! But that's okay.

ChrisspyB · 2025-07-15T18:11:05Z

src/metkit/mars/Type.cc

@@ -111,7 +111,7 @@ std::unique_ptr<Context> Context::parseContext(eckit::Value c) {
 //----------------------------------------------------------------------------------------------------------------------

 Type::Type(const std::string& name, const eckit::Value& settings) :
-    name_(name), flatten_(true), multiple_(false), duplicates_(true) {
+    name_(name), flatten_(true), multiple_(false), duplicates_(true), deduplicate_(false) {


Do we really need duplicates_ and deduplicate?

Assuming it is never valid to have duplicates_ = false and deduplicate = true, I wonder if we could replace the two bools with one enum, something like

enum class DuplicatePolicy { Error, Ignore, Deduplicate };

And then we check the value of this enum instead of the pair of bools. I personally struggle to parse things like deduplicate_ || !duplicates_

ChrisspyB · 2025-07-15T18:19:18Z

src/metkit/mars/Type.cc

+        if (deduplicate_ || !duplicates_) {
+            for (const auto& v : vv) {
+                if ((deduplicate_ || !duplicates_) && seen.find(v) != seen.end()) {
+                    if (!duplicates_) {


Can we rename duplicates_ to allowDuplicates_? Assuming my understanding is correct and that name is accurate.

(or see my enum class comment above)

ChrisspyB · 2025-07-15T18:53:50Z

src/metkit/mars/TypeAny.h

@@ -32,7 +32,8 @@ class TypeAny : public Type {
 private:  // methods

    void print(std::ostream& out) const override;
-    bool expand(const MarsExpandContext& ctx, std::string& value, const MarsRequest& request = {}) const override;
+    [[nodiscard]] std::vector<std::string> expand(const MarsExpandContext& ctx, const std::string& value,


Any particular reason for using [[nodiscard]] here, but not in some of the other classes (e.g. TypeDate::expand) ?

ChrisspyB · 2025-07-15T19:11:52Z

src/metkit/odb/OdbMetadataDecoder.cc

+        std::vector<std::string> tidyVal = t->tidy(stringVal);
+        if (tidyVal.size() == 1 && stringVal == tidyVal[0])  // if t->tidy had no effect, set the original value
            gather_.setValue(keyword, val);
        else
-            gather_.setValue(keyword, tidyVal);
+            gather_.setValue(keyword, tidyVal[0]);


Is it possible for t->tidy(stringVal) to return an empty vector? (If so, line 90 is an error).
If we only ever use the 0th value in tidyVal, should we be asserting the length of tidyVal is 1? If not, why not (why is the 0th value the preferred value?)?

ChrisspyB · 2025-07-15T19:12:40Z

src/metkit/mars/TypeParam.h


-    virtual bool expand(const MarsExpandContext& ctx, const MarsRequest& request, std::vector<std::string>& values,
-                        bool fail) const;
+    //     bool expand(const MarsExpandContext& ctx, const MarsRequest& request, std::vector<std::string>& values,


curious that the protected function is no longer required

ChrisspyB · 2025-07-15T19:29:38Z

src/metkit/mars/TypeEnum.cc

+                firstName = eckit::StringTools::upper(firstName);
+            }
+            for (size_t i = 1; i < names.size(); ++i) {
+                // handle aliases


Is this supposed to be a todo?

ChrisspyB · 2025-07-15T19:51:01Z

src/metkit/mars/TypeEnum.cc

+        case 1: {
+            if (v[0].empty()) {
+                return {};
+            }
+            StringManyMap::const_iterator k = mapping_.find(eckit::StringTools::lower(v[0]));
+            ASSERT(k != mapping_.end());
+            cache_[val] = (*k).second;
+            return {(*k).second};
+        }
+        default: {
+            cache_[val] = v;
+            return v;
+        }
    }


So if there's a single value, we return a canonical version, and if there are multiple values we return them all?

src/metkit/mars/TypeEnum.cc

mcakircali · 2025-07-16T09:30:52Z

src/metkit/mars/MarsLanguage.cc

+            names.insert({*j});
        }
        else {
            names.insert((*k).second);


Suggested change

names.insert({*j});

}

else {

names.insert((*k).second);

for (const auto& value : best) {

if (const auto iter = aliases.find(value); iter != aliases.end()) {

names.insert(iter->second);

}

else {

names.insert({value});

mcakircali · 2025-07-16T11:46:48Z

src/metkit/mars/Type.cc

-                   [this, ctx, request](const std::string& s) { return this->tidy(ctx, s, request); });
-
+    for (auto& v : values) {
+        auto expanded = expand(ctx, v, request);


Suggested change

auto expanded = expand(ctx, v, request);

for (const auto& value : values) {

auto expanded = expand(ctx, value, request);

mcakircali · 2025-07-16T14:34:06Z

src/metkit/mars/TypeDate.cc

                }
            }
            else {
                if (tokens.size() == 1 &&
                    (!std::isdigit(value[0]) || value.size() <= 2)) {  // month (i.e. TypeClimateMonthly)
-                    value = month(tokens[0]);
+                    return {month(tokens[0])};
                }
                else {
                    eckit::Date date(value);
-                    value = l2s(date.yyyymmdd());
+                    return {l2s(date.yyyymmdd())};
                }
            }
        }


std::isdigit is in #include <cctype>

remove else after return

Suggested change

}

}

else {

if (tokens.size() == 1 &&

(!std::isdigit(value[0]) || value.size() <= 2)) { // month (i.e. TypeClimateMonthly)

value = month(tokens[0]);

return {month(tokens[0])};

}

else {

eckit::Date date(value);

value = l2s(date.yyyymmdd());

return {l2s(date.yyyymmdd())};

}

}

}

// month-day (i.e. TypeClimateDaily)

std::string m = month(tokens[0]);

long d = s2l(tokens[1]);

return {m + "-" + l2s(d)};

}

if (tokens.size() == 1 &&

(!std::isdigit(value[0]) || value.size() <= 2)) { // month (i.e. TypeClimateMonthly)

return {month(tokens[0])};

}

eckit::Date date(value);

return {l2s(date.yyyymmdd())};

}

tbkr · 2025-07-17T08:12:18Z

src/metkit/mars/TypeEnum.cc

+        for (size_t j = 0; j < val.size(); ++j) {
+            std::string VV = val[j];
+            std::string v  = eckit::StringTools::lower(VV);
+            //                LOG_DEBUG_LIB(LibMetkit) << "v[" << j << "] : " << v << std::endl;


Remove or log?

tbkr · 2025-07-17T08:18:21Z

src/metkit/mars/MarsLanguage.cc

-std::string MarsLanguage::bestMatch(const MarsExpandContext& ctx, const std::string& name,
-                                    const std::vector<std::string>& values, bool fail, bool quiet, bool fullMatch,
-                                    const std::map<std::string, std::string>& aliases) {
+std::vector<std::string> MarsLanguage::bestMatch(const MarsExpandContext& ctx, const std::string& name,


Is it possible to remove parts of the functionality of this function. If I remember correctly, the fuzzy matching for names shouldn't be used anymore.

We are calling this function with fullmatch=true in almost all cases (besides in TypeParam). The amount of boolean param make it really hard to decipher what is going on in each scenario.

tbkr · 2025-07-17T08:25:18Z

src/metkit/mars/TypeParam.cc

-
-bool TypeParam::expand(const MarsExpandContext& ctx, const MarsRequest& request, std::vector<std::string>& values,
-                       bool fail) const {
+void TypeParam::pass2(const MarsExpandContext& ctx, MarsRequest& request) {


The merge of expand with pass2 combines two different steps now.

Is this what we want to have here? Also a comment would be very nice, what we want to achieve in the second pass.

pass2 is just to generic to derive any functional meaning from the name alone.

tbkr · 2025-07-18T14:20:55Z

src/metkit/mars/TypeEnum.cc

+
+    auto it = find(value);
+    if (it != values_.end()) {
+        return groups_.at(it->second).second;
    }


This function may misses a return value if it == values_.end()

tbkr · 2025-07-18T14:24:38Z

src/metkit/mars/MarsLanguage.cc

@@ -38,10 +38,10 @@
 static pthread_once_t once = PTHREAD_ONCE_INIT;
 static eckit::Value languages_;
 static std::set<std::string> verbs_;
-static eckit::StringDict verbAliases_;
+static std::map<std::string, std::string> verbAliases_;


Some of the includes are not used.

tbkr · 2025-07-18T14:25:00Z

src/metkit/mars/TypeEnum.cc

@@ -18,171 +18,137 @@
 #include "metkit/mars/TypesFactory.h"


Some of the includes are not used directly.

Same for src/metkit/mars/TypeParam.cc

tbkr · 2025-07-18T14:29:20Z

src/metkit/mars/TypeMixed.cc

-            auto tmp = (*it).second->expand(ctx, value, request);
-            if (!tmp.empty()) {
-                return tmp;
+            std::string tmp = value;


Suggested change

std::string tmp = value;

tbkr · 2025-07-18T14:29:33Z

src/metkit/mars/TypeMixed.cc

-            if (!tmp.empty()) {
-                return tmp;
+            std::string tmp = value;
+            if ((*it).second->expand(ctx, tmp, request)) {


Suggested change

if ((*it).second->expand(ctx, tmp, request)) {

if ((*it).second->expand(ctx, value, request)) {

@tbkr, the code modifies value only if expand() returns true

tbkr · 2025-07-18T15:05:02Z

src/metkit/mars/TypeParam.cc


-    const Rule* rule = 0;
+    bool fail                       = true;


Make this const. This isn't modified. I'd wish to remove it from the bestMatch function completely. Would be great to have the bestMatch function to always throw an error and have a wrapping function which makes it secure to remove the boolean parameters.

The other two boolean parameters can be deleted similarly: one is just for logging (set the logging level correctly and the other one is only modifying the score in the first line.

ChrisspyB

Definitely better, imo

ChrisspyB · 2025-07-18T16:01:45Z

src/metkit/mars/MarsLanguage.h

@@ -79,9 +77,9 @@ class MarsLanguage : private eckit::NonCopyable {
    std::vector<std::pair<std::string, Type*>> typesByAxisOrder_;
    std::vector<std::string> keywords_;

-    StringMap aliases_;
+    std::map<std::string, std::string> aliases_;


Would be good to consistently use StringDict or std::map<std::string, std::string>

ChrisspyB · 2025-07-18T16:11:31Z

src/metkit/mars/TypeEnum.h

@@ -15,6 +15,8 @@

 #pragma once

+#include <mutex>


Is mutex used?

ah yes, for std::once_flag

ChrisspyB · 2025-07-18T16:16:26Z

tests/test_expand.cc

@@ -1040,11 +1041,13 @@ CASE("test_metkit_expand_MARSC-219") {
    const char* text =
        "retrieve,class=od,date=20231205,expver=0001,obstype=gpsro,stream=lwda,time=18,type=ai,target=\"reference."
        "E2RRc8.data\"";
-    /// @todo OBSTYPE, OBSGROUP
+    /// @todo OBSTYPE


Does test_obstype cover this or no?

mcakircali · 2025-07-18T15:25:17Z

src/metkit/mars/MarsExpandContext.h

+#pragma once

 #include "eckit/memory/NonCopyable.h"

 #include <iosfwd>


as you touch, we can remove unused include

Suggested change

#pragma once

#include "eckit/memory/NonCopyable.h"

#include <iosfwd>

#pragma once

#include <iosfwd>

mcakircali · 2025-07-18T15:29:18Z

src/metkit/mars/MarsLanguage.cc

-    bool expand(const MarsExpandContext& ctx, std::string& value, const MarsRequest& /* request */) const override {
-        return true;
-    }
+    bool expand(const MarsExpandContext&, std::string& value, const MarsRequest&) const override { return true; }


while at it, remove value or better:

Suggested change

bool expand(const MarsExpandContext&, std::string& value, const MarsRequest&) const override { return true; }

bool expand(const MarsExpandContext& /*ctx*/, std::string& /*value*/,

const MarsRequest& /*request*/) const override {

return true;

mcakircali · 2025-07-18T15:49:05Z

src/metkit/mars/Type.h

@@ -221,6 +219,11 @@ class Type : public eckit::Counted {

    virtual size_t count(const std::vector<std::string>& values) const;

+protected:  // members


members -> methods

mcakircali · 2025-07-18T15:51:08Z

src/metkit/mars/Type.h

+    virtual bool expand(const MarsExpandContext& ctx, std::string& value, const MarsRequest& request = {}) const;
    virtual void expand(const MarsExpandContext& ctx, std::vector<std::string>& values,
                        const MarsRequest& request = {}) const;
-    virtual bool expand(const MarsExpandContext& ctx, std::string& value, const MarsRequest& request = {}) const;



not this PR but I'm not in favor of defaulted virtuals
one could do type.expand(ctx, value, {}); at call site

mcakircali · 2025-07-18T15:53:36Z

src/metkit/mars/TypeAny.cc

@@ -24,8 +24,7 @@ void TypeAny::print(std::ostream& out) const {
    out << "TypeAny[name=" << name_ << "]";
 }

-bool TypeAny::expand(const MarsExpandContext& /* ctx */, std::string& /* value */,
-                     const MarsRequest& /* request */) const {
+bool TypeAny::expand(const MarsExpandContext&, std::string& value, const MarsRequest&) const {


obviously messed during merge or some git ninja

mcakircali · 2025-07-18T16:02:31Z

src/metkit/mars/TypeEnum.cc

-    bool uppercase      = false;
-    if (settings.contains("uppercase")) {
-        uppercase = settings["uppercase"];
+void TypeEnum::addValue(const std::string& vv, uint16_t idx, bool allowDuplicates) const {


signature is const std::string& value, uint16_t idx, bool allowDuplicates but here becomes const std::string& vv ...
I'd do std::string value, uint16_t idx, bool allowDuplicates and value = eckit::StringTools::lower(value)

mcakircali · 2025-07-18T16:05:17Z

src/metkit/mars/TypeEnum.cc

-            values_.push_back(v);
+        return groups_.at(idx).second;
+    }
+    else {


no need for else here

src/metkit/mars/TypeEnum.cc

mcakircali · 2025-07-18T16:23:13Z

src/metkit/mars/TypeMixed.cc

-            if (!tmp.empty()) {
-                return tmp;
+            std::string tmp = value;
+            if ((*it).second->expand(ctx, tmp, request)) {


@tbkr, the code modifies value only if expand() returns true

mcakircali · 2025-07-18T16:25:19Z

src/metkit/mars/TypeParam.cc

@@ -221,7 +221,7 @@ Rule::Rule(const eckit::Value& matchers, const eckit::Value& values, const eckit
                precedence[v] = j;
            }

-            mapping_[v] = first;
+            mapping_[v] = {first};


Suggested change

mapping_[v] = {first};

mapping_[v] = first;

danovaro added 6 commits June 23, 2025 10:47

wip

1c79a5a

Merge branch 'develop' into feature/METK-133-obstype

bec893d

wip

cce8eff

Merge branch 'develop' into feature/METK-133-obstype

679f321

METK-133 obstype + expand to multiple values

fd788be

METK-133 obstype tests

61096e1

danovaro requested review from mcakircali and simondsmart July 15, 2025 14:47

format

4e014a1

ChrisspyB reviewed Jul 15, 2025

View reviewed changes

Merge branch 'develop' into feature/METK-133-obstype

6068551

mcakircali reviewed Jul 16, 2025

View reviewed changes

tbkr requested changes Jul 17, 2025

View reviewed changes

danovaro added 8 commits July 17, 2025 19:21

METK-133 simplified TypeEnum group handling

359f7a2

METK-133 simplified TypeEnum group handling

b99b907

format

22437aa

Merge branch 'develop' into feature/METK-133-obstype

b07af27

cleanup

a544db2

METK-133 obstype lazy loading

93e582b

format

41ad5ba

compatibility

aeac1b2

tbkr requested changes Jul 18, 2025

View reviewed changes

ChrisspyB reviewed Jul 18, 2025

View reviewed changes

mcakircali reviewed Jul 18, 2025

View reviewed changes

-            names.insert({*j});
-        }
-        else {
-            names.insert((*k).second);
+    for (const auto& value : best) {
+        if (const auto iter = aliases.find(value); iter != aliases.end()) {
+            names.insert(iter->second);
+        }
+        else {
+            names.insert({value});

	auto expanded = expand(ctx, v, request);
	for (const auto& value : values) {
	auto expanded = expand(ctx, value, request);

	if ((*it).second->expand(ctx, tmp, request)) {
	if ((*it).second->expand(ctx, value, request)) {

		@@ -221,6 +219,11 @@ class Type : public eckit::Counted {

		virtual size_t count(const std::vector<std::string>& values) const;

		protected: // members

Feature/metk 133 obstype #98

Are you sure you want to change the base?

Feature/metk 133 obstype #98

Conversation

danovaro commented Jul 15, 2025

Description

Contributor Declaration

Uh oh!

codecov-commenter commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ChrisspyB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisspyB left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

codecov-commenter commented Jul 15, 2025 •

edited

Loading