Skip to content

fix(engine): an ordScope bound shorter than the index key bounds by prefix - #155

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

fix(engine): an ordScope bound shorter than the index key bounds by prefix#155
russimicro wants to merge 1 commit into
FiveTechSoft:mainfrom
russimicro:pr/scope-partial-key-prefix

Conversation

@russimicro

Copy link
Copy Markdown
Collaborator

Clipper / DBFCDX treat a scope bound shorter than the index key as a prefix: every key beginning with it is inside the scope. Three places assumed the bound was always full width, and together they emptied the scope instead.

1. Table::key_in_bottom_scope_ compared the full-width key against the bound

return key <= *order_->scope().bottom;

Every extension of a prefix sorts after the prefix. With a CON+DOC+STR(SEQ,6,0) tag (16 bytes) and a CON+DOC bound (10), every row fell past the bottom — the scope came out empty, where DBFCDX over the same table returns that document's lines. key_in_top_scope_ was right by accident (an extension is >= the prefix); it is made symmetric so a future edit cannot break one without the other.

2. AdsSetScope padded the bound out to key_length

That padding was added so an unpadded bound on a single-field tag would work — rddads sends OrdScope() values at hb_itemGetCLen(), i.e. trimmed. But it is exactly what breaks the partial-key case that shares the path: the bound becomes "CON+DOC" + 6 spaces, which every real key sorts after.

With prefix comparison the padding is unnecessary for both cases, so it is gone. An over-long bound is still trimmed to key_length.

3. CdxIndex::count_scoped_keys compared key_size_ bytes against the bound

Once bounds are no longer padded, that would also read past a short bound's buffer. It now compares min(bottom.size(), key_size_).

Test

abi_scope_partial_key_test — partial bound as top+bottom (selects the whole document), bottom-only, top-only, full-width bounds (exactly one line), a partial bound matching nothing (empty scope), and no scope at all.

Plus a second case covering what the padding in (2) was protecting: a trimmed bound on a single-field C(8) tag must still select its row — now by prefix comparison instead of by padding the bound.

Real-data validation

Harbour 3.2 + rddads, production table with a 16-byte tag. ADS now matches DBFCDX in all four combinations of RDD × SET DELETED, for the scoped walk, for a partial seek inside an active scope, and with a filter under SET OPTIMIZE ON:

[ADS   ] DELETED OFF/ON  ordScope parcial -> 1 row (expected 1)   OK
[DBFCDX] DELETED OFF/ON  ordScope parcial -> 1 row (expected 1)   OK

Before the change both ADS rows returned 0.

Suite: 1234/1240 — the 6 failures are the pre-existing Remote: locking cases in abi_pritpal_lock_test (embedded TCP server), identical with and without this change in our environment.

Found running a Harbour/FiveWin ERP on OpenADS, while checking the other consumers of the key-comparison primitive touched by #154. Independent of that PR: this one applies cleanly on its own.

…refix

Clipper / DBFCDX treat a scope bound shorter than the index key as a prefix:
every key beginning with it is inside the scope. Three places assumed the
bound was always full width, and together they emptied the scope instead.

  1. Table::key_in_bottom_scope_ compared the full-width index key against
     the bound: `key <= *scope().bottom`. Every extension of a prefix sorts
     AFTER the prefix, so with a CON+DOC+STR(SEQ,6,0) tag (16 bytes) and a
     CON+DOC bound (10) every row fell past the bottom -> 0 rows in scope,
     where DBFCDX returns that document's lines. key_in_top_scope_ was right
     by accident (an extension is >= the prefix); it is made symmetric.

  2. AdsSetScope padded the incoming bound out to key_length with spaces.
     That was added so an unpadded bound on a single-field tag would work
     (rddads sends OrdScope() values at hb_itemGetCLen(), i.e. trimmed), but
     it is what breaks the partial-key case sharing the same path: the bound
     becomes "CON+DOC" + 6 spaces, which every real key sorts after. With
     prefix comparison the padding is unnecessary for both cases, so it is
     gone; an over-long bound is still trimmed to key_length.

  3. CdxIndex::count_scoped_keys compared key_size_ bytes against the bound.
     Once bounds are no longer padded that also read past a short bound's
     buffer. It now compares min(bottom.size(), key_size_).

Test: abi_scope_partial_key_test -- partial bound as top+bottom (the whole
document), bottom-only, top-only, full-width bounds, a partial bound matching
nothing, and no scope at all. Plus a second case for what the padding in (2)
was protecting: a TRIMMED bound on a single-field C(8) tag must still select
its row, now by prefix instead of by padding.

Real data (Harbour 3.2 + rddads, production table, 16-byte tag): ADS now
matches DBFCDX in all four combinations of RDD x SET DELETED, for the scoped
walk, for a partial seek inside an active scope, and with a filter under
SET OPTIMIZE ON.

Suite: 1244/1250 -- 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 <[email protected]>
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