Feature/fdb 327 fdb compare

[stefaniereuter]

Description

Opening this as a draft pull request, while finishing the refactoring on the grib comparison but to start discussing the design and features.

This draft shows the direction in which I would like to continue refactoring.

This tool allows to compare two FDB or two experiments in one FDB. It uses the standard fdb tool mechanisms, minimal-keys, CLI request, but also allows to directly specify mars keys, where entries should be ignored. it uses fdb list to generate a request then sorts the results and compares them, depending on scope, where the default is just a comparison of the mars keys, without the messages itself, possibility of comparing grib-header only and then grib-header plus data section. Default is a key by key comparison,
More explanation and examples can be found here:

https://confluence.ecmwf.int/display/~ecm4053/FDB+Comparison

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 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/fdb/pull-requests/PR-165

🌈🌦️📖🚧 Documentation Z3FDB 🚧📖🌦️🌈
https://sites.ecmwf.int/docs/dev-section/z3fdb/pull-requests/PR-165

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

[stefaniereuter]

This was supposed to be a draft pull request....

[pgeier]

Pending tasks:

• add license to source and header files
• document functions
• fix tests
• add data check tests

[codecov-commenter]

Codecov Report

❌ Patch coverage is 71.52778% with 205 lines in your changes missing coverage. Please review.
✅ Project coverage is 73.33%. Comparing base (b073f57) to head (5557389).

Files with missing lines	Patch %	Lines
src/fdb5/tools/compare/grib/CompareBitwise.cc	47.61%	66 Missing ⚠️
src/fdb5/tools/compare/common/Types.cc	70.62%	42 Missing ⚠️
src/fdb5/tools/compare/grib/CompareKeys.cc	56.04%	40 Missing ⚠️
src/fdb5/tools/compare/fdb-compare.cc	85.60%	19 Missing ⚠️
src/fdb5/tools/compare/grib/CompareHash.cc	46.87%	17 Missing ⚠️
src/fdb5/tools/compare/grib/Compare.cc	89.42%	11 Missing ⚠️
src/fdb5/tools/compare/grib/CompareDatasection.cc	83.33%	4 Missing ⚠️
src/fdb5/tools/compare/grib/Utils.cc	84.00%	4 Missing ⚠️
src/fdb5/tools/compare/common/Util.cc	75.00%	2 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop     #165      +/-   ##
===========================================
- Coverage    73.38%   73.33%   -0.06%     
===========================================
  Files          363      373      +10     
  Lines        21956    22676     +720     
  Branches      2255     2380     +125     
===========================================
+ Hits         16113    16629     +516     
- Misses        5843     6047     +204     

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

[simondsmart]

A few extra details...

[simondsmart]

And I note that you have not reformatted the header files in line with our standard convention. Please do not mark this as resolved unless that is done.

[simondsmart]

I've added another review. This PR is a bit weird. The code seems to go out of its way to avoid using some of the classes (notably Key, FieldLocation) in the fdb, and to reimplement (incompletely) the functionality. As such this tool won't work alongside the FDB remote protocol.

I really want to see this using the functionality that we have implemented (as per the specific comments).

I also really don't want to see issues marked as resolved when no attempt has been made to address them (thinking here the overall structural formatting of the files, which does not match the rest of the project).

[simondsmart]

Looks good. Let's do it.

[Ozaq]

Please add documentation, and clean up commits :)

[Ozaq]

Claude Review:

Code Review: fdb-compare Tool (commit 75d9ae9)

Overview

New CLI tool for comparing two FDB (Fields DataBase) instances. Supports three
comparison scopes (MARS keys, header-only, full data) and three GRIB comparison
methods (key-by-key, hash, bit-identical). Well-structured with clear
separation between MARS-level and GRIB-level comparison.

Architecture & Design

• Good: Clean separation into common/, mars/, and grib/ modules
• Good: Strategy-pattern-like dispatch for comparison methods in
  gribCompareSingleMessage
• Good: CompareResult enum with
  Match/OtherMismatch/DataSectionMismatch allows deferred numeric
  comparison
• Good: IIFE pattern for CompFDB initialization and method dispatch is
  idiomatic

Bugs & Correctness Issues

1. NumericError::avg() division by zero (Types.cc:186-187): When count == 0, avg() returns 0.0/0 = NaN. Should guard against this.

2. GRIB1 total length read as 24-bit (CompareBitwise.cc:169): Uses
   readU32Be(bufferRef + 4) but GRIB1 stores total length as 3 bytes at
   offset 4-6 (byte 7 is edition number). Should use readU24Be. This
   contradicts the correct usage in parseGribHeader at line 54.

3. bitComparison uses length from test only (Compare.cc:37):
   gribLocTest.length() is passed but gribLocRef.length() is never checked.
   If ref and test have different lengths, the comparison may read out of
   bounds or miss the mismatch.

4. errCode logic always returns OtherMismatch
   (CompareBitwise.cc:195):

   const auto errCode = i < numSections ? CompareResult::OtherMismatch : CompareResult::DataSectionMismatch;

   Since i iterates 0..numSections-1, i < numSections is always true,
   so errCode is always OtherMismatch. The intent was likely i < numSections - 1 to distinguish header sections from the data section.

5. Unused variables (CompareKeys.cc:208-209): matchreturn and
   dataSectionreturn are declared but never used.

