Skip to content

Commit 3a98211

Browse files
committed
fix: address automated review findings in PostgreSQL parsing and execution
Avoid recursive expression allocation while probing a generic typed literal: scan modifier tokens first and validate expressions only when a following string confirms a type prefix. Ordinary nested calls now fit a bounded arena instead of exhausting it around fourteen levels. Preserve PostgreSQL operator precedence while allowing exact binary concatenation through the local evaluator. Parse MySQL ROWS/RANGE frames while keeping GROUPS/EXCLUDE specific to PostgreSQL and unsupported frame execution explicitly guarded. Make emission tolerate missing children after public subtree removal. Store aliases as undelimited token values with retained source spelling and delimiter flags, including RETURNING and JOIN USING aliases. Preserve cloning, parameterization and edited-identifier emission. Keep ONLY/inheritance target names directly accessible under table references, restoring UPDATE/DELETE metadata while retaining planner and parameterizer rejection of those features. Accept PostgreSQL ColId names in COPY and transaction utilities, and reject PREPARED after END/ABORT aliases. Dispatch RELEASE SAVEPOINT and ROLLBACK TO through the correct transaction-manager operations, decoding quoted savepoint names and folding unquoted PostgreSQL names. Require complete successful parsing before transaction effects; reject unimplemented transaction options. Preserve simple MySQL controls by consuming optional WORK and named SAVEPOINT operands, and reject missing START TRANSACTION/SAVEPOINT operands. Keep PostgreSQL LIMIT ALL/NULL unbounded and OFFSET NULL at zero. Reject local limit expressions, negative counts and overflowing literals that cannot be correctly evaluated, rather than extracting digits or silently treating them as zero. Reject inputs larger than UINT32_MAX before scanning so public source spans and offsets cannot wrap, without changing the 48-byte AST layout. Correct stale README layout/test counts and generic-template guidance. Document source/value mutation rules, existing identifier-escape representation, input limits and local execution behavior. Do not apply the disputed DEFAULT-predicate suggestion: parenthesized full expressions remain valid PostgreSQL syntax. Unsupported DDL continues to report errors; this fix does not add an opaque fallback or change the classification-status API. Validation: - Red/green regressions for all ten confirmed functional findings and oversized input handling; independent review fixed a MySQL transaction compatibility issue before completion. - Forced full build, then rebuild of every parameterizer-dependent test after final integration correction: 1,514 passed, 37 external-database tests skipped; corpus-test executable built. - ASan/UBSan and leak detection: 20 new tests and 32 AST regression tests passed. - Compatibility harness: 87 tests passed. Pinned original-source corpus gained three deeply supported statements with zero lost cases (49,848/51,415). - All three additions and 653 affected existing alias/DML corpus statements reconstruct to equivalent PostgreSQL raw ASTs, stripping only source positions. - Existing PostgreSQL parser benchmark smoke ran; no timing claims or benchmark sources, reports, or benchmark-only README/.gitignore changes are committed. The correctness snapshot follows in a separate commit.
1 parent 9057d79 commit 3a98211

23 files changed

Lines changed: 785 additions & 105 deletions

‎CLAUDE.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ Full benchmark + corpus report: `bash scripts/run_benchmarks.sh report.md`.
5050

5151
### Namespace
5252

53-
Everything parser-side is in `namespace sql_parser`. All templates are parameterized on `Dialect D`.
53+
Everything parser-side is in `namespace sql_parser`. Templates with dialect-specific behavior use `Dialect D`; dialect-independent generic helpers are exempt.
5454

5555
### Adding a new deep parser
5656

@@ -242,7 +242,7 @@ Subclass `TransactionManager`. Provide `begin()/commit()/rollback()` semantics;
242242

243243
## Tests
244244

245-
Google Test. 1,160 tests across 50 test files. Validated against 86K+ external queries from 9 corpora (99.92% fully parsed).
245+
Google Test. Run `make test` for the current test totals. Validated against 86K+ external queries from 9 corpora (99.92% fully parsed).
246246

247247
Run a single test: `./run_tests --gtest_filter="*WindowFunc*"`.
248248

