Skip to content

Commit 18be7d3

Browse files
reinaldocrespoclaude
reinaldocrespo
and
claude
committed
fix(dd): proc/function SQL bodies no longer truncated (catalog + binary parse)
Found while diffing the imported PMSYS DD against SAP: stored-proc and function bodies came back truncated, and every imported proc was missing its opening statement. Two distinct bugs, neither in DD storage (which held full bodies all along): 1. Catalog truncation (ace_exports.cpp): build_system_table declared system.storedprocedures PROCEDURE and system.functions FUNC_BODY as fixed CHAR(255) — a 5359-byte body (pmsys sp_SaveIntoAuditLog) came back as a 255-byte prefix. system.triggers CONTAINER C(4096) and system.views VIEW_SQL C(250) had the same class of cap. Fix: build the rows first and size the text columns to the longest actual value (new fit_width helper, clamped under the 64 KB record limit). 2. SAP-binary proc parser dropped each body's FIRST LINE (data_dict.cpp): the parser located the body by scanning forward to the first CRLF — but the first CRLF is the END of line 1, so e.g. sp_GetPhysicalPath lost "DECLARE @SQL STRING;". Hex-dumping pmsys.add shows the real layout after the params: [le16 invoke_option][le32 reserved][le16 body_len][body...], with the inline zone holding the first bytes and the .am continuation exactly the rest. Parse that structurally (body_len is authoritative — no CRLF/printable-scan heuristics), keeping the old scan as a fallback for layouts that don't match. Verified against PMSYS with SAP ACE as oracle: after re-import, all 7 stored-proc bodies and all 8 comparable function bodies byte-match SAP's system.* output (whitespace-normalized). Regression tests: - data_dict_test.cpp "DataDict SAP-binary proc parse keeps the body's first line" — fabricates a minimal SAP-binary .add with a structured Procedure record; fails on the old CRLF scan. - abi_dd_proc_view_test.cpp "system.storedprocedures returns >255-byte bodies untruncated". Note: proc/UDF EXECUTION on the imported DD still has gaps (nested UDF calls, EXECUTE IMMEDIATE, cursors) — that is the mini-interpreter, a separate workstream; this commit makes the catalog and parsed bodies byte-faithful so the interpreter work starts from correct text. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent ef61ef1 commit 18be7d3

4 files changed

Lines changed: 238 additions & 56 deletions

File tree

‎src/abi/ace_exports.cpp‎

Lines changed: 57 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -17989,6 +17989,23 @@ build_system_table(Connection* c, std::string sys_name,
1798917989
return build_memory_result("system." + sys_name, cols, rows);
1799017990
};
1799117991

