MemberJunction / MemberJunction/MJ
QueryPagingEngine.outerWrap hoists an alias-qualified ORDER BY out of scope, producing invalid SQL
- Dominant language
- TSQL
- Stars
- 29
- Forks
- 6
- Avg merge
- 2d 1h
- Merged PRs (30d)
- 323
Description
## Summary
`QueryPagingEngine.outerWrap` enforces `MaxRows` by wrapping a query in a derived table. Because `ORDER BY` is illegal inside a derived table on SQL Server, it correctly strips the top-level `ORDER BY` from the inner query and re-attaches it to the outer `SELECT` — but it re-attaches it **verbatim, keeping the table-alias qualifier**. The alias only exists inside the derived table, so the resulting SQL cannot bind.
## Reproduction
Input query (valid, returns 21 rows on its own):
```sql
SELECT nl.NeoGovTrainingRecordID, nl.Completion_Date, ...
FROM [dbo].[vwEmployees] e
JOIN [dbo].[vwNeoGovLearns] nl ON LOWER(LTRIM(RTRIM(CASE WHEN TRY_CAST(...) ...)))
WHERE e.EmployeeID = 16615
ORDER BY nl.Completion_Date DESC
```
What `outerWrap` produces:
```sql
SELECT TOP 10 * FROM (
SELECT nl.NeoGovTrainingRecordID, nl.Completion_Date, ...
FROM [dbo].[vwEmployees] e JOIN [dbo].[vwNeoGovLearns] nl ON ...
) AS _mj_capped
ORDER BY nl.Completion_Date DESC -- `nl` does not exist at this level
```
Result:
```
The multi-part identifier "nl.Completion_Date" could not be bound.
```
Verified against a live database:
| Form | Result |
|---|---|
| as `outerWrap` emits it | **FAIL** — `"nl.Completion_Date" could not be bound` |
| identical, qualifier stripped (`ORDER BY Completion_Date`) | **OK — 10 rows** |
## When it fires
Only on the `outerWrap` fallback path. Most queries take the AST path, which injects `TOP`/`LIMIT` inline and never wraps. `outerWrap` is reached when the AST cannot parse the SQL — and the code comment names the culprits directly (`TRY_CAST`, `IIF`, `STRING_AGG`). So the trigger is:
**alias-qualified top-level `ORDER BY` + SQL the AST cannot parse.**
A query without `ORDER BY` passes; the same query with one fails.
## Impact
The failure surfaces to callers as a SQL error on a query that is actually valid. In our case the caller is an LLM-driven query-writing loop: it sees `"nl.Completion_Date" could not be bound` next to its own SQL, where `nl` is plainly in scope, so it has no way to diagnose the problem and burns its entire retry budget making cosmetic edits. The wrapper is never visible to it.
Note that `TestQuerySQLResolver` returns `RenderedSQL`, which would make this diagnosable — callers that don't surface it are flying blind.
## Suggested fix
When hoisting the `ORDER BY` to the outer `SELECT`, strip the table-alias qualifiers. The derived table exposes columns by their output name, so `ORDER BY Completion_Date` binds correctly.
One caveat worth handling: if an `ORDER BY` column is not in the `SELECT` list it won't exist in the derived table, and stripping the qualifier still won't bind. That case needs either skipping the wrap or dropping the `ORDER BY`. It fails 100% today either way, so qualifier-stripping is strictly an improvement.
## Location
`packages/GenericDatabaseProvider/src/queryPagingEngine.ts` — `QueryPagingEngine.outerWrap`
Contributor guide
Research direction
Start in packages/GenericDatabaseProvider/src/queryPagingEngine.ts at QueryPagingEngine.outerWrap and reproduce the supplied SQL Server case with an alias-qualified ORDER BY on the fallback path. Check how the top-level ORDER BY is extracted and reattached, then verify that the wrapped query binds the ordering column and returns the capped rows without the multi-part identifier error.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- sql, typescript
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 2/5
- Estimated time
- 1-3 hours
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 75/100