‎Makefile‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,10 @@ TEST_SRCS = $(TEST_DIR)/test_main.cpp \
6363
$(TEST_DIR)/test_pg_foreign_ddl.cpp \
6464
$(TEST_DIR)/test_pg_table_constraints.cpp \
6565
$(TEST_DIR)/test_pg_maintenance.cpp \
66+
$(TEST_DIR)/test_review_expressions.cpp \
67+
$(TEST_DIR)/test_review_ast.cpp \
68+
$(TEST_DIR)/test_review_transactions.cpp \
69+
$(TEST_DIR)/test_review_runtime.cpp \
6670
$(TEST_DIR)/test_pg_admin_commands.cpp \
6771
$(TEST_DIR)/test_pg_session_commands.cpp \
6872
$(TEST_DIR)/test_pg_dml_extensions.cpp \

‎README.md‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ coverage and remaining syntax gaps.
6161
### Build
6262

6363
```bash
64-
make all # Build library + run all 1,160 tests
64+
make all # Build library + run the full test suite
6565
make lib # Just the static library
6666
make bench # Benchmarks (-O2); use scripts/run_benchmarks.sh for -O3 report
6767
make build-sqlengine # Interactive SQL CLI
@@ -264,7 +264,7 @@ auto report = recovery.recover();
264264
│ ▼
265265
│ ┌──────────────┐
266266
│ │ Full AST in │
267-
│ │ arena (32 B │
267+
│ │ arena (48 B │
268268
│ │ nodes) │
269269
│ └──────┬───────┘
270270
│ │
@@ -403,11 +403,11 @@ owning copies, subtree replacement and context-aware parameterization.
403403
| `engine_stress_test` | `make engine-stress` | Direct-API engine stress test |
404404
| `bench_distributed` | `make bench-distributed` | Distributed query benchmark + pipeline breakdown |
405405
| `run_bench` | `make bench` | Google-Benchmark micro-benchmarks |
406-
| `run_tests` | `make test` | 1,160 Google-Test unit tests |
406+
| `run_tests` | `make test` | Google-Test unit tests |
407407
408408
## Testing
409409
410-
- **1,160 unit tests** (Google Test, 50 test files)
410+
- **Unit tests** (Google Test; run `make test` for current totals)
411411
- **86,467 external corpus queries** validated via `scripts/run_benchmarks.sh`
412412
- **CI** — runs unit tests + a corpus-subset on every push/PR
413413
- **Integration tests** (MySQL/PgSQL) auto-skip when no live backend is reachable
@@ -448,7 +448,7 @@ src/sql_engine/ function_registry.cpp, in_memory_catalog.cpp,
448448
pgsql_remote_executor.cpp, multi_remote_executor.cpp
449449
450450
tools/ sqlengine.cpp mysql_server.cpp engine_stress_test.cpp bench_distributed.cpp
451-
tests/ 1,160 Google-Test tests across 50 files
451+
tests/ Google-Test unit and regression tests
452452
bench/ bench_parser.cpp bench_engine.cpp bench_comparison.cpp
453453
scripts/ run_benchmarks.sh run_comparison.sh
454454
docs/benchmarks/ latest.md comparison.md distributed_comparison.md REPRODUCING.md

‎docs/ast-utilities.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,19 @@ tree. Validation happens before mutation; errors leave input links unchanged.
5151
Nodes are never freed individually, and callers retain responsibility for the
5252
lifetimes of every participating arena.
5353

54+
Identifier and alias values omit their surrounding delimiters; original spelling
55+
is retained separately in `source()`. Token text can still contain doubled
56+
identifier quotes; it is not a fully decoded SQL identifier. When editing a
57+
parsed value for re-emission,
58+
also clear its retained spelling with `node.set_source({})` (or replace it with
59+
matching source text). `set_value()` alone deliberately leaves `source()` intact.
60+
The emitter uses identifier flags to quote a changed delimited identifier when
61+
no retained spelling remains.
62+
63+
Removing a required child can produce an incomplete SQL tree. Emission tolerates
64+
missing children, but callers must validate edited SQL before using it; tree
65+
link validation does not establish SQL grammar validity.
66+
5467
Failed operations can consume arena space; they do not reset or roll back an
5568
arena that might contain other live nodes. Temporary STL allocation failures use
5669
the usual C++ exception behavior.

‎docs/postgresql-analysis-features.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,24 @@ for name resolution, types, privileges and semantic checks. Corpus acceptance
215215
alone does not establish grammar parity or round-trip equivalence for every
216216
accepted statement.
217217

218+
## Local execution and review corrections
219+
220+
PostgreSQL `LIMIT ALL` and `LIMIT NULL` preserve an unlimited local row count;
221+
`OFFSET NULL` is zero. The local planner rejects limit/offset expressions it
222+
cannot evaluate, including dynamic, negative and overflowing counts, rather
223+
than treating their text as a number. PostgreSQL binary `||` uses the existing
224+
concatenation evaluator while retaining PostgreSQL operator precedence.
225+
MySQL ROWS/RANGE frames are parsed and emitted, but remain unsupported by the
226+
local planner. Nested ordinary function calls no longer allocate discarded
227+
expression trees during speculative type-prefix parsing.
228+
229+
Session transaction dispatch requires a successful, complete parse before any
230+
transaction-manager call. PostgreSQL SAVEPOINT, RELEASE and ROLLBACK TO use the
231+
named operand, including identifier case folding and quoted-name decoding.
232+
Prepared transactions, chaining and other options the local manager cannot
233+
execute are rejected. Basic MySQL controls retain optional WORK and named
234+
SAVEPOINT forms. Parsing transaction syntax alone does not execute it.
235+
218236
## Multiple statements
219237

220238
```cpp
@@ -232,6 +250,10 @@ All returned ASTs remain alive until the next `parse`, `parse_all`, or `reset`
232250
on that parser. Input SQL must remain alive too. Use `clone_ast` to retain a tree
233251
independently. The result vector owns records, not the ASTs or source bytes.
234252

