B4 GROUP+HAVING profile showed SqlIsAggName at ~9% of CPU —
SqlEvalFunc checks it for every function in every row, and the
PRG body was two string allocations + a substring scan:
RETURN ("," + c + ",") $ ("," + AGG_FUNCTIONS + ",")
Replace with a hash lookup against the existing aggFuncSet map
in hbrtl/sqlexpr.go (already populated for SqlExprHasAgg, same
AGG_FUNCTIONS list). Upper-casing skips the allocation when the
input is already upper, which it almost always is in practice.
Bench deltas (median of 3 steady runs, 1000 iters):
B4_GROUP_HAVING 447 → 418 us -6.5%
B14_COUNT 252 → 235 us -7%
B15_CTE_WIN_JOIN 1595 → 1577 us -1%
Other benches unchanged (no aggregate calls per row).
FiveSql2 43/43, Harbour compat 56/56.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Systematic bug-hunt driven by an automated analysis of all FiveSql2
source files. Each fix is targeted — no speculative refactoring.
--- #1 CLASSDATA hSubCache leaked across queries (CRITICAL) ---
CLASSDATA hSubCache INIT { => } SHARED
shared one hash across ALL TSqlExecutor instances. A non-correlated
subquery cached in query A was silently returned for an unrelated
query B if the subquery text happened to produce the same cache key.
Converted to instance DATA initialized in New().
--- #5+#21 IS NULL / COALESCE treated empty string as NULL (HIGH) ---
RETURN xL == NIL .OR. ( ValType(xL) == "C" .AND. Empty(AllTrim(xL)) )
SQL standard: '' is a valid non-NULL value. Removed the empty-string
check from both IS NULL evaluation and COALESCE skip logic.
--- #4 Multiple ? parameters all returned first value (HIGH) ---
ND_PAR nodes had no index — EvalExpr always returned ::aParams[1].
Parser now stamps each ? with a sequential 1-based index in xNode[2].
EvalExpr uses it to return the correct ::aParams[n].
--- #10+#11 SqlEvalRowExpr missing / and || operators, single-arg
function eval (MEDIUM) ---
Division and string concatenation fell through to RETURN NIL in the
row-expression evaluator used by recursive CTEs and aggregate
ComputeAgg. Also, multi-argument functions like SUBSTR(x,2,3) only
received the first argument. Both fixed.
--- #9 SUM/AVG/MIN/MAX of all NULLs returned 0 instead of NULL
(MEDIUM) ---
SQL standard requires NULL. Changed the aggregate return path to
return NIL when nCount == 0 (SUM/AVG) or when xMin/xMax == NIL.
--- #8 MIN/MAX used SqlCoerceNum for comparison (MEDIUM) ---
Strings and dates were coerced to numbers (Val()) before comparing,
making MIN('banana') == MIN('apple') == 0. Switched to SqlCmpLt
which handles type-appropriate comparison.
--- #7 SqlExprHasAgg only checked top-level node (MEDIUM) ---
Expressions like `salary + COUNT(*)` were not detected as containing
an aggregate because the top node was ND_BIN, not ND_FN. Made the
function recursive — walks ND_BIN, ND_UNI, ND_FN args, ND_CASE
branches.
--- #13 SELECT * only expanded first table in JOINs (MEDIUM) ---
`SELECT * FROM orders o JOIN customers c ON ...` only included
fields from orders. Changed the expansion loop to iterate ALL
entries in ::aTables.
--- #2 s_aOuterStack not unwound on subquery error (HIGH) ---
SubqueryCached's PushOuter/PopOuter pair was not protected by
BEGIN SEQUENCE. A runtime error inside the subquery left a stale
entry on the module-level outer stack, corrupting all subsequent
queries' correlated column resolution. Wrapped in SEQUENCE/RECOVER.
Validation:
- FiveSql2 43/43
- Harbour compat 51/51
- go test ./... ALL PASS
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
A scalar correlated subquery with a JOIN inside:
SELECT e.name,
(SELECT SUM(o.qty * p.price)
FROM ord o INNER JOIN prod p ON o.prod_id = p.id
WHERE o.emp_id = e.id) AS revenue
FROM emp e WHERE e.dept = 'SALES'
returned wrong values (equal to SUM(qty) instead of SUM(qty*price))
or zero for all but the first outer row. Root cause was a triple
interaction between three independent bugs.
--- Bug 1: Subquery cache leaked across five_SQL invocations ---
hSubCorrCache, aSubCacheSlots, aSemiJoinSlots, nSubCacheSeq were
declared as DATA ... INIT { => } / {} / 0. In Five's compiled output,
hash/array INIT literals may share the same backing instance across
New() calls, so the cache from query A (SUM qty, no join) was still
there when query B ran, providing a hit on the same key — returning
A's cached (wrong) value instead of re-executing B's subquery.
Fix: explicit initialization in New().
--- Bug 2: aJoins alias mutation across subquery invocations ---
RunSelect's join-alias sync loop mutated aJoins[i][3] from the
user alias ("p") to the depth-suffixed temp alias ("FA_0003").
aJoins was a direct reference into hQuery["joins"], so the mutation
persisted across re-executions of the same hQuery. On the 2nd call,
the sync loop couldn't find a matching aTables entry because the
stale temp alias ("FA_0003") didn't match the new one ("FA_0005").
The join table's workarea was positioned wrong → empty join result.
Fix: deep-clone both ::aTables and aJoins at the start of RunSelect
so each invocation starts from the parsed originals.
--- Bug 3: SqlCollectCols stripped alias prefixes ---
When adding hidden columns for complex aggregate arguments (e.g.
SUM(o.qty * p.price)), SqlCollectCols returned bare names like
"qty" and "price" instead of qualified "o.qty" / "p.price". In a
JOIN context, unqualified "price" routed FetchRow to the first
table (ord) instead of prod — FieldPos returned 0, the column was
silently NIL, and the multiplication collapsed to qty*1 = qty.
Fix: new SqlCollectColExprs returns the original ND_COL AST nodes
with qualified names preserved. The hidden-column loop now inserts
these directly so FetchRow's dot-qualified path resolves to the
correct workarea via FindWA.
--- Verification ---
Deterministic 5-emp / 6-order / 3-product test:
Expected revenues per emp:
Emp 1: 2*10 + 3*20 = 80 → got 80.00 ✓
Emp 2: 1*10 + 4*30 = 130 → got 130.00 ✓
Emp 3: 5*20 = 100 → got 100.00 ✓
Emp 4: no orders = 0 → got 0 ✓
Emp 5: 7*10 = 70 → got 70.00 ✓
Also verified SUM(qty*2) and SUM(p.price) variants.
Validation:
- FiveSql2 43/43
- Harbour compat 51/51
- go test ./... ALL PASS
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
