schema - optional with defaults - improved behaviour (not injecting the default in the index)

[danovaro]

…e index

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.

🌈🌦️📖🚧 Documentation FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-246

[codecov-commenter]

Codecov Report

❌ Patch coverage is 95.78947% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.18%. Comparing base (db7d78a) to head (8cab7ca).

Files with missing lines	Patch %	Lines
src/fdb5/rules/Matcher.cc	0.00%	2 Missing ⚠️
src/fdb5/rules/Rule.cc	87.50%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #246      +/-   ##
===========================================
- Coverage    71.19%   71.18%   -0.02%     
===========================================
  Files          376      376              
  Lines        23762    23786      +24     
  Branches      2478     2482       +4     
===========================================
+ Hits         16918    16932      +14     
- Misses        6844     6854      +10     

☔ 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.

[Copilot]

Pull request overview

This PR adjusts FDB schema/rule handling for optional attributes with defaults so that default values are not implicitly injected into index keys/axes, and adds/updates tests to validate the new behavior (including a new timespan test suite).

Changes:

• Update rule matching/key expansion logic for optional predicates with defaults, and adjust optional matcher behavior.
• Suppress “empty-only” axes entries in fdb-axes output (text and JSON) and update expectations accordingly.
• Add a new timespan test suite with multiple schema layouts to validate indexing/listing/retrieval behavior.

Reviewed changes

Copilot reviewed 26 out of 27 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
tests/fdb/tools/fdb_axes.sh.in	Updates expected fdb-axes outputs to reflect omission of empty-only axes.
tests/fdb/timespan/timespan.sh.in	New integration test script exercising timespan indexing/retrieval under multiple schema layouts.
tests/fdb/timespan/schema1	New test schema variant (timespan default at top level).
tests/fdb/timespan/schema2	New test schema variant (timespan default at second level).
tests/fdb/timespan/schema3	New test schema variant (timespan default at index level).
tests/fdb/timespan/req	New sample retrieve request for timespan combinations.
tests/fdb/timespan/local1.yaml	New local config referencing schema1.
tests/fdb/timespan/local2.yaml	New local config referencing schema2.
tests/fdb/timespan/local3.yaml	New local config referencing schema3.
tests/fdb/timespan/in.grib	New GRIB test input data for timespan tests.
tests/fdb/timespan/CMakeLists.txt	Registers the new timespan test with CTest/ecbuild.
tests/fdb/test_service.cc	Modernizes loops/assertions and adjusts parameter strings in tests.
tests/fdb/etc/fdb/schema	Updates the test schema content/types and adds timespan-related rules/keywords.
tests/fdb/daos/test_daos_catalogue.cc	Adjusts MarsRequest creation via Key::request() default verb.
tests/fdb/api/test_select.cc	Builds requests via MarsParser and ensures inspect requests are non-empty.
tests/fdb/api/test_fdb_c.cc	Adjusts metadata iteration expectations to skip empty optional metadata entries.
tests/fdb/api/test_dist.cc	Ensures inspect request has a verb (MarsRequest{\"retrieve\"}).
tests/fdb/api/test_callback.cc	Updates archived type used in callback test.
tests/fdb/api/test_auxiliary.cc	Updates archived type used across auxiliary/wipe tests.
tests/fdb/CMakeLists.txt	Adds the new timespan subdirectory to the test build.
src/fdb5/tools/compare/grib/CompareBitwise.cc	Removes a leftover commented debug print.
src/fdb5/tools/compare/fdb-compare.cc	Removes an unused member variable.
src/fdb5/rules/Rule.cc	Updates optional/default handling in key matching and key expansion logic.
src/fdb5/rules/MatchOptional.cc	Changes missing optional value lookup to return empty rather than the default.
src/fdb5/database/IndexAxis.cc	Skips emitting axes that only contain a single empty value (text/JSON).
src/fdb5/api/FDB.cc	Filters out postproc/sink parameters before inspecting (index-relevant request only).
src/fdb5/CMakeLists.txt	Comments out building fdb-patch under HAVE_GRIB.

Comments suppressed due to low confidence (2)

src/fdb5/rules/Rule.cc:229

• In findMatchingKey(const eckit::StringList& values), the assertion now allows values.size() to be smaller than predicates_.size() (when optionals exist), but the loop still does values.at(i) for every predicate. If the fingerprint/tokenizer drops trailing empty fields, this can throw std::out_of_range instead of treating missing optional values as empty/default. Consider iterating values and predicates together and, when values runs out, only accepting remaining predicates if they are optional (using an empty string/default as appropriate).

    size_t numPred = predicates_.size();
    for (const auto& p : predicates_) {
        if (p->optional()) {
            numPred--;
        }
    }
    ASSERT(values.size() >= numPred);

    TypedKey key(registry_);

    for (auto iter = predicates_.begin(); iter != predicates_.end(); ++iter) {
        const auto& pred = *iter;

        const auto& keyword = pred->keyword();

        /// @note 1-1 order between predicates and values
        const auto& value = values.at(iter - predicates_.begin());

        if (!pred->match(value)) {

src/fdb5/rules/Rule.cc:438

• tryFill() now asserts values.size() >= numPred (excluding optional predicates), but the fill loop still expects all predicates to be consumed (it_pred == end). If the serialized fingerprint omits trailing empty optional fields, tryFill() will return false (and fill() will ASSERT) even though the remaining predicates are optional. Adjust the termination/consumption checks so that missing trailing optional values are accepted, and make sure the mapping between values and predicates stays aligned.

    size_t numPred = predicates_.size();
    for (const auto& p : predicates_) {
        if (p->optional()) {
            numPred--;
        }
    }
    ASSERT(values.size() >= numPred);  // Should be equal, except for quantile (FDB-103)
    ASSERT(values.size() <= predicates_.size() + 1);

    auto it_value = values.begin();
    auto it_pred = predicates_.begin();

    for (; it_pred != predicates_.end() && it_value != values.end(); ++it_pred, ++it_value) {

        if (values.size() == (predicates_.size() + 1) && (*it_pred)->keyword() == "quantile") {
            std::string actualQuantile = *it_value;
            ++it_value;
            if (it_value == values.end()) {
                return false;
            }
            actualQuantile += std::string(":") + (*it_value);
            (*it_pred)->fill(key, actualQuantile);
        }
        else {
            (*it_pred)->fill(key, *it_value);
        }
    }

    // Check that everything is exactly consumed
    if (it_value != values.end()) {
        return false;
    }
    if (it_pred != predicates_.end()) {
        return false;
    }

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

findMatchingKeys(const MarsRequest&) uses pred->values(request) directly. For optional-with-default predicates, MatchOptional::values() returns the default when the keyword is missing, so this path never considers the empty-string alternative. That can prevent tools that rely on Schema::expandDatabase() (e.g. fdb-root) from matching databases indexed with an omitted optional (stored as empty) when the request omits the keyword. Align this implementation with the ReadVisitor overload: explicitly detect missing keywords and add both defaultValue() and "" (when appropriate) to the candidate values.

[simondsmart]

Looks good. Just two quick queries:

• copilot review notes a discrepancy between fdb-patch in two places in the codebase. Has this been checked.
• I really don't understand why we are changing the behaviour of inspect() wrt. pproc and sink parameters. And certainly not why that is happening in this PR. I don't think that change is conceptually correct - if those parameters are present, they belong to a tool higher up in the chain, and should be being removed there not in the FDB.