Add PostgreSQL $N and SQLite :name placeholder syntaxes (#263)#264
Open
aleks-f wants to merge 6 commits into
Open
Add PostgreSQL $N and SQLite :name placeholder syntaxes (#263)#264aleks-f wants to merge 6 commits into
aleks-f wants to merge 6 commits into
Conversation
Extends the parser to recognise two placeholder forms alongside the existing ?: - $N (PostgreSQL 1-based explicit numbering, e.g. WHERE a = $1) - :name (SQLite named, e.g. WHERE a = :user) The ? path is unchanged at every layer (lexer, parser, AST, result ordering); existing tests under test/prepare_tests.cpp pass untouched. Implementation: - New lexer rules for \$[0-9]+ and :[A-Za-z][A-Za-z0-9_]* placed before the punctuation catch-all. $ was previously unused; : was in the punctuation class but never referenced by any grammar rule, so the longer :identifier match wins without conflict. - New bison tokens DOLLAR_PARAM (carries the int N) and NAMED_PARAM (carries the identifier). param_expr now has three alternatives. $0 is rejected via YYERROR at parse time. - Three ExprType values appended (no ordinal change): kExprParameter for ?, kExprParameterDollar for $N (ival = N, 1-based), and kExprParameterNamed for :name (name = identifier). Distinct enum values rather than one with discriminator fields keep consumer switch statements explicit and round-trip printing straightforward. - Factory functions Expr::makeDollarParameter and makeNamedParameter. isLiteral returns true for all three. The destructor already frees name so no further changes are needed. - The top-level input rule's renumber loop only touches kExprParameter; $N keeps its explicit ival, :name keeps its name. SQLParserResult::addParameter still sorts by ival, which now orders $N by explicit position and clusters named placeholders. - sqlhelper print case extended to handle the two new ExprType values. Tests: - Three new cases in test/prepare_tests.cpp covering $N, $N declared out of order (verifying ival-sorted result vector), and :name. - Three new good queries and two new bad queries ($0, lone $) in test/queries/. - bison -v reports no new shift/reduce or reduce/reduce conflicts. - make test passes all three checks (SQL tests, valgrind, grammar).
Author
|
I just noticed bison upgrade also went in. If you want that reverted, let me know. |
…XECUTE Two improvements on top of the placeholder support landed in hyrise#263: 1. Placeholder Expr nodes now record their source column (0-based) in Expr::ival2 across all three kinds. For ? the column is yylloc.total_column - 1, for $N total_column - (1 + digits of N), for :name total_column - 1 - strlen(name). ival2 was previously written once for ? (yyloc.param_list.size(), which duplicated the sequential id later stored in ival by the top-level renumber loop) and not read by any consumer. Repurposing it for the source column gives all three placeholder kinds a consistent meaning: ival is the binding identity (sequential for ?, explicit N for $N, 0 for :name); ival2 is the source column. This lets consumers walk parameters() sorted by ival2 and do text-based placeholder substitution without re-scanning the input. 2. The extended_literal rule no longer rejects ? placeholders. The check at bison_parser.y:1098 was inconsistent with the newer $N / :name placeholders, which the same rule already accepts. Common patterns like INSERT INTO t VALUES(?, ?, ?) and EXECUTE foo(?) now parse natively. Two queries-bad.sql cases that asserted the rejection move to queries-good.sql. prepare_tests.cpp gains ival2 assertions on the four placeholder cases validating computed source columns match hand-counted offsets. Verified: make test passes all three checks (SQL tests, valgrind, grammar conflict).
The macro had a '!= = 0' typo that would fail to compile if ever expanded. No callers currently use ASSERT_STRNEQ so the typo was dormant, but it is still broken on its face. Correct the condition to '== 0' (the inverse of ASSERT_STREQ's '!= 0').
ColumnConstraints owns two heap-allocated members (constraints and references, the latter containing heap-allocated ReferencesSpecification pointers). On the happy path those pointers transfer into a ColumnDefinition which deletes them in its destructor. On parse error bison reaches the wildcard %destructor (delete $$), which only frees the wrapper struct and leaks the inner members. Add a dedicated %destructor for <column_constraints_t> that releases the inner heap-allocated state before deleting the wrapper. valgrind on queries-good.sql + queries-bad.sql now reports 14,545 allocs / 14,545 frees, no leaks.
bison_parser.cpp #includes flex_lexer.h, which is generated alongside flex_lexer.cpp from flex_lexer.l. Under parallel make (with -j>1), the compilation of bison_parser.o could start before flex regenerated flex_lexer.h, producing 'hsql_lex was not declared' errors. Add flex_lexer.cpp as an explicit prerequisite of bison_parser.o so make serializes correctly. The reverse dep (flex_lexer.o depends on bison_parser.cpp) was already wired up; this completes the pair.
The macros expanded to a statement followed by a trailing
static_assert(true, "...") that enforced a closing semicolon. The
expansion was technically a statement + declaration sequence, which
breaks unbraced control-flow contexts (e.g. if (c) ASSERT_TRUE(x); else
...) due to dangling-else / declaration-as-if-body rules.
Wrap each assertion macro body in do { ... } while (false). The
trailing semicolon at the call site now naturally terminates the loop;
the macro is a single statement that composes safely in any context.
Drop the static_assert(true, ...) suffix since it is no longer needed.
ASSERT_EQ / ASSERT_NEQ gain a semicolon after their inner ASSERT call,
which previously relied on the suffix to absorb the user's trailing ';'.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extends the parser to recognise two placeholder forms alongside the existing ?:
The ? path is unchanged at every layer (lexer, parser, AST, result ordering); existing tests under test/prepare_tests.cpp pass untouched.
Implementation:
Tests: