Skip to content

fix: compilation warning in crash recovery ut#217

Merged
zhourrr merged 1 commit intomainfrom
minor/fix_ut_warning
Mar 12, 2026
Merged

fix: compilation warning in crash recovery ut#217
zhourrr merged 1 commit intomainfrom
minor/fix_ut_warning

Conversation

@zhourrr
Copy link
Collaborator

@zhourrr zhourrr commented Mar 12, 2026

Greptile Summary

This PR targets compilation warnings in the crash recovery unit test, but bundles three distinct changes — only one of which directly addresses a warning. The core fix changes the loop variable in data_generator.cc from uint64_t to int to resolve a signed/unsigned comparison warning against batch_end. Alongside it, two functional test-configuration changes are included: reducing max_buffer_size_ from 64 MB to 4 MB in CollectionOptions, and increasing set_max_doc_count_per_segment from 2000 to 10000 in utility.h.

Key observations:

  • The actual warning fix (loop variable type change) is straightforward but leaves a residual implicit intuint64_t conversion when i is passed to CreateTestDoc(uint64_t, int), which may itself trigger -Wsign-conversion warnings.
  • Reducing max_buffer_size_ to 4 MB is a significant functional change (1/16th of the default) that increases flush frequency during tests; no motivation or comment is provided.
  • Increasing max_doc_count_per_segment to 10000 also changes crash-recovery test behavior and is not explained.
  • Mixing functional test-configuration changes into a "fix: compilation warning" commit makes the PR harder to review and audit.

Confidence Score: 3/5

  • Safe to merge for test-only scope, but the undocumented functional changes reduce confidence in intent and correctness.
  • All changes are confined to test utilities with no impact on production code. However, two out of three changes are undocumented functional alterations to test configuration (buffer size, segment doc count) that could silently change crash-window behavior, and the core warning fix leaves a residual implicit conversion issue.
  • tests/db/crash_recovery/data_generator.cc — contains the undocumented max_buffer_size_ reduction and the residual implicit int→uint64_t conversion.

Important Files Changed

Filename Overview
tests/db/crash_recovery/data_generator.cc Contains three distinct changes: the intended loop-variable type fix (int i), an undocumented reduction of max_buffer_size_ from 64 MB to 4 MB in CollectionOptions, and a residual implicit int→uint64_t conversion at the CreateTestDoc call site.
tests/db/crash_recovery/utility.h Only change is increasing set_max_doc_count_per_segment from 2000 to 10000 — a functional test-configuration change unrelated to compilation warnings.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[main] --> B["zvec::Collection::Open\n(max_buffer_size = 4 MB)"]
    B --> C{Open success?}
    C -- No --> D[LOG_ERROR & return -1]
    C -- Yes --> E[Loop: start_id to end_id in batches of 20]
    E --> F["for int i = start_id; i < batch_end\n(was: uint64_t i)"]
    F --> G["CreateTestDoc(i, version)\nuint64_t doc_id ← implicit int→uint64_t"]
    G --> H{operation type}
    H -- insert --> I[collection->Insert]
    H -- upsert --> J[collection->Upsert]
    H -- update --> K[collection->Update]
    H -- delete --> L[collection->Delete]
    I & J & K & L --> M{Results OK?}
    M -- No --> N[LOG_ERROR & return 1]
    M -- Yes --> O[Advance start_id, log progress]
    O --> E
    E -- done --> P[Print success & return 0]
Loading

Last reviewed commit: 21f8dd1

Greptile also left 1 inline comment on this PR.

@zhourrr zhourrr requested a review from chinaux March 12, 2026 02:58
Copy link
Collaborator

@egolearner egolearner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zhourrr zhourrr force-pushed the minor/fix_ut_warning branch from a559ed9 to 21f8dd1 Compare March 12, 2026 08:25
@zhourrr
Copy link
Collaborator Author

zhourrr commented Mar 12, 2026

@greptile

@zhourrr zhourrr merged commit 27c1248 into main Mar 12, 2026
10 checks passed
@zhourrr zhourrr deleted the minor/fix_ut_warning branch March 12, 2026 09:38
iaojnh pushed a commit that referenced this pull request Mar 19, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants