feat(adi): v2 tags — compound / computed / FOR expressions on ADT, opt-in - #165
Merged
Merged
Conversation
Reapplies the Russoft ADI v2 rework on top of v1.8.31. Upstream touched ADI only twice since our fork point (+47/-4), so the driver is taken from our tree and those two changes are re-applied on top: - CICHAR case-insensitive collation (9376af9): fold_for_compare_ + has_ci_component_, used by compare_keys_ and the branch descent. - IIndex::file_path() override for the management surface (9539cf3). What the v2 rework brings: a variable-length dense leaf whose entries are front-coded against the previous key (split chosen so BOTH halves stay under the page cap), compound keys, multilevel clear, and a bottom-up bulk build. clear_data() and build_bulk() move up to IIndex as virtuals with working defaults (erase-every-entry / per-record insert), so AdiIndex overrides them the same way CdxIndex does and callers dispatch polymorphically. Suite: 1177/1189, same 12 SQL-parser (7200) failures as the baseline — no regressions. (abi_remote_ordered_prefetch is flaky on its own: 1 of 3 solo runs fails on a byte-volume threshold, with and without this change.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 05f004f) (cherry picked from commit fcda3eb1303f20a6f297bdf27b1ae7029441420c)
The key-collection loop bulk-loaded only CDX; ADI fell back to per-record insertion. Now that clear_data/build_bulk are IIndex virtuals, collect for both and dispatch through idx_owner->build_bulk(), which reaches the v2 ADI dense-leaf bottom-up build. NTX keeps the incremental path via the default. Suite: 1177/1189, same 12 SQL-parser (7200) failures as the baseline. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 008c28b) (cherry picked from commit 39388566c1d33b807f23c0fba09d47540a7f4d8e)
The ERP stores ADT data in <base>.DAT with a companion <base>.ADI (ARC-CAJA ExtFile='.DAT'), opened via ADS_ADT. Both places that decided "is this an ADT table?" tested the extension only, so a .DAT table got the .cdx default bag on AdsCreateIndex61 (5000) and never auto-bound its .adi on open. - AdsCreateIndex61: fall back to dynamic_cast<AdtDriver*>(t->driver()) when the extension is not .adt, so the structural bag defaults to .adi. - AdsOpenTable: auto-open the companion .adi for .dat as well as .adt (case-insensitive extension compare). Fixes abi_adi_dat_extension_path_test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 4772150) (cherry picked from commit 419ede3003bf16cdd535e41a7e40bf12c4e1b607)
Brings over the 9 tests written against our June tree and registers them in
tests/CMakeLists.txt. They pin ERP behaviour that upstream has no coverage
for, so they double as the checklist for what is still missing from this
integration branch.
Green now: abi_adi_clear_multilevel, abi_adi_dat_extension_path,
abi_adi_frontcoding_size (abi_adi_reindex_bench is skip-by-default).
RED — remaining gaps, all in ace_exports/engine logic not yet ported:
abi_adi_estaelec_compound (ADI->CDX reroute, 5000)
abi_adi_native_estaelec (2 cases: compound/computed/FOR tags, and
tag ordinals in creation order, 5000)
abi_cdx_estaelec_compound (navigation lands on recno 1)
abi_stale_index_walk (AdsGotoTop 5000 after PACK)
abi_sql_temp_browse_nav (ORDER BY result browse out of sync)
Suite: 1181/1199 — the 12 pre-existing SQL-parser (7200) failures plus these 6.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 4a9a27a)
(cherry picked from commit 3901f4f1d106da76b9ca4a1e2bcd1cef423cdf9b)
Upstream's ADI create path accepts only a bare field name ('ADI index
expression must be a bare field name'), so the ERP's ESTAELEC tag set
(cCodigoCon+cDocumeTra, DTOS(dFecTraTra), FOR cCorEnvEle != 'S') could not be
built on an ADT table at all.
Ported from our tree:
- AdsCreateIndex61 falls back to the first field for tag metadata when the
expression is not a bare field, and creates a v2 tag (identity by NAME, with
key expression / FOR condition / full key length persisted in the per-tag
header). A bare numeric or date field keeps the legacy numeric ADI leaf so
existing packed-key seeks still work.
- Overwriting an existing tag matches on the v2 tag name (falling back to the
field name for legacy bags) and clears its tree instead of failing.
- ADI->CDX reroute: an ADT table's .adi bag can be routed through the CdxIndex
engine via OPENADS_ADT_CDX_INDEX=1, and is routed automatically whenever the
bag on disk already carries the CDX 'RCHB' signature, so a reroute-written
bag opens correctly even without the env flag. AdsOpenIndex mirrors the same
routing.
Fixes abi_adi_native_estaelec 'tag ordinals follow creation order'.
Suite 1184/1199: the 12 pre-existing SQL-parser failures plus 3 still-red
Russoft tests (FOR-clause key count on a native ADI tag; index count after
reopening a rerouted bag; SQL ORDER BY cursor recno semantics).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
(cherry picked from commit 1f4f310)
(cherry picked from commit c65b8bde3176d0f267168b128e597e5fdfc2c756)
…Index Two ADI paths still assumed CDX-or-nothing: - AdsGetKeyCount / AdsGetRecordCount(index handle) special-cased CdxIndex and otherwise fell through to the table's record_count(). A native ADI v2 tag with a FOR clause therefore reported every row (4) instead of its matching subset (2), which is what rddads' OrdKeyCount drives xBrowse position math from. Both now count the ADI index walk, honouring SET DELETED. - AdsOpenIndex listed a rerouted bag's tags through CdxIndex but then opened each tag with the native AdiIndex reader, which cannot parse a CDX-format .adi — the call failed and the auto-open on AdsOpenTable swallowed it, so a reopened table showed 0 indexes and the first AdsSetIndexOrder returned 5000. The per-tag open now follows the same adt_to_cdx routing as the listing. Fixes abi_adi_native_estaelec and abi_adi_estaelec_compound. Suite 1186/1199: the 12 pre-existing SQL-parser (7200) failures plus abi_sql_temp_browse_nav, which is a deliberate semantic difference (ORDER BY cursor recno numbering), not a port gap. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> (cherry picked from commit 2e845ac) (cherry picked from commit c82b422476fcbb1e4117e01ee745a02756092d83)
With an ADI order active the count now comes from the index walk (through count_live_recnos, the helper the CDX branch already uses) instead of the table's physical record count. Without it a conditional (FOR) tag reports every row in the table rather than the subset it indexes — the rule AdsGetKeyCount already applies. It must not walk the table per call: rddads answers OrdKeyCount() through this entry point and FWH's TXBrowse asks for it hundreds of times while opening a screen. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With the switch off, AdsCreateIndex61 behaves exactly as it did before: a bare field tag takes the legacy layout, and a compound / computed expression is rejected rather than silently written into a tag header that has nowhere to keep it. With the switch on, the v2 layout is used for character and expression keys. The two test cases that exercise v2 turn it on for their own duration and put it back afterwards, so the rest of the suite keeps running against the legacy layout. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
IIndex now declares virtual clear_data() and build_bulk(); CdxIndex already implements both but lacked the override keyword, so -Winconsistent-missing-override failed the CI build under clang (PR FiveTechSoft#165).
cur_idx_ and the dense-leaf loop counter are signed; indexing the vector with them trips -Wsign-conversion under -Werror on clang CI.
_putenv_s is MSVC-only; the two v2-gated ADI tests used it unconditionally and failed the clang CI build. Mirror the EnvGuard pattern already used by abi_adi_estaelec_compound_test.
Reopen used ADS_DEFAULT (shared). Shared GoHot requires RLock/FLock for AdsWriteRecord; AdsAppendRecord auto-lock was not enough on this path and the suite returned 5035. Open exclusive for write/setup, matching the pattern already used by abi_pritpal_lock_test.
…chSoft#164 Keep both the OPENADS_ADI_V2 gate (adi_v2_enabled) and the live-key count memoisation from FiveTechSoft#160. Register the new keycount cache test alongside the ADI v2 regression suite.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #147. Closes #159.
What the format cannot do today
A
.adibag stores, per tag, a single field number. That is enough forINDEX ON Field, and it is what SAP's format holds. It cannot express what anxBase application actually indexes:
Our article table asks for 22 tags of those shapes. On today's format the
bag ends up holding one, so every
DBSETORDER/ column-click that asks for anorder that is not there falls back to a scan or to building the index on the
fly over 34,595 rows. #159 is the same limitation seen from the API side
(
AdsCreateIndex61with a compound expression -> 5063).What v2 is
An OpenADS-proprietary layout for the per-tag header and the dense leaf:
FORconditionpersisted, so both survive a reopen (the legacy header has nowhere to put them);
up the bag;
CREATE INDEX/REINDEX— inserting one key at atime is not viable on a large table;
record_count().It is not byte-compatible with SAP's
.adi. That is the whole point of thequestion below: a bag written by v2 is ours, not theirs.
How the switch behaves
OPENADS_ADI_V2, read at index-creation time.AdsCreateIndex61behaves exactly as it does onmain:a bare field tag takes the legacy layout, and a compound / computed expression
is rejected rather than written into a header that cannot describe it.
Reading auto-detects: a v2 bag is recognised by its own metadata, a legacy bag
is read as before, with the switch in either position.
Measured on real data
34,595-row ADT table, tags created from scratch:
cCodigocClasi1+...+cClasi5+cCodigoUPPER(cNombre)cCodigo FOR nExistencia > 0Opening the resulting 26 MB bag with 22 tags: 17.8 ms.
Known gap, stated up front: a numeric key does not take the bulk path yet
and builds in ~3 s. The ordering is correct; it is the build that is slow.
What it touches outside the ADI driver
One branch in
AdsGetRecordCount: with an ADI order active the count comes fromthe index walk (via
count_live_recnos, the helper the CDX branch already uses)instead of the table's physical record count. Without it a conditional tag
reports every row in the table rather than the subset it indexes.
rddadsanswers
OrdKeyCount()through this entry point andTXBrowseasks for ithundreds of times per open, so it must not walk the table per call.
One behaviour change you need to know about
With v2 on,
AdiIndex::list_tags()returns the tag name the caller passed toAdsCreateIndex61(TCODIGO), not the indexed field name (CCODIGO) — thelegacy format could only report the latter because it stored nothing else. With
the switch off, nothing changes.
Tests
Full suite green with the switch off (1297 cases): that is the important
number, since off is the default. The two cases that exercise v2 turn it on for
their own duration and put it back afterwards.
New:
abi_adi_native_estaelec_test(compound / computed / FOR tags, ordinals),abi_adi_frontcoding_size_test(leaf density),abi_adi_estaelec_compound_test,abi_adi_reindex_bench_test,abi_adi_dat_extension_path_test,abi_adi_clear_multilevel_test.The question
Any of these three works for us, we just need to know which:
(a) it goes in behind
OPENADS_ADI_V2, off by default — as it stands in thisPR: nothing changes for existing users and we stop maintaining a parallel format;
(b) it goes in as the default for newly created bags, keeping the legacy
reader for existing ones;
(c) it does not go in, and we keep it in our fork.
(c) is a perfectly good answer. What we cannot do is keep guessing: #147 has
been open since 29 July with no reply. If the honest answer is "not this
format", saying so costs you nothing and saves us from syncing something you
intend to solve another way.