Skip to content

fix(sql): ORDER BY cursor keeps the source column types (#146) - #150

Closed
russimicro wants to merge 1 commit into
FiveTechSoft:mainfrom
russimicro:fix/sql-orderby-preserves-column-types
Closed

fix(sql): ORDER BY cursor keeps the source column types (#146)#150
russimicro wants to merge 1 commit into
FiveTechSoft:mainfrom
russimicro:fix/sql-orderby-preserves-column-types

Conversation

@russimicro

Copy link
Copy Markdown
Collaborator

Fixes #146.

Problem

#136 materialises a single-table SELECT ... ORDER BY / DISTINCT / LIMIT
through build_memory_result(). That helper was written for system.*
catalogs and sp_* result sets, where every column is small and integral:

if (col.type == 'N') {
    f.raw_type = 11;
    f.type = ...::Integer;
    f.length = 4;            // the caller's length/decimals are discarded
}
...
if (rlen > 0xFFFFu) return Error{AE_INTERNAL_ERROR, 0, "result table record too large", tag};

The ORDER BY block does pass sc.decimals = fd.decimals, but the helper ignores
it. So over a user table:

  • every N(n,d) comes back truncated — 12345.67 reads as 12345 — and values
    past 2^31 wrap. Silent, which is worse than the error;
  • a wide table exceeds the 64 KB record cap and the statement fails. This is how
    we hit it: Select * From [articulo.dat] as a ... order by a.cnombreart on a
    187-field table returned ADSCDX/1012.

Change

Materialise from the source field descriptors: create a temp table from the
source structure (AdsCreateTable with the projected columns' real type, length
and decimals), copy the ordered rows in, close the live source, hand back a
cursor over the temp.

Every property #136 was after is kept:

  • the cursor is staticINDEX ON over the result cannot reach the
    production .cdx;
  • recnos are 1..N in result order;
  • the column ACL is applied before the copy, so a restricted user's temp holds
    only permitted columns;
  • no residue: the temp is registered against the cursor handle and
    AdsCloseTable removes its files, including any .cdx/.fpt an application
    built on the result. That is the one thing the memory table gave for free, so
    it is done explicitly here (materialised_cursor_temps /
    drop_materialised_cursor_temp).

The trade against a memory table is that the result lands on disk. For the ERP
workload that is the point — the application indexes and browses the result, and
it can be larger than is comfortable to hold in memory.

build_memory_result is untouched and still serves its catalog / sp_*
callers.

Verification

tests/unit/abi_sql_orderby_types_test.cpp — an N(12,2) column through
SELECT * ... ORDER BY, checking 10.50, 0.05 and 12345.67 round-trip.
Fails before the change (10, 0, 12345), passes after.

abi_sql_orderby_test.cpp (the #136 tests: materialised recnos, ORDER BY data,
source .cdx untouched) keeps passing.

Full suite on this branch (MSVC x64 Release): 1227/1235. The 8 failures are
pre-existing on main and unrelated — 6 × abi_pritpal_lock_test (remote
connect, the residual noted in the v1.8.37 release) and 2 ×
abi_server_fs_test whose teardown threw on remove_all because the temp
directory was held by another process on this machine.

This has been running the reporting ERP's ~60 SQL call sites in production
shape since we adopted v1.8.37.

…#146)

FiveTechSoft#136 materialises a single-table ORDER BY / DISTINCT / LIMIT through
build_memory_result(), a helper written for system.* catalogs and sp_*
result sets. It types every numeric column as a 4-byte integer and caps
the record at 64 KB, so a SELECT * over a user table dropped the decimals
of every N(n,d), wrapped anything past 2^31, and failed outright on a wide
table. Reported from an ERP whose article table has 187 fields and prices
with two decimals.

Materialise from the SOURCE field descriptors instead: create a temp table
from the source structure, copy the ordered rows into it, close the live
source and hand back a cursor over the temp. Types round-trip, and the
FiveTechSoft#136 properties hold -- the cursor is static, INDEX ON over it cannot
reach the production .cdx, and recnos are 1..N in result order.

The temp is registered against the cursor handle and its files (.dbf plus
any .cdx/.fpt an application built on the result) are removed by
AdsCloseTable, so a data directory does not accumulate one temp per query.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@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.

v1.8.37 regression (#136): the ORDER BY memory cursor types every numeric as int32 and caps the record at 64 KB

2 participants