Repository navigation
Require a full statement match (trailing tokens were silently ignored); evaluate unsupported flat SELECTs with the superset connector - #84
Conversation
The SELECT grammar was matched with parseString() and no end anchor, so
pyparsing stopped at the first token it did not recognize and silently
discarded the rest of the statement. A table alias therefore dropped the
WHERE clause: SELECT id FROM t AS a WHERE id = 'x' ran as SELECT id FROM t
and returned every row. The same applied to other unsupported trailing
clauses (e.g. GROUP BY, ORDER BY without a direction, a second statement).
DynamoDB itself rejects these statements ("Aliasing is not supported").
SELECT parsing (including the superset connector) now requires the whole
statement to match, allowing one trailing semicolon, and raises a
ParseException otherwise. Other statement types are unchanged.
…ELECTs with the superset connector Every statement parser used an unanchored parseString(), so trailing tokens were silently ignored and the driver could act on a different target than written: DROP TABLE a b dropped a, DESC a b described a, and a trailing token after an UPDATE/DELETE WHERE clause was dropped. All parsers now require the whole statement to match (one trailing semicolon allowed). The INSERT item grammar stopped at the first closing brace, truncating a literal nested map; the item is now matched with balanced braces (braces in quoted strings are ignored). With connector=superset, a single-table SELECT that PartiQL cannot express (aggregates, GROUP BY, table aliases) -- e.g. a chart on a physical table -- used to fail to parse, or with an alias ran without its WHERE clause. It is now read with a PartiQL scan and evaluated as a whole by the query DB, like the outer part of a nested SELECT. Bind parameters are rejected for this form with a clear error. The query DB loader now reads all pages before creating its table, pads rows read before an attribute first appeared, and stores map/list/set values as JSON text.
…bqueries A Superset series-limit chart JOINs a table against its own top-N subquery (SELECT col AS col__, ... ) AS series_limit ON col = col__. Two problems made it fail with connector=superset: - the statement compiler dropped every label, so the subquery lost "AS col__" and the ON clause referenced a column that did not exist; labels inside subqueries are now rendered when they rename their element (top-level labels stay omitted: PartiQL has no column aliases) - the flat form evaluated FROM <cache> while the subquery still named the DynamoDB table; the query DB now evaluates the statement with a CTE named after the scanned table, so every reference reads the scanned rows Without connector=superset such a SELECT cannot be expressed in PartiQL; the default cursor now raises NotSupportedError saying so instead of a raw ParseException.
…aliases A full-match parse made a trailing comment (e.g. a '-- tag' line appended by query logging) a parse error. Comments are now stripped before parsing, with quoted text kept. ORDER BY accepts a key list with optional ASC/DESC; the grammar required exactly one key with a direction, so 'ORDER BY id' failed. A single-table alias (FROM t [AS] a with a.col references) is resolved to plain PartiQL on both cursors: DynamoDB rejects table aliases and reads a.col as a document path. Joins and subqueries are not rewritten and still require connector=superset. With connector=superset an aliased SELECT now runs as PartiQL (bind parameters work) instead of a scan evaluated in the query DB.
|
Added a45e259: the full-match parse made trailing SQL comments a parse error (for example a DynamoDB Local 3.3.1: full suite 273 passed, 1 skipped. With the source changes reverted and the new tests kept: 16 failed. |
…ng rows pyparsing 3.3.3 rewrote infix_notation and nested_expr, which changed how the WHERE-clause grammar consumes trailing tokens: valid statements like "SELECT id FROM t WHERE id = 'x' ORDER BY id" and "WHERE ... AND contains(...)" now fail the full-statement match this PR added, or silently drop function arguments. Verified with pyparsing 3.3.2 the whole suite is green (310 passed) and with 3.3.3 eight tests fail with ParseException/wrong results. Cap pyparsing below 3.3.3 in setup.py, requirements.txt and the test workflow's install step until the grammar is adapted to the new implementation. Also fixed test_reflected_default_schema_select (merged in from main), which put_item's two rows into the table shared with test_sqlalchemy_dynamodb.py::test_reflect_table and never removed them, making that test's exact row-count assertion depend on run order. It now cleans up the rows it seeds.
|
Pushed c64b482: pins pyparsing below 3.3.3 (a newly released version that rewrote Also merged current Verified locally and via the fork's own Actions run (https://github.com/aminghadersohi/PyDynamoDB/actions/runs/36667961587): all pytest/black/flake8 steps pass across Python 3.9-3.14 for both SQLAlchemy 1.x and 2.x (310 passed each). The only failing jobs there are SonarCloud and Codecov upload, which fail solely because their tokens are repo secrets not available to a fork-triggered run — unrelated to this PR's code. Note for visibility: this repo's |
|
@aminghadersohi Thank you. Acknowledged and Merged! |
Problem
Every statement parser runs
Base.root_parse_results, which callsparseString()without an end anchor. pyparsing therefore stops at the first token the grammar does not recognize and silently discards the rest of the statement. The driver then executes something other than what was written:SELECT id FROM t AS a WHERE id = ?runs asSELECT id FROM "t"and returns every row, although DynamoDB itself rejects aliasing (ValidationException: Aliasing is not supported). Bare aliases (FROM t a),GROUP BY,ORDER BYwithout a direction and a second statement after;are dropped the same way.DROP TABLE Issues TopicsdropsIssues, andDESC Issues TopicsdescribesIssues. A trailing token after anUPDATE/DELETEWHERE clause is dropped and the statement still runs.}, soVALUE {'a': {'k': '1'}}is sent without its final brace.A related case: with
connector=superset, a single-table SELECT that PartiQL cannot express fails to parse. This covers aggregates andGROUP BYon a table, which is what a chart on a physical table issues, e.g.SELECT txt, COUNT(*) FROM t WHERE ... GROUP BY txt. With a table alias it runs without its WHERE clause. Only the nested form (... FROM (SELECT ...) AS virtual_table ...) was evaluated by the query DB.Fix
ParseException.FROM t [AS] awitha.colreferences is rewritten to plain PartiQL on both cursors, since DynamoDB rejects table aliases and readsa.colas a document path. Joins and subqueries are not rewritten.-- tagline appended by query logging is not a parse error under the full match.ASC/DESC; the grammar required exactly one key with a direction, soORDER BY idfailed.connector=superset. A base SELECT is used only when it matches the whole statement (after alias resolution). Otherwise a single-table SELECT is read with a PartiQL scan (SELECT * FROM "t") and evaluated as a whole by the query DB, exactly like the outer part of a nested SELECT. Bind parameters are rejected for this form with a clearNotSupportedError, because only the scan is sent to DynamoDB.SELECT *load on heterogeneous items failed with a binding error.Changed existing tests
Three tests asserted the old lenient behaviour. Silently ignoring trailing tokens can make the driver act on the wrong target, so these now expect an error or the complete statement:
test_ddl_drop.py::test_parse_drop:DROP TABLE Issues Topicsnow raises instead of droppingIssues.test_util_sql.py::test_desc_table:desc Issues Topicsnow raises instead of describingIssues.test_dml_insert.py::test_parse_simple_case_2: the nested-map INSERT now keeps its closing brace. The old expected statement was unbalanced.New tests
tests/test_parse_trailing_tokens.py:;) and rejectsUnexpected,Unexpected Tokens,; DROP TABLE Otherand;;;GROUP BY,ORDER BYand the trailing semicolon.GROUP BYflat SELECT becomes scan + query DB evaluation with the WHERE intact;virtual_tablequery is unchanged.connector=superset) still raises;COUNT/SUM ... GROUP BYreturns the same result as the nested equivalent;Against DynamoDB Local 3.3.1 (Python 3.11, SQLAlchemy 2.0.52, pyparsing 3.3.2): this branch gives 244 passed, 1 skipped.
mainsource with the new tests gives 167 passed / 77 failed (every new or changed expectation).black --check pydynamodbandflake8 pydynamodbare clean.Follow-up: series-limit JOINs (commit 74f9ad1)
A Superset series-limit chart JOINs the table against its own top-N subquery:
(SELECT col AS col__, ...) AS series_limit ON col = col__. Two things made it fail withconnector=superset:AS col__. Labels inside subqueries are now rendered when they rename their element. Top-level labels are still omitted, because PartiQL has no column aliases.FROM <cache>while the subquery still named the DynamoDB table. The query DB now evaluates the statement with a CTE named after the scanned table, so every reference reads the scanned rows.Without
connector=superset, such a SELECT cannot be expressed in PartiQL. The default cursor now raisesNotSupportedErrorsaying so, instead of a rawParseException.New tests:
This head gives 247 passed / 1 skipped. The previous head with these tests gives 4 failed.
Follow-up: comments, ORDER BY lists and single-table aliases (commit a45e259)
The full-match parse made trailing SQL comments a parse error, and
ORDER BY idwithout a direction failed. Comments are now stripped before parsing, ORDER BY takes a key list with optional directions, and a single-table alias is resolved to plain PartiQL on both cursors instead of being rejected (withconnector=superset, an aliased SELECT therefore runs as PartiQL, with bind parameters, rather than as a scan evaluated in the query DB). Joins and subqueries are unchanged.New tests in
tests/test_comments_order_by_aliases.py(comment stripping incl. markers inside quotes, ORDER BY key lists, alias resolution, statements that must not be rewritten, both cursors running an aliased SELECT). One existing SELECT parser case now usesORDER BY id NULLS FIRSTas its unsupported trailing clause, sinceORDER BY idis now valid.DynamoDB Local 3.3.1: full suite 273 passed, 1 skipped. With the source changes reverted and the new tests kept: 16 failed.
Follow-up: pin pyparsing below 3.3.3 (commit c64b482)
pyparsing 3.3.3 (released after the last update above) rewrote
infix_notationandnested_expr, which changed how much of a statement the WHERE-clause grammar consumes. Under 3.3.3 the full-match parse this PR added starts rejecting valid statements (SELECT id FROM t WHERE id = 'x' ORDER BY idfails with "Expected end of text, found 'WHERE'") and acontains(...)/begins_with(...)call in a WHERE clause loses the comma between its arguments. Verified: with pyparsing 3.3.2 the full suite is green; with 3.3.3, eight tests fail.setup.py,requirements.txt, and the test workflow's install step now cappyparsing<3.3.3until the grammar is adapted to the new implementation.Also fixed
test_reflected_default_schema_select(pulled in by the merge frommain/#83), which seeded two rows into the table shared withtest_sqlalchemy_dynamodb.py::test_reflect_tableand never removed them, making that test's exact row-count assertion depend on execution order. It now cleans up the rows it seeds.This head (merged with current
main) gives 310 passed against DynamoDB Local, verified on Python 3.9-3.14 with both SQLAlchemy 1.x and 2.x via the fork's own Actions run.