253+
Both `parse` and `parse_all` reject input longer than `UINT32_MAX` bytes before
254+
reading the buffer, preserving the 32-bit source-location contract. An oversized
255+
batch returns one ERROR record with an empty source span and a diagnostic.
256+
235257
The tokenizer identifies boundaries, so semicolons in strings, quoted
236258
identifiers and comments do not split statements. Empty statements/comments are
237259
skipped. Malformed statements remain in the result; scanning continues at the

‎include/sql_engine/plan_builder.h‎

Lines changed: 43 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "sql_parser/arena.h"
2727
#include <cstring>
2828
#include <vector>
29+
#include <limits>
2930

3031
namespace sql_engine {
3132

@@ -122,6 +123,14 @@ class PlanBuilder {
122123
case NodeType::NODE_PG_DDL_SYNTAX:
123124
return true;
124125
// PG_GAPS_QUERY_GUARD
126+
case NodeType::NODE_LIMIT_CLAUSE: {
127+
int64_t value;
128+
const auto* count = node->first_child;
129+
if (!read_limit_literal(count, -1, value)) return true;
130+
const auto* offset = count->next_sibling;
131+
if (offset && (!read_limit_literal(offset, 0, value) || offset->next_sibling)) return true;
132+
break;
133+
}
125134
case NodeType::NODE_TABLE_REF:
126135
if (node->flags & (sql_parser::FLAG_TABLE_ONLY | sql_parser::FLAG_TABLE_INHERIT)) return true;
127136
break;
@@ -481,41 +490,44 @@ class PlanBuilder {
481490
return current;
482491
}
483492

484-
// Build a Limit plan node from LIMIT clause AST
485-
PlanNode* build_limit_node(const sql_parser::AstNode* limit_clause, PlanNode* child) {
486-
PlanNode* limit = make_plan_node(arena_, PlanNodeType::LIMIT);
487-
limit->limit.count = -1;
488-
limit->limit.offset = 0;
489-
490-
const sql_parser::AstNode* first = limit_clause->first_child;
491-
if (first) {
492-
// Parse the literal count value
493-
limit->limit.count = parse_int_literal(first);
494-
495-
const sql_parser::AstNode* second = first->next_sibling;
496-
if (second) {
497-
// LIMIT count OFFSET offset_val or LIMIT offset, count (MySQL)
498-
// In the AST, second child is always the offset value
499-
limit->limit.offset = parse_int_literal(second);
493+
// Local LIMIT execution supports integer constants and PostgreSQL NULL
494+
// (including normalized ALL). Other expressions need runtime evaluation.
495+
static bool read_limit_literal(const sql_parser::AstNode* node,
496+
int64_t null_value, int64_t& value) {
497+
if (!node) return false;
498+
if constexpr (D == sql_parser::Dialect::PostgreSQL) {
499+
if (node->type == sql_parser::NodeType::NODE_LITERAL_NULL) {
500+
value = null_value;
501+
return true;
500502
}
501503
}
502-
limit->left = child;
503-
return limit;
504+
if (node->type != sql_parser::NodeType::NODE_LITERAL_INT) return false;
505+
const auto text = node->value();
506+
if (text.empty()) return false;
507+
value = 0;
508+
for (uint32_t i = 0; i < text.len; ++i) {
509+
const char c = text.ptr[i];
510+
if (c < '0' || c > '9') return false;
511+
const int digit = c - '0';
512+
if (value > (std::numeric_limits<int64_t>::max() - digit) / 10) return false;
513+
value = value * 10 + digit;
514+
}
515+
return true;
504516
}
505517

506-
// Parse an integer literal from an AST node
507-
static int64_t parse_int_literal(const sql_parser::AstNode* node) {
508-
if (!node) return 0;
509-
sql_parser::StringRef val = node->value();
510-
if (val.len == 0) return 0;
511-
int64_t result = 0;
512-
for (uint32_t i = 0; i < val.len; ++i) {
513-
char c = val.ptr[i];
514-
if (c >= '0' && c <= '9') {
515-
result = result * 10 + (c - '0');
516-
}
517-
}
518-
return result;
518+
// Build a Limit plan node without converting unsupported expressions to zero.
519+
PlanNode* build_limit_node(const sql_parser::AstNode* limit_clause, PlanNode* child) {
520+
int64_t count, offset = 0;
521+
const auto* first = limit_clause->first_child;
522+
if (!read_limit_literal(first, -1, count)) return nullptr;
523+
const auto* second = first->next_sibling;
524+
if (second && (!read_limit_literal(second, 0, offset) || second->next_sibling)) return nullptr;
525+
PlanNode* limit = make_plan_node(arena_, PlanNodeType::LIMIT);
526+
if (!limit) return nullptr;
527+
limit->limit.count = count;
528+
limit->limit.offset = offset;
529+
limit->left = child;
530+
return limit;
519531
}
520532

521533
// Build plan for CTE (WITH clause)

‎include/sql_engine/session.h‎

Lines changed: 80 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -156,42 +156,114 @@ class Session {
156156
parser_.reset();
157157
auto pr = parser_.parse(sql, len);
158158

159-
// Check for transaction control statements first (parser may not
160-
// produce an AST for these — they are classified by stmt_type).
159+
auto transaction_error = [](const char* message) {
160+
DmlResult dr;
161+
dr.error_message = message;
162+
return dr;
163+
};
164+
auto transaction_name = [](const sql_parser::AstNode* operand) {
165+
std::string name;
166+
const bool quoted = (operand->flags & sql_parser::FLAG_IDENT_DELIMITED) != 0;
167+
auto value = operand->value();
168+
name.reserve(value.len);
169+
for (uint32_t i = 0; i < value.len; ++i) {
170+
char c = value.ptr[i];
171+
if (quoted && c == '"' && i + 1 < value.len && value.ptr[i + 1] == '"') ++i;
172+
else if (!quoted && c >= 'A' && c <= 'Z') c = static_cast<char>(c - 'A' + 'a');
173+
name.push_back(c);
174+
}
175+
return name;
176+
};
177+
if (pr.status != sql_parser::ParseResult::OK || !pr.full_input)
178+
return transaction_error("parse error");
179+
180+
// PostgreSQL transaction controls retain their operands and options
181+
// in the AST. MySQL's legacy transaction extractor has no AST.
182+
if constexpr (D == sql_parser::Dialect::PostgreSQL) {
183+
if ((pr.stmt_type == sql_parser::StmtType::BEGIN ||
184+
pr.stmt_type == sql_parser::StmtType::START_TRANSACTION ||
185+
pr.stmt_type == sql_parser::StmtType::COMMIT ||
186+
pr.stmt_type == sql_parser::StmtType::ROLLBACK ||
187+
pr.stmt_type == sql_parser::StmtType::SAVEPOINT ||
188+
pr.stmt_type == sql_parser::StmtType::RELEASE_SAVEPOINT) &&
189+
(!pr.ast || pr.ast->type != sql_parser::NodeType::NODE_TRANSACTION_STMT))
190+
return transaction_error("parse error");
191+
}
192+
161193
switch (pr.stmt_type) {
162194
case sql_parser::StmtType::BEGIN:
163195
case sql_parser::StmtType::START_TRANSACTION: {
196+
if constexpr (D == sql_parser::Dialect::PostgreSQL) {
197+
if (pr.ast->first_child)
198+
return transaction_error("unsupported transaction options");
199+
}
164200
DmlResult dr;
165201
dr.success = txn_mgr_.begin();
166202
if (!dr.success) dr.error_message = "BEGIN failed";
167203
return dr;
168204
}
169205
case sql_parser::StmtType::COMMIT: {
206+
if constexpr (D == sql_parser::Dialect::PostgreSQL) {
207+
if (pr.ast->first_child)
208+
return transaction_error("unsupported transaction options");
209+
}
170210
DmlResult dr;
171211
dr.success = txn_mgr_.commit();
172212
if (!dr.success) dr.error_message = "COMMIT failed";
173213
return dr;
174214
}
175215
case sql_parser::StmtType::ROLLBACK: {
216+
if constexpr (D == sql_parser::Dialect::PostgreSQL) {
217+
const auto* option = pr.ast->first_child;
218+
if (option) {
219+
const auto* name = option->next_sibling;
220+
if (option->type != sql_parser::NodeType::NODE_TRANSACTION_OPTION ||
221+
option->value() != sql_parser::StringRef{"TO SAVEPOINT", 12} ||
222+
!name || name->type != sql_parser::NodeType::NODE_IDENTIFIER ||
223+
name->next_sibling)
224+
return transaction_error("unsupported transaction options");
225+
std::string savepoint = transaction_name(name);
226+
DmlResult dr;
227+
dr.success = txn_mgr_.rollback_to(savepoint.c_str());
228+
if (!dr.success) dr.error_message = "ROLLBACK TO SAVEPOINT failed";
229+
return dr;
230+
}
231+
}
176232
DmlResult dr;
177233
dr.success = txn_mgr_.rollback();
178234
if (!dr.success) dr.error_message = "ROLLBACK failed";
179235
return dr;
180236
}
181237
case sql_parser::StmtType::SAVEPOINT: {
182238
DmlResult dr;
183-
// The savepoint name is in the AST value or table_name
184239
std::string name;
185-
if (pr.table_name.ptr && pr.table_name.len > 0)
240+
if constexpr (D == sql_parser::Dialect::PostgreSQL) {
241+
const auto* operand = pr.ast->first_child;
242+
if (!operand || operand->type != sql_parser::NodeType::NODE_IDENTIFIER ||
243+
operand->next_sibling)
244+
return transaction_error("parse error");
245+
name = transaction_name(operand);
246+
} else if (pr.table_name.ptr && pr.table_name.len > 0)
186247
name.assign(pr.table_name.ptr, pr.table_name.len);
187-
else if (pr.ast && pr.ast->value_ptr && pr.ast->value_len > 0)
188-
name.assign(pr.ast->value_ptr, pr.ast->value_len);
189-
else
190-
name = "sp";
248+
else name = "sp";
191249
dr.success = txn_mgr_.savepoint(name.c_str());
192250
if (!dr.success) dr.error_message = "SAVEPOINT failed";
193251
return dr;
194252
}
253+
case sql_parser::StmtType::RELEASE_SAVEPOINT: {
254+
if constexpr (D == sql_parser::Dialect::PostgreSQL) {
255+
const auto* operand = pr.ast->first_child;
256+
if (!operand || operand->type != sql_parser::NodeType::NODE_IDENTIFIER ||
257+
operand->next_sibling)
258+
return transaction_error("parse error");
259+
std::string name = transaction_name(operand);
260+
DmlResult dr;
261+
dr.success = txn_mgr_.release_savepoint(name.c_str());
262+
if (!dr.success) dr.error_message = "RELEASE SAVEPOINT failed";
263+
return dr;
264+
}
265+
break;
266+
}
195267
default:
196268
break;
197269
}

‎include/sql_parser/delete_parser.h‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -350,8 +350,10 @@ class DeleteParser {
350350
if (next.type == TokenType::TK_AS) {
351351
tok_.skip();
352352
Token alias_name = tok_.next_token();
353-
ret->add_child(make_node(arena_, NodeType::NODE_ALIAS,
354-
alias_name.source.empty() ? alias_name.text : alias_name.source));
353+
AstNode* alias = make_node_from_token(arena_, NodeType::NODE_ALIAS, alias_name);
354+
if (alias && alias_name.source.ptr != alias_name.text.ptr)
355+
alias->flags |= FLAG_IDENT_DELIMITED;
356+
ret->add_child(alias);
355357
}
356358

357359
if (tok_.peek().type == TokenType::TK_COMMA) {

0 commit comments

Comments
 (0)