6. Test script bug (all_identical.sh.in:81): [ $tool_rc -e 0 ] should
   be [ $tool_rc -ne 0 ]. The -e flag tests file existence, not numeric
   equality.

Performance

1. assembleCompareMap copies entire DataIndex (mars/Compare.cc:29):
   auto refRemaining(ref) copies the full map including all ListElements.
   Consider using a std::unordered_set<fdb5::Key> of remaining keys instead.

2. File I/O bottleneck acknowledged (Utils.cc:77-79): The comment about
   keeping file handles open is valid. For large FDBs, opening/closing per
   message will dominate runtime. Worth tracking as a follow-up.

3. extractGribMessage allocates per-message (Utils.cc:69-85): For
   sequential comparisons, reusing a buffer would reduce allocations. Minor for
   now.

Code Quality

1. parseMethod silently defaults (Types.cc:138-141): Unknown strings
   silently fall through to KeyByKey. Should throw on unrecognized input
   (like parseScope does) to avoid user confusion.

2. Typos in output strings:

   • Compare.cc:115: "Differenene" should be "Difference"
   • CompareKeys.cc:143: Missing space: "lengthTest= " vs " lengthRef= "

3. fdb-compare.cc:11: File header says fdb-compare.h but this is a
   .cc file.

4. fdb-compare.cc:240: compare::operator<<(eckit::Log::info(), opts_.marsReqDiff) — calling the operator explicitly with qualified name is
   unusual. Using eckit::Log::info() << opts_.marsReqDiff would be cleaner if
   ADL finds it (may need a using declaration).

Security

1. Buffer reads in bitComparison: The ASSERT(length > 16) at line 133
   is good, but the section-walking loop at lines 189-229 trusts
   sectionLengthRef from the buffer without bounds-checking against the total
   length. A malformed GRIB could cause out-of-bounds reads via offset + sectionLengthRef > length.

Test Coverage

• Good: Four test scenarios covering identical FDBs, GRIB header
  mismatches, data mismatches with tolerance, and different experiment versions
• Missing: No unit tests for individual functions (compareValues,
  bitComparison, requestDiff, etc.)
• Missing: No test for error paths (corrupt GRIB, missing config files,
  empty FDBs)
• Missing: No test for the hash comparison method
• Bug in test: all_identical.sh.in:81 uses -e instead of -ne (see
  item 6)

Commit Message

The commit message "Added fdb-compare tool to compare two FDBs" is inadequate
for a 2760-line, 46-file addition. It should:

• Explain the motivation (why is this tool needed, what workflow does it
  enable?)
• Summarize the architecture (three comparison scopes, three GRIB methods, MARS
  key diffing)
• Reference a ticket/issue number if one exists
• Mention key design decisions (e.g. why multiple comparison strategies,
  tolerance-based FP comparison)

End-User Documentation

docs/fdb/cli_tools/compare.rst exists and covers options reasonably, but has
issues:

• Typos and formatting: "comparsion" (line 7), unclosed backtick in example
  on line 79 (config.yaml missing closing > and has trailing ` ),
  unbalanced parentheses on lines 68 and 70
• Example uses wrong binary name (line 84): fdbcompare should be
  fdb-compare
• No explanation of exit codes: The tool returns 0 on match and 1 on
  mismatch (used extensively in tests), but this is undocumented. Critical for
  scripted usage.
• No description of output format: Users need to know what [MARS KEYS COMPARE] SUCCESS, [GRIB COMPARISON MISMATCH], etc. mean and how to parse
  them
• Missing guidance on method selection: No advice on when to use
  grib-keys vs hash-keys vs bit-identical (trade-offs between speed,
  granularity of error reporting, and strictness)
• No explanation of --mars-keys-ignore vs --grib-keys-ignore: The
  difference between filtering at MARS level (excluding entire entries) vs GRIB
  level (skipping specific metadata keys) is not clarified
• Single-FDB mode undocumented: Using --config alone (without
  --reference-config/--test-config) to compare within one FDB is only
  discoverable by reading the source

API Documentation (Headers)

Most public headers have doxygen @param/@return comments, which is
positive. Issues:

• CompareBitwise.h:26: compareHeader() has no documentation at all — no
  description, no @param, no @return
• Utils.h:34-36: isDataKey doc is oddly structured — @return says
  "Check if a given key is a data key" but that's a description, not a return
  value. Should be @return true if the key is data-relevant
• Utils.h:39-40: appendDataRelevantKeys has @return but returns
  void
• CompareResult enum (Utils.h:28-32): No documentation on the enum
  values. DataSectionMismatch vs OtherMismatch semantics are non-obvious
  and important for callers
• Types.h: Options struct fields have inline comments (good), but
  NumericError and Result have no member-level docs. NumericError::min
  being initialized to 0.0 rather than +inf is surprising and undocumented
  — first call to update() will never increase min above 0 even if all
  errors are positive
• mars/Compare.h:19: using namespace compare; in a header pollutes the
  namespace of all includers

Summary

The tool is well-architected with a clean module layout. The main concerns are:

• Critical: GRIB1 length parsing mismatch (item 2), buffer bounds issue
  (item 14), test bug (item 6)
• Medium: Division-by-zero in avg() (item 1), errCode always
  OtherMismatch (item 4), silent default for unknown methods (item 10),
  commit message quality, documentation gaps
• Low: Unused variables, typos, performance notes, API doc inconsistencies