list feature: key levels #30

mcakircali · 2024-08-01T10:44:15Z

adds level var to fdb::list

new fdb-list option: --depth

cherry-picked (from Simon's other branch) and fixed some issues

from Simon branch

codecov-commenter · 2024-08-01T10:49:01Z

Codecov Report

Attention: Patch coverage is 69.69697% with 20 lines in your changes missing coverage. Please review.

Project coverage is 63.56%. Comparing base (c2fbb55) to head (d0a30f0).

Files	Patch %	Lines
src/fdb5/tools/fdb-list.cc	62.96%	10 Missing ⚠️
src/fdb5/api/RemoteFDB.cc	0.00%	5 Missing ⚠️
src/fdb5/database/FieldLocation.cc	37.50%	5 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop      #30      +/-   ##
===========================================
+ Coverage    63.52%   63.56%   +0.03%     
===========================================
  Files          236      236              
  Lines        13410    13426      +16     
  Branches      1291     1288       -3     
===========================================
+ Hits          8519     8534      +15     
- Misses        4891     4892       +1

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

simondsmart

I would like you to review this in two main ways:

Please don't refactor tests in the regressions folder. I would like to see what fails (and what the minimum possible changes are) to make FDB-240 work. That should not break with any of the changes in this PR
I don't like the output that tails empty {} in the list output. It isn't obvious that the ListElement should always contain 3 components when level < 3.
The RemoteFDB handler needs to be made to work
Did you look at what would be involved in removing the NullFieldLocation?

simondsmart · 2024-08-01T10:55:22Z

src/fdb5/api/FDB.cc

@@ -231,8 +231,8 @@ ListIterator FDB::inspect(const metkit::mars::MarsRequest& request) {
    return internal_->inspect(request);
 }

-ListIterator FDB::list(const FDBToolRequest& request, bool deduplicate) {
-    return ListIterator(internal_->list(request), deduplicate);
+ListIterator FDB::list(const FDBToolRequest& request, const bool deduplicate, const int level) {


You've inserted an incorrect const before "bool deduplicate" here.

no, it's correctly set as const. it's passed by value to ListIterator, where it'll live as a non-const.

simondsmart · 2024-08-01T10:58:16Z

tests/fdb/tools/fdb_list.sh.in

+try test "$exp" = "$out"
+
+# test date depth=2
+out=$(fdb-list date=20201102 --minimum-keys="" --porcelain --depth=2)


We shouldn't have the trailing {} - these are not components with empty keys, we just haven't explored there yet.

simondsmart · 2024-08-01T11:05:28Z

src/fdb5/api/local/ListVisitor.h

@@ -55,6 +55,13 @@ struct ListVisitor : public QueryVisitor<ListElement> {
            indexRequest_.unsetValues(kv.first);
        }

+        if (level_ == 1) {
+            /// @todo fix this so that we don't need to create an empty key
+            const auto emptyKey = Key {currentCatalogue_->schema().registry()};


Is this necessary with the schema().registry() - given that there are no keys contained inside? Does anything break if we just default construct the empty keys().

Further, can we not just emplace a single part key here, rather than all 3 parts?

we need this hacky empty keys since there's a Key hash functor using valuesToString .. which kicks in when Key used in unordered containers.. see the note, I will attempt to fix in a later PR.

simondsmart · 2024-08-01T11:07:13Z

src/fdb5/database/Manager.cc

@@ -298,7 +298,7 @@ std::vector<eckit::URI> Manager::visitableLocations(const metkit::mars::MarsRequ

    std::set<std::string> engines = Manager::engines(rq, all);

-    LOG_DEBUG_LIB(LibFdb5) << "Matching engines for request " << rq << " -> " << engines << std::endl;
+    LOG_DEBUG_LIB(LibFdb5) << "Matching engines for request " << rq << (all ? " ALL" : "") << " -> " << engines << std::endl;


Not quite sure what the value of this change is?

I can't tell if useful or not, it was cherry-picked from your branch.

simondsmart · 2024-08-01T11:07:45Z

src/fdb5/remote/Handler.cc

@@ -88,7 +88,10 @@ struct BaseHelper {
 };

 struct ListHelper : public BaseHelper<ListElement> {
+    // void extraDecode(eckit::Stream& s) { s >> level_; }


This is incomplete. As discussed, the remote side needs to work...

simondsmart · 2024-08-01T11:08:48Z

src/fdb5/tools/fdb-list.cc

        options_.push_back(new SimpleOption<bool>("json", "Output available fields in JSON form"));
        options_.push_back(new SimpleOption<bool>("compact", "Aggregate available fields in MARS requests"));
+        options_.push_back(new SimpleOption<long>("depth", "Output entries up to 'depth' levels deep"));


Note " (default 3)" in the usage statement.

simondsmart · 2024-08-01T11:09:41Z

src/fdb5/tools/fdb-list.cc

-    for (auto k : key.keys()) {
-        signature += separator+k;
-        separator=":";
+    for (auto&& k : key.keys()) {


Should be "const auto&". Doesn't need && - not intending either to do move(), perfect forwarding or lifetime extension.

unrelated perf fix.. here, "auto&&" becomes "const auto&"

simondsmart · 2024-08-01T11:10:19Z

src/fdb5/tools/fdb-list.cc

@@ -93,24 +86,20 @@ void FDBList::init(const CmdArgs& args) {
    porcelain_ = args.getBool("porcelain", false);
    json_ = args.getBool("json", false);
    compact_ = args.getBool("compact", false);
+    depth_     = args.getInt("depth", 3);


Given that we set the default in the class/constructor, we should default getInt("depth", depth_). That probably applies for the other arguments as well...

simondsmart · 2024-08-01T11:14:46Z

src/fdb5/tools/fdb-list.cc

-        }
+    if (json_) { porcelain_ = true; }
+
+    if (porcelain_) {


This is quite elegant, in that it removes the need for if (porcelain_) later on.

But I would rather that we still throw a UserError if the user asks for something else, and explain what the conflict is, rather than just give them an output that they likely don't expect.

mcakircali · 2024-08-01T12:11:11Z

I would like you to review this in two main ways:

Please don't refactor tests in the regressions folder. I would like to see what fails (and what the minimum possible changes are) to make FDB-240 work. That should not break with any of the changes in this PR

that's fine, I can understand it. I'm not anxious since I reviewed every line but will remove that commit.

I don't like the output that tails empty {} in the list output. It isn't obvious that the ListElement should always contain 3 components when level < 3.

this is already WIP. there will be a separate PR since it's a big change.

The RemoteFDB handler needs to be made to work

Good point, I will fix that.

Did you look at what would be involved in removing the NullFieldLocation?

Yes, as mentioned, in a separate PR.

mcakircali · 2024-08-05T20:12:34Z

closing, see more involved PR #31

mcakircali added 15 commits July 29, 2024 12:11

test(toKey): add check step value

2c2d872

test(FDB-240): print failing command

b3a368a

feat(list): cherry-pick level

844009e

from Simon branch

test(fdb-list): add new

5c1270d

test(fdb-list): prepare grib file

5b56c5e

fix(fdb-list): init class member

6393740

fix(fdb-list): unnecessary copy in loop

044af72

fix(fdb-list): use make_unique not "new"

f9762f3

fix(fdb-list): use "override" (not virtual)

54ee84c

fix(fdb-list): default member init and tab size

c53c13f

test(fdb-list): added --depth option

a3b9f4d

fix(ListVisitor): empty keys

c5c761a

style(fdb-list): clean code

a8fd786

fix(fdb-list): porcelain defaults other options

eb4db7f

chore(remote/Handler): todo list w/ level

d0a30f0

mcakircali requested a review from simondsmart August 1, 2024 10:44

simondsmart requested changes Aug 1, 2024

View reviewed changes

mcakircali closed this Aug 5, 2024

mcakircali deleted the feature/list-depth-level branch August 8, 2024 07:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

list feature: key levels #30

list feature: key levels #30

mcakircali commented Aug 1, 2024

codecov-commenter commented Aug 1, 2024

simondsmart left a comment

simondsmart Aug 1, 2024

mcakircali Aug 1, 2024

simondsmart Aug 1, 2024

simondsmart Aug 1, 2024

mcakircali Aug 1, 2024

simondsmart Aug 1, 2024

mcakircali Aug 1, 2024

simondsmart Aug 1, 2024

simondsmart Aug 1, 2024

mcakircali Aug 1, 2024

simondsmart Aug 1, 2024

mcakircali Aug 1, 2024

simondsmart Aug 1, 2024

mcakircali Aug 1, 2024

simondsmart Aug 1, 2024

mcakircali commented Aug 1, 2024

mcakircali commented Aug 5, 2024

list feature: key levels #30

list feature: key levels #30

Conversation

mcakircali commented Aug 1, 2024

codecov-commenter commented Aug 1, 2024

Codecov Report

simondsmart left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcakircali commented Aug 1, 2024

mcakircali commented Aug 5, 2024