Skip to content

fix(engine): partial SEEK keeps Found() true when deleted rows are hidden - #154

Closed
russimicro wants to merge 1 commit into
FiveTechSoft:mainfrom
russimicro:pr/seek-partial-key-prefix-deleted
Closed

fix(engine): partial SEEK keeps Found() true when deleted rows are hidden#154
russimicro wants to merge 1 commit into
FiveTechSoft:mainfrom
russimicro:pr/seek-partial-key-prefix-deleted

Conversation

@russimicro

Copy link
Copy Markdown
Collaborator

Table::seek_key re-derives the exact flag whenever deleted records are hidden, and it did so by padding the search key out to the full index key width with spaces:

del_key.resize(idx->key_length(), ' ');
ck.resize(del_key.size(), ' ');
exact = (ck == del_key);

A partial (prefix) key can never satisfy that — the pad bytes get compared against the stored suffix. For a tag built as CON+DOC+STR(SEQ,6,0), a 10-byte CON+DOC seek compared "2P 972299" + " " against "2P 972299" + " 1", so the cursor landed on the right record but AdsIsFound reported 0. It only happens with SET DELETED ON, which is what made it look arbitrary. The same padding delimited the equal-key run in the walk_to_last (AdsSeekLast) block.

Clipper, Harbour DBFCDX and SAP ACE all treat a shorter seek key as a prefix match, and legacy xBase code relies on it: SEEK con+doc followed by IF !FOUND() over a detail table reported every document as missing on the ADS path while DBFCDX found them.

CdxIndex::seek_key itself was already correct (covered by cdx_prefix_seek_test); the defect was strictly the Table-layer re-derivation. The fix compares only the bytes the caller supplied; a key longer than the index key keeps the historical truncation. It applies to both ADS_CDX and ADS_ADT, since it sits in the shared engine layer.

Test

abi_prefix_seek_deleted_test — prefix / full / absent key under both AdsShowDeleted states, plus a group whose first line is deleted (the prefix must still find the surviving line) and a fully deleted group (must miss).

Verified that the test actually catches the defect: built against the tree with the fix reverted, it fails on exactly the two prefix-with-DELETED-ON assertions.

build unit suite this test
without the fix 1241 / 1248 FAIL — 56/58 assertions
with the fix 1242 / 1248 PASS — 58/58 assertions

The remaining failures are the pre-existing Remote: / networking cases (abi_pritpal_lock_test, embedded TCP server), identical with and without this change in our environment.

Real-data validation

Harbour 3.2 + rddads + this DLL, against a production table (1,674 records, 6 tags, 16-byte tag 1). 18/18 across CDX and ADT/ADI, both DELETED states, including the non-regression case (absent prefix must miss):

format RDD DELETED partial (10) full (16) absent prefix
CDX ADS OFF / ON SI / SI SI / SI NO / NO
CDX DBFCDX OFF / ON SI / SI SI / SI NO / NO
ADT/ADI ADS OFF / ON SI / SI SI / SI NO / NO

ADS now matches DBFCDX in every cell. Before the fix, ADS / DELETED ON / partial was the only failing one.

Found while running a Harbour/FiveWin ERP on OpenADS: an ADS/CDX company could not read any movement document. Validated end-to-end in the ERP with no application change.

…dden

Table::seek_key re-derives the `exact` flag whenever deleted records are
hidden, and it did so by padding the search key out to the full index key
width with spaces:

    del_key.resize(idx->key_length(), ' ');
    ck.resize(del_key.size(), ' ');
    exact = (ck == del_key);

A partial (prefix) key can never satisfy that: the pad bytes are compared
against the stored suffix. For a tag built as CON+DOC+STR(SEQ,6,0) a
10-byte CON+DOC seek compared "2P  972299" + 6 spaces against
"2P  972299" + "     1", so the cursor landed on the right record but
AdsIsFound reported 0 -- only with SET DELETED ON, which is what made it
look arbitrary. The same padding delimited the equal-key run in the
walk_to_last (AdsSeekLast) block.

Clipper, Harbour DBFCDX and SAP ACE all treat a shorter seek key as a
prefix match, and legacy xBase code relies on it: `SEEK con+doc` followed
by `IF !FOUND()` over a detail table reported every document as missing on
the ADS path while DBFCDX found them.

CdxIndex::seek_key itself was already correct (covered by
cdx_prefix_seek_test); the defect was strictly the Table-layer re-derivation.

Compare only the bytes the caller supplied. A key longer than the index key
keeps the historical truncation.

Test: abi_prefix_seek_deleted_test -- prefix / full / absent key under both
AdsShowDeleted states, plus a group whose first line is deleted (the prefix
must still find the surviving line) and a fully deleted group (must miss).
Verified to fail on the two prefix-with-DELETED-ON assertions when built
against the unfixed tree.

Found running a Harbour/FiveWin ERP on OpenADS: an ADS/CDX company could not
read any movement document. Suite: 1242/1248 -- the 6 failures are the
pre-existing `Remote:` locking cases in abi_pritpal_lock_test, identical with
and without this change.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01K5wBA9raJypuatEm7WdQv6
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

FiveTechSoft added a commit that referenced this pull request Jul 30, 2026
Cherry-picked and conflict-resolved against main (post CI fix 54e977c):

- #150 fix(sql): ORDER BY cursor keeps source column types
- #148 fix(adi): tag ordinals follow creation order
- #152 fix(adt): non-.adt extension (.DAT) works end to end
- #151 fix(session): create honours absolute path with existing parent
  (+ #include <vector> so the new unit test builds under clang)
- #154 fix(engine): partial SEEK keeps Found() with SET DELETED ON
- #155 fix(engine): ordScope bound shorter than key is a prefix
- #153 perf(engine): live key count without per-row goto + recno order

Also includes the CMakeLists resolution that keeps every new regression
case in the unit-test target.
@FiveTechSoft

Copy link
Copy Markdown
Owner

Integrated into main as part of the PR stack merge (9e7ac41). Cherry-picked onto post-CI-fix main, conflict-resolved (CHANGELOG / CMakeLists), and verified locally. Thank you @russimicro.

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