17992+
// RCB 07/16/2026: size a text column to its longest actual value instead
17993+
// of a fixed CHAR width. system.{storedprocedures,functions} declared
17994+
// their body columns as C(255), so multi-KB SQL bodies (pmsys:
17995+
// sp_SaveIntoAuditLog is 5359 bytes) came back as a 255-byte prefix —
17996+
// silent data loss in the catalog even though the DD and the execution
17997+
// path hold the full text. Memory-table records cap at 64 KB total, so
17998+
// clamp each column defensively below that.
17999+
auto fit_width = [](const std::vector<std::vector<std::string>>& rows,
18000+
std::size_t col_idx, std::size_t min_w)
18001+
-> std::uint16_t {
18002+
std::size_t w = min_w;
18003+
for (const auto& r : rows)
18004+
if (col_idx < r.size() && r[col_idx].size() > w)
18005+
w = r[col_idx].size();
18006+
return static_cast<std::uint16_t>(std::min<std::size_t>(w, 60000));
18007+
};
18008+
1799218009
if (sys_name == "tables") {
1799318010
// Column set matches SAP ADS system.tables for compatibility.
1799418011
const std::vector<Col> cols = {
@@ -18759,18 +18776,6 @@ build_system_table(Connection* c, std::string sys_name,
1875918776
if (sys_name == "triggers") {
1876018777
// TIMING decodes SAP binary timing byte: 1=BEFORE 2=INSTEAD OF 4=AFTER
1876118778
// EVENT_MASK is the SAP event type byte: 1=INSERT 2=UPDATE 3=DELETE
18762-
const std::vector<Col> cols = {
18763-
{"TRIG_NAME", 'C', 200, 0},
18764-
{"TABLE_NAME", 'C', 200, 0},
18765-
{"EVENT_MASK", 'N', 10, 0},
18766-
{"TIMING", 'C', 15, 0},
18767-
{"EVENT", 'C', 20, 0},
18768-
{"CONTAINER", 'C', 4096, 0},
18769-
{"PROC", 'C', 200, 0},
18770-
{"PRIORITY", 'N', 10, 0},
18771-
{"ENABLED", 'L', 1, 0},
18772-
{"TRIG_OPTIONS", 'N', 10, 0},
18773-
};
1877418779
auto timing_str = [](std::uint32_t t) -> std::string {
1877518780
if (t == 1) return "BEFORE";
1877618781
if (t == 2) return "INSTEAD OF";
@@ -18802,53 +18807,72 @@ build_system_table(Connection* c, std::string sys_name,
1880218807
for (const auto& kv : dd->triggers())
1880318808
add_trigger_row(kv.second);
1880418809
}
18810+
// RCB 07/16/2026: CONTAINER sized to content (was fixed C(4096) —
18811+
// enough for pmsys but silently truncates any larger trigger body).
18812+
const std::vector<Col> cols = {
18813+
{"TRIG_NAME", 'C', 200, 0},
18814+
{"TABLE_NAME", 'C', 200, 0},
18815+
{"EVENT_MASK", 'N', 10, 0},
18816+
{"TIMING", 'C', 15, 0},
18817+
{"EVENT", 'C', 20, 0},
18818+
{"CONTAINER", 'C', fit_width(rows, 5, 4096), 0},
18819+
{"PROC", 'C', 200, 0},
18820+
{"PRIORITY", 'N', 10, 0},
18821+
{"ENABLED", 'L', 1, 0},
18822+
{"TRIG_OPTIONS", 'N', 10, 0},
18823+
};
1880518824
return build(cols, rows);
1880618825
}
1880718826
if (sys_name == "storedprocedures") {
18808-
const std::vector<Col> cols = {
18809-
{"PROC_NAME", 'C', 200, 0},
18810-
{"CONTAINER", 'C', 250, 0},
18811-
{"PROCEDURE", 'C', 255, 0},
18812-
{"INPUT", 'C', 250, 0},
18813-
{"OUTPUT", 'C', 250, 0},
18814-
};
1881518827
std::vector<std::vector<std::string>> rows;
1881618828
for (const auto& kv : dd->procs()) {
1881718829
const auto& e = kv.second;
1881818830
rows.push_back({e.name, e.container, e.procedure,
1881918831
e.input_params, e.output_params});
1882018832
}
18833+
// RCB 07/16/2026: rows built first so the text columns can be sized
18834+
// to the real content — a fixed C(255) truncated multi-KB SQL bodies.
18835+
const std::vector<Col> cols = {
18836+
{"PROC_NAME", 'C', 200, 0},
18837+
{"CONTAINER", 'C', fit_width(rows, 1, 250), 0},
18838+
{"PROCEDURE", 'C', fit_width(rows, 2, 255), 0},
18839+
{"INPUT", 'C', fit_width(rows, 3, 250), 0},
18840+
{"OUTPUT", 'C', fit_width(rows, 4, 250), 0},
18841+
};
1882118842
return build(cols, rows);
1882218843
}
1882318844
if (sys_name == "functions") {
18824-
// Column names capped at 10 chars (DBF field descriptor limit in build()).
18825-
const std::vector<Col> cols = {
18826-
{"FUNC_NAME", 'C', 200, 0},
18827-
{"CONTAINER", 'C', 250, 0},
18828-
{"RET_TYPE", 'C', 50, 0},
18829-
{"IN_PARAMS", 'C', 200, 0},
18830-
{"FUNC_BODY", 'C', 255, 0},
18831-
{"COMMENT", 'C', 200, 0},
18832-
};
1883318845
std::vector<std::vector<std::string>> rows;
1883418846
for (const auto& kv : dd->functions()) {
1883518847
const auto& e = kv.second;
1883618848
rows.push_back({e.name, e.container, e.return_type,
1883718849
e.input_params, e.implementation, e.comment});
1883818850
}
18851+
// RCB 07/16/2026: FUNC_BODY sized to content — C(255) truncated real
18852+
// UDF bodies (pmsys NewSeqKey is 1517 bytes).
18853+
const std::vector<Col> cols = {
18854+
{"FUNC_NAME", 'C', 200, 0},
18855+
{"CONTAINER", 'C', fit_width(rows, 1, 250), 0},
18856+
{"RET_TYPE", 'C', 50, 0},
18857+
{"IN_PARAMS", 'C', fit_width(rows, 3, 200), 0},
18858+
{"FUNC_BODY", 'C', fit_width(rows, 4, 255), 0},
18859+
{"COMMENT", 'C', 200, 0},
18860+
};
1883918861
return build(cols, rows);
1884018862
}
1884118863
if (sys_name == "views") {
18842-
const std::vector<Col> cols = {
18843-
{"VIEW_NAME", 'C', 200, 0},
18844-
{"VIEW_SQL", 'C', 250, 0},
18845-
{"COMMENT", 'C', 200, 0},
18846-
};
1884718864
std::vector<std::vector<std::string>> rows;
1884818865
for (const auto& kv : dd->views()) {
1884918866
const auto& e = kv.second;
1885018867
rows.push_back({e.name, e.sql, e.comment});
1885118868
}
18869+
// RCB 07/16/2026: VIEW_SQL sized to content (same C(255)-class
18870+
// truncation as storedprocedures/functions).
18871+
const std::vector<Col> cols = {
18872+
{"VIEW_NAME", 'C', 200, 0},
18873+
{"VIEW_SQL", 'C', fit_width(rows, 1, 250), 0},
18874+
{"COMMENT", 'C', 200, 0},
18875+
};
1885218876
return build(cols, rows);
1885318877
}
1885418878
if (sys_name == "dictionary") {

‎src/engine/data_dict.cpp‎

Lines changed: 68 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -876,32 +876,77 @@ util::Result<void> DataDict::load_add_binary_(const std::string& buf) {
876876
}
877877
}
878878
}
879-
for (; pos + 1 < PL; ++pos) {
880-
if (static_cast<uint8_t>(buf[PS + pos]) == 0x0D &&
881-
static_cast<uint8_t>(buf[PS + pos+1]) == 0x0A) break;
879+
// RCB 07/16/2026: after the output-params lstr the SAP
880+
// layout is STRUCTURED (verified by hex-dump of pmsys.add):
881+
// [le16 invoke_option][le32 reserved][le16 body_len][body…]
882+
// body_len counts the WHOLE body; the inline zone holds the
883+
// first PL-pos bytes and the .am continuation the rest.
884+
// The old code scanned forward to the first CRLF to find
885+
// the body start — but the first CRLF is the END of the
886+
// body's FIRST LINE, so every proc lost its opening line
887+
// (e.g. sp_GetPhysicalPath lost "DECLARE @sql STRING;").
888+
bool structured = false;
889+
if (pos + 8 <= PL) {
890+
uint16_t invoke = le16(buf, PS + pos);
891+
uint16_t blen = le16(buf, PS + pos + 6);
892+
if (invoke < 0x100 && blen > 0 && blen != 0xFFFFu) {
893+
std::size_t bpos = pos + 8;
894+
std::size_t inline_n = std::min<std::size_t>(
895+
blen, PL - bpos);
896+
e.procedure = buf.substr(PS + bpos, inline_n);
897+
std::size_t remaining = blen - inline_n;
898+
if (remaining > 0) {
899+
// Continuation: exactly `remaining` bytes from
900+
// the .am block — no printable-scan heuristics
901+
// needed because the length is authoritative.
902+
auto am_block =
903+
static_cast<uint32_t>(rec.more_property[0])
904+
| (static_cast<uint32_t>(rec.more_property[1]) << 8)
905+
| (static_cast<uint32_t>(rec.more_property[2]) << 16)
906+
| (static_cast<uint32_t>(rec.more_property[3]) << 24);
907+
std::size_t am_off =
908+
static_cast<std::size_t>(am_block) * 8;
909+
if (am_block > 0 && am_off < am_buf.size()) {
910+
std::size_t take = std::min<std::size_t>(
911+
remaining, am_buf.size() - am_off);
912+
e.procedure += am_buf.substr(am_off, take);
913+
}
914+
}
915+
auto lc = e.procedure.find_last_not_of(" \t\r\n\0",
916+
std::string::npos, 5);
917+
if (lc != std::string::npos) e.procedure.resize(lc + 1);
918+
else e.procedure.clear();
919+
structured = true;
920+
}
882921
}
883-
if (pos + 1 < PL) {
884-
std::size_t end = PL;
885-
for (std::size_t j = pos; j < PL; ++j)
886-
if (buf[PS + j] == '\0') { end = j; break; }
887-
std::string body = buf.substr(PS + pos, end - pos);
888-
auto f = body.find_first_not_of(" \t\r\n");
889-
if (f != std::string::npos) body = body.substr(f);
890-
auto l = body.find_last_not_of(" \t\r\n");
891-
if (l != std::string::npos) body.resize(l + 1);
892-
e.procedure = std::move(body);
922+
if (!structured) {
923+
// Legacy fallback: CRLF scan (pre-structured layouts).
924+
for (; pos + 1 < PL; ++pos) {
925+
if (static_cast<uint8_t>(buf[PS + pos]) == 0x0D &&
926+
static_cast<uint8_t>(buf[PS + pos+1]) == 0x0A) break;
927+
}
928+
if (pos + 1 < PL) {
929+
std::size_t end = PL;
930+
for (std::size_t j = pos; j < PL; ++j)
931+
if (buf[PS + j] == '\0') { end = j; break; }
932+
std::string body = buf.substr(PS + pos, end - pos);
933+
auto f = body.find_first_not_of(" \t\r\n");
934+
if (f != std::string::npos) body = body.substr(f);
935+
auto l = body.find_last_not_of(" \t\r\n");
936+
if (l != std::string::npos) body.resize(l + 1);
937+
e.procedure = std::move(body);
938+
}
939+
// SAP .am body is a NUL-terminated C string; strip from
940+
// first NUL.
941+
const auto body_end = e.procedure.size();
942+
append_am(e.procedure, rec.more_property);
943+
auto nul = e.procedure.find('\0', body_end);
944+
if (nul != std::string::npos) e.procedure.resize(nul);
945+
auto lc = e.procedure.find_last_not_of(" \t\r\n");
946+
if (lc != std::string::npos) e.procedure.resize(lc + 1);
947+
else e.procedure.clear();
893948
}
894949
}
895-
// SAP .am body is a NUL-terminated C string; strip from first NUL.
896-
{
897-
const auto body_end = e.procedure.size();
898-
append_am(e.procedure, rec.more_property);
899-
auto nul = e.procedure.find('\0', body_end);
900-
if (nul != std::string::npos) e.procedure.resize(nul);
901-
auto lc = e.procedure.find_last_not_of(" \t\r\n");
902-
if (lc != std::string::npos) e.procedure.resize(lc + 1);
903-
else e.procedure.clear();
904-
}
905950
}
906951
procs_[e.name] = std::move(e);
907952

‎tests/unit/abi_dd_proc_view_test.cpp‎

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -254,3 +254,41 @@ TEST_CASE("AdsDDDropView — unknown view returns error") {
254254
UNSIGNED8 name[32] = "no_such_view";
255255
CHECK(AdsDDDropView(f.hConn, name) != 0);
256256
}
257+
258+
// RCB 07/16/2026: system.storedprocedures declared its PROCEDURE column as a
259+
// fixed CHAR(255), so any SQL body longer than 255 bytes came back as a
260+
// truncated prefix — silent data loss in the catalog even though the DD and
261+
// the execution path hold the full text (pmsys sp_SaveIntoAuditLog is 5359
262+
// bytes). The column is now sized to the longest actual value. Same fix
263+
// applies to system.functions FUNC_BODY, system.triggers CONTAINER and
264+
// system.views VIEW_SQL.
265+
TEST_CASE("system.storedprocedures returns >255-byte bodies untruncated") {
266+
PvFixture f;
267+
268+
std::string body = "DECLARE @pad STRING;\r\n";
269+
while (body.size() < 1200)
270+
body += " @pad = 'padding padding padding padding';\r\n";
271+
body += " INSERT INTO __output VALUES ('END_MARKER_OK');";
272+
273+
UNSIGNED8 name[32] = "sp_longbody";
274+
REQUIRE(AdsDDCreateProcedure(f.hConn, name, nullptr,
275+
(UNSIGNED8*)body.c_str(), 0, nullptr, nullptr, nullptr) == 0);
276+
277+
ADSHANDLE hStmt = 0, hCur = 0;
278+
REQUIRE(AdsCreateSQLStatement(f.hConn, &hStmt) == 0);
279+
UNSIGNED8 sql[64] = "SELECT * FROM system.storedprocedures";
280+
REQUIRE(AdsExecuteSQLDirect(hStmt, sql, &hCur) == 0);
281+
282+
std::vector<UNSIGNED8> val(16384, 0);
283+
UNSIGNED32 vlen = static_cast<UNSIGNED32>(val.size() - 1);
284+
UNSIGNED8 fld[16] = "PROCEDURE";
285+
REQUIRE(AdsGetField(hCur, fld, val.data(), &vlen, 0) == 0);
286+
std::string got(reinterpret_cast<char*>(val.data()), vlen);
287+
while (!got.empty() && got.back() == ' ') got.pop_back();
288+
289+
CHECK(got.size() > 1200); // not the 255-byte prefix
290+
CHECK(got.find("END_MARKER_OK") != std::string::npos);
291+
292+
AdsCloseTable(hCur);
293+
AdsCloseSQLStatement(hStmt);
294+
}

‎tests/unit/data_dict_test.cpp‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,3 +323,78 @@ TEST_CASE("DataDict remove_table + reopen no longer has the alias") {
323323
}
324324
fs::remove(p);
325325
}
326+
327+
// RCB 07/16/2026: SAP-binary Procedure records carry the SQL body in a
328+
// STRUCTURED layout after the params: [le16 invoke_option][le32 reserved]
329+
// [le16 body_len][body...]. The old parser scanned forward to the first
330+
// CRLF to locate the body — but the first CRLF is the END of the body's
331+
// FIRST LINE, so every imported proc silently lost its opening statement
332+
// (pmsys sp_GetPhysicalPath lost "DECLARE @sql STRING;", breaking it).
333+
// This fabricates a minimal SAP-binary .add with one Procedure record in
334+
// that layout and asserts the body round-trips VERBATIM, first line included.
335+
TEST_CASE("DataDict SAP-binary proc parse keeps the body's first line") {
336+
auto p = fs::temp_directory_path() / "openads_dd_sap_procbody.add";
337+
fs::remove(p);
338+
fs::remove(fs::temp_directory_path() / "openads_dd_sap_procbody.am");
339+
340+
const std::string body =
341+
" DECLARE @first STRING;\r\n"
342+
" @first = 'line one must survive';\r\n"
343+
" INSERT INTO __output VALUES (@first);";
344+
const std::string in_params = "a,CHAR,5;";
345+
346+
constexpr std::size_t kHdr = 2200, kRec = 524;
347+
std::string buf(kHdr + kRec, '\0');
348+
std::memcpy(&buf[0], "ADS Data Dictionary", 19);
349+
auto p32at = [&](std::size_t off, std::uint32_t v) {
350+
buf[off] = static_cast<char>( v & 0xFF);
351+
buf[off + 1] = static_cast<char>((v >> 8) & 0xFF);
352+
buf[off + 2] = static_cast<char>((v >> 16) & 0xFF);
353+
buf[off + 3] = static_cast<char>((v >> 24) & 0xFF);
354+
};
355+
p32at(0x20, kHdr);
356+
p32at(0x24, kRec);
357+
358+
std::size_t base = kHdr;
359+
buf[base] = 0x04; // active
360+
p32at(base + 5, 1); // obj_id
361+
std::memset(&buf[base + 13], ' ', 10);
362+
std::memcpy(&buf[base + 13], "Procedure", 9); // obj_type
363+
std::memset(&buf[base + 23], ' ', 200);
364+
std::memcpy(&buf[base + 23], "sp_firstline", 12); // obj_name
365+
366+
// Property zone: [in_params\0][6x 0xFF][le16 invoke=4][le32 reserved]
367+
// [le16 body_len][body]
368+
const std::uint16_t plen =
369+
static_cast<std::uint16_t>(in_params.size() + 1);
370+
buf[base + 223] = static_cast<char>(plen & 0xFF);
371+
buf[base + 224] = static_cast<char>((plen >> 8) & 0xFF);
372+
std::size_t pp = base + 225;
373+
std::memcpy(&buf[pp], in_params.data(), in_params.size());
374+
pp += plen; // includes the NUL
375+
for (int i = 0; i < 6; ++i) buf[pp++] = static_cast<char>(0xFF);
376+
buf[pp++] = 0x04; buf[pp++] = 0x00; // invoke_option
377+
pp += 4; // reserved le32 = 0..
378+
buf[pp - 4] = 0x04; // ..matches SAP dumps
379+
const std::uint16_t blen = static_cast<std::uint16_t>(body.size());
380+
buf[pp++] = static_cast<char>(blen & 0xFF);
381+
buf[pp++] = static_cast<char>((blen >> 8) & 0xFF);
382+
std::memcpy(&buf[pp], body.data(), body.size());
383+
// more_property [498..506] stays zero: body is fully inline.
384+
385+
std::ofstream(p, std::ios::binary).write(buf.data(),
386+
static_cast<std::streamsize>(buf.size()));
387+
388+
auto opened = DataDict::open(p.string());
389+
REQUIRE(opened.has_value());
390+
DataDict dd = std::move(opened).value();
391+
REQUIRE(dd.has_proc("sp_firstline"));
392+
const auto& e = dd.procs().at("sp_firstline");
393+
CHECK(e.input_params == in_params);
394+
// The regression: the old CRLF scan dropped everything before the first
395+
// CRLF, i.e. the whole first line.
396+
CHECK(e.procedure == body);
397+
CHECK(e.procedure.find("DECLARE @first") != std::string::npos);
398+
399+
fs::remove(p);
400+
}

0 commit comments

Comments
 (0)