Fix wrong results in SQL translation, the SQLAlchemy dialect and superset mode (GROUP BY, NOT/NULL, aliases, LIKE, Decimal, UUID, time grains) - #43
Open
aminghadersohi wants to merge 19 commits into
Conversation
…eflection get_columns typed each field from the first sampled value, and _infer_bson_type had no branch for Decimal128, Int64 or binary values, so they fell through to String. A field whose first sampled document held a null reflected as NullType even when later documents held values. The type map lookup also lowercased its key, so the camel-case objectId and binData entries could never match. Type a field from its first non-null sampled value, recognise Decimal128 (DECIMAL), Int64 (BigInteger) and Binary/bytes (LargeBinary), and look the BSON type up without changing its case.
The dialect declares supports_native_decimal, so SQLAlchemy passes decimal.Decimal parameters straight to the DBAPI and installs no Numeric result processor. PyMongo cannot encode decimal.Decimal, so binding one failed with "cannot encode object", and reads returned bson.Decimal128 instead of the decimal.Decimal a Numeric column promises. Encode bound decimal.Decimal values as Decimal128 when placeholders are replaced, and give Numeric and Float columns result processors that convert Decimal128 before the usual Numeric handling.
SQLAlchemy qualifies every table-bound column (SELECT users.name FROM users), and the compiler only dropped the qualifier for names starting with an underscore. The translator reads a dotted name as an embedded-document path, so users.name looked for a field name inside a field users: projections silently returned NULL, filters matched nothing, and select(table) raised NoSuchColumnError. Render columns without a table qualifier in the SQLAlchemy compiler, and resolve collection-qualified references in hand-written SQL to the field before building the plan. Other dotted names keep their nested-path meaning.
Several constructs were translated into MongoDB queries that ran without
error but returned wrong rows:
- GROUP BY was ignored: the generated $group always used _id: null, so
SELECT flag, COUNT(*) ... GROUP BY flag returned one global count and
dropped the grouped column. ORDER BY, OFFSET and LIMIT were also
dropped from aggregate queries, and ? placeholders in their WHERE
clause were never replaced.
- IN wrapped every literal in quotes, so IN (1, 2) compared numbers with
the strings '1' and '2', and a quoted value containing a comma was
split in two. NOT IN and NOT LIKE were read as a field named <x>NOT.
- LIKE did not escape regex metacharacters, and a SQL-escaped quote
('O''Brien') was kept doubled.
- The SQLAlchemy dialect never quoted PartiQL keywords, so a label such
as COUNT(*) AS count failed to parse, and a quoted alias kept its
quotes in the result description.
Group on the GROUP BY keys and project the SELECT list in order, apply
ORDER BY/OFFSET/LIMIT and parameters inside the generated pipeline, keep
IN literal types, support NOT IN and NOT LIKE, escape LIKE patterns,
unescape doubled quotes, quote PartiQL keywords in the dialect and
unquote quoted aliases and ORDER BY keys. Columns that are neither
grouped nor aggregated, and HAVING, now raise instead of being dropped.
Superset-mode subqueries that aggregate are run as aggregates.
The dialect did not declare native UUID support, so SQLAlchemy 2's Uuid type used its string-based processors. PyMongo returns uuid.UUID (or a subtype-4 Binary), and reading a Uuid column failed with "'UUID' object has no attribute 'replace'". Declare native UUID support and map Uuid to a type that binds standard subtype-4 binaries and reads uuid.UUID, subtype-4 Binary or string values. Legacy subtype 3 is returned unchanged because its byte order depends on the driver that wrote it.
The command responses the DBAPI decodes carry 64-bit integers as bson.Int64, so Integer and BigInteger columns returned that subclass rather than the int SQLAlchemy promises. Convert it in the Integer result processor.
The preprocessor cut every line at the first "--", including one inside a string literal or quoted identifier, so WHERE name = 'a -- b' failed to parse. Strip a line comment only when it starts outside quotes.
WHERE clauses were translated by splitting getText() output, which has no whitespace. NOT a = 1 became a filter on a field named "NOTa" (no rows), NOT (a = 1 OR b = 2) produced no filter at all (every row), and an operand that could not be translated was silently dropped from an AND, or the whole clause fell back to a $text search. <>, NOT IN and NOT LIKE also matched documents where the field was NULL or missing, which SQL never returns. Translate WHERE over the parse tree. Each predicate yields the filter of documents for which it is TRUE and the filter for which it is FALSE (a NULL or missing operand is in neither). NOT swaps them and AND/OR combine them by De Morgan's laws, so NOT a = 1 excludes NULLs as in SQL. A bare boolean field (WHERE flag / WHERE NOT flag) is supported. A predicate on anything but a field path, or a LIKE with a bound pattern, now raises instead of matching the wrong rows; the SQLAlchemy dialect renders LIKE patterns inline so Core like() keeps working. "= NULL" keeps its existing IS NULL meaning. DELETE and UPDATE use the same translation, and a WHERE clause that cannot be translated now fails the statement: it previously became an empty filter and matched every document.
…slated The FROM handler used the whole table reference text as the collection name, so FROM users AS u read a collection named "usersASu" and returned no rows, and u.name was read as an embedded path. Joins and, outside superset mode, subqueries were treated the same way and silently returned nothing. Read the collection and its alias from the parse tree, resolve alias-qualified references (u.name, including in GROUP BY and the ordered SELECT list) to the field, and raise NotSupportedError for joins, subqueries in standard mode and AT/BY bindings. Collection- qualified GROUP BY keys are also resolved now; they grouped on a missing nested path before.
… stage Superset-mode subqueries load the MongoDB rows into an in-memory SQLite table. SQLite has no boolean type, so boolean columns came back as 1/0, and a single NULL in a column made the whole column TEXT, returning numbers and booleans as strings. Declare boolean columns with a private type that is converted back to bool when a query selects the column (expressions such as SUM stay numeric), and let NULL values fit any column type when inferring the schema.
Author
|
Three more commits, each with tests that fail before and pass after:
Seven existing test expectations changed because they encoded the removed behaviour: |
Result rows carried bson.Decimal128 and, in command responses, bson.Int64 values. DB API consumers expect decimal.Decimal and int; Decimal128 in particular cannot be summed or serialised by most libraries. Convert both, including inside embedded documents and arrays.
WHERE predicates recovered the field name by searching the predicate's
concatenated token text for IN(, LIKE, ISNULL and similar, so a field
whose name contains one of them was truncated:
"dislikes IS NULL" filtered on "dis" and "unlike = 5" became a regex on
"un". SELECT returned other rows, and DELETE and UPDATE removed or
rewrote documents that did not match. The same text search rejected
quoted field names with spaces, hyphens or non-ASCII characters,
reversed comparisons (5 < age), and string literals containing IN( or
LIKE.
Read each predicate from its parse-tree node instead: the field is the
path on one side of the operator (either side), the value the literal,
parameter or value function on the other. Also:
- LIKE: fold concatenated string literals ('%' || 'ab' || '%', as
SQLAlchemy renders contains/startswith/endswith) and honour ESCAPE
(like(..., escape=...), autoescape). ESCAPE is re-attached when the
grammar lets it absorb the rest of the WHERE clause.
- Parameters: a bound parameter is a marker in the translated filter, so
a string literal '?' is compared as a value, and a parameter the
statement does not use raises instead of being ignored.
- LIMIT/OFFSET must be integer literals (the dialect renders them
inline); LIMIT ? was dropped and returned every row.
- COUNT/SUM/AVG/MIN/MAX(DISTINCT x) are computed from the set of
distinct non-NULL values; COUNT(DISTINCT x) returned 0.
- HAVING is translated into a $match after grouping, including
aggregates that are not in the SELECT list.
- SELECT expressions other than fields and aggregates raise instead of
returning a NULL column.
- A fractional literal no double represents exactly (0.1) is compared
as a double against double fields and exactly (Decimal128) against
other numeric types.
- Generated pipelines use extended JSON so Decimal128 and date literals
survive.
- UPDATE SET values are read from the parse tree like WHERE values, so SET x = '?' stores the string and SET x = ? binds a parameter; SET and WHERE parameters are bound together in statement order. - LIMIT and OFFSET accept a bound parameter (validated as a non-negative integer at execution) instead of being dropped, which returned every row. Other non-integer values raise. - LIMIT 0 returns no rows: MongoDB reads limit 0 as "no limit", and $limit: 0 is invalid in a pipeline. Tests that used a quoted '?' as a WHERE or SET parameter now use the documented unquoted ?; a quoted '?' is a string literal.
Author
|
Three more commits from a follow-up review, each with tests that fail before and pass after:
The full suite passes on SQLAlchemy 2.0.52 and 1.4.54. |
…late ILIKE
The dialect rendered LIKE patterns as inline literals so the translator
could turn them into a regex while parsing. SQLAlchemy's positional
compilation then rewrites any %(name)s inside such a literal as a
parameter (contains('%(k)s') failed with KeyError, other patterns could
be corrupted).
Render LIKE patterns as bound parameters again and translate them when
parameters are bound: a pattern built from literals and parameters
('%' || ? || '%' ESCAPE '/', as SQLAlchemy renders contains, startswith
and endswith, including autoescape) becomes the regex at execution. A
non-string pattern parameter raises. lower(col) LIKE lower(pattern), as
SQLAlchemy renders ilike(), becomes a case-insensitive regex.
SQLAlchemy 2.0 renders every bind as %(name)s and then converts the whole compiled statement to qmark with a regular expression. Under literal_binds that also rewrites the text of an inline string literal, so x = '%(k)s' was sent as x = '?'. The compiler now masks quoted literals and identifiers while the markers are converted. limit_clause passed literal_binds twice when the statement was compiled with literal_binds (as Apache Superset compiles chart queries), raising TypeError for any query with a LIMIT.
DATE_TRUNC('<unit>', field) in a projection or GROUP BY is translated to
$dateTrunc (week-ending units add six days with $dateAdd). Units: second,
minute, hour, day, week, week_monday, month, quarter, year,
week_ending_saturday, week_ending_sunday; truncation is in UTC. An unknown
unit or a non-field argument raises instead of dropping the column.
The superset-mode SQLite stage registers the same DATE_TRUNC and
STR_TO_DATETIME, stores datetimes as fixed-width UTC text and returns
datetime values for them.
Decimal128 columns were stored in the SQLite stage as doubles or text, so
SUM lost digits and ORDER BY sorted text. They are now stored as REAL plus
an exact text copy, and queries are rewritten (sqlglot, the new
"superset" extra) so the column, SUM/AVG/MIN/MAX, GROUP BY, ORDER BY and
comparisons with numeric literals are evaluated with Decimal arithmetic in
Decimal128 precision (34 digits). Other uses of such a column raise
NotSupportedError instead of computing with doubles.
SUM over a group whose values are all NULL or missing returned 0 ($sum of no numbers); SQL returns NULL. SUM(DISTINCT) likewise. An aggregate without GROUP BY over no input rows returned no row; SQL returns one row (COUNT 0, other aggregates NULL), which is what a chart showing a total expects. The $group is wrapped in $facet so the empty input yields that row; HAVING, LIMIT and grouped queries are unchanged.
When a virtual dataset's own query matched no documents the SQLite stage created no table, so the outer query failed with "no such table" instead of returning COUNT 0 or no rows. The table is now created from the subquery's columns.
A grouped query ordered by an aggregate that is not in the SELECT list (Apache Superset's series-limit pre-query: GROUP BY the series, ORDER BY the limit metric) raised. The aggregate is now computed as a hidden output, like HAVING's, and removed after $sort. HAVING raised KeyError when the SELECT list had a DATE_TRUNC column.
Author
|
New commits (no history rewritten):
New tests: |
aminghadersohi
changed the title
This branch has not been deployed
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
1 participant
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.
Summary
This fixes cases where a query ran without error but returned wrong rows or wrong types, through the SQL translator, the SQLAlchemy dialect and superset mode. Each fix comes with tests that fail before it and pass after it. Two commits are covered by tests added in the next commit: 6f843f5 by f3d45fb, and fb75dcb by 4a94d00. No history has been rewritten; later commits were added in response to review.
Types and SQLAlchemy dialect
get_columnstyped each field from the first sampled value, so a leadingnullproducedNullType._infer_bson_typehad no branch forDecimal128or binary values, which fell through toString, andInt64reflected asIntegerrather thanBigInteger. The type-map lookup lowercased its key, so theobjectId/binDataentries could never match.supports_native_decimal, sodecimal.Decimalbinds reached PyMongo unencoded ("cannot encode object"), andNumericreads returnedbson.Decimal128. Bound decimals are now encoded asDecimal128, andNumeric/Floatresults are converted back.SELECT users.name FROM users), and the translator readusers.nameas an embedded-document path, so the query silently returned NULL. The compiler now renders unqualified columns, and a qualifier equal to the collection name is resolved to the field.Uuidcolumns (b14ae06, SQLAlchemy 2). Reading one failed with'UUID' object has no attribute 'replace'. Uuid now binds standard subtype-4 binaries and readsuuid.UUID, subtype-4Binaryor strings. Legacy subtype 3 is left unchanged.Int64(8ec2dbf).Integer/BigIntegercolumns returnedbson.Int64instead ofint.Decimal/intinstead ofDecimal128/Int64.%(name)sto?when compiling withliteral_bindsfor qmark dialects (reported upstream as literal_binds: a string literal containing %(name)s is rendered as '?' (qmark) or '%%s' (format) in 2.x sqlalchemy/sqlalchemy#13609). The compiler now masks quoted segments during that conversion.limit_clausealso passedliteral_bindstwice underliteral_binds, which raisedTypeErrorfor any query with a LIMIT compiled that way (Apache Superset compiles chart queries like this).Query translation
GROUP BYwas ignored ($groupwith_id: null).ORDER BY/OFFSET/LIMITand did not bind?parameters in theirWHERE.IN (1, 2)compared numbers against the strings'1'and'2'.NOT INandNOT LIKEwere read as a field named<x>NOT.LIKEdid not escape regex metacharacters, and'O''Brien'kept the doubled quote.COUNT(*) AS countwas a syntax error because the dialect never quoted PartiQL keywords.--inside a quoted literal, soWHERE name = 'a -- b'failed to parse.pymongosql/sql/where_tree.py). WHERE is translated over the parse tree: each predicate produces a TRUE and a FALSE filter, NOT swaps them, and AND/OR combine them by De Morgan's laws.NOT a = 1excludes rows whereais NULL or missing, as SQL does; it previously filtered on a field namedNOTa.NOT (a = 1 OR b = 2)previously produced no filter at all.<>,NOT INandNOT LIKEno longer match NULL or missing fields.$textsearch.FROM t AS xandFROM t xare read from the parse tree, andx.colresolves tocol, including in GROUP BY. Joins, standard-mode subqueries and AT/BY now raise instead of returning no rows.IN(/LIKE/ISNULL, so a field such asdislikeswas truncated todis, and a DELETE or UPDATE touched the wrong documents (a live test covers this).COUNT(DISTINCT x)andHAVINGnow work. LIKE literals built by concatenation (as SQLAlchemy renderscontains/startswith/endswith) andESCAPEare supported.LIMIT ?andOFFSET ?are bound as parameters, a quoted string literal'?'is no longer taken for a parameter (a placeholder must be an unquoted?), andLIMIT 0returns no rows.ILIKEis translated (lower(col) LIKE lower(pattern)).SUMof no non-NULL values is NULL (was 0). An aggregate without GROUP BY over no rows returns one row (was none).DATE_TRUNCcolumn no longer raises.Superset mode
boolinstead of1/0, and a NULL no longer turns a numeric column into TEXT.DATE_TRUNC('<unit>', field)in projections and GROUP BY is translated to$dateTrunc(week-ending units add six days with$dateAdd; unknown units raise). The sameDATE_TRUNCandSTR_TO_DATETIMEare registered in the superset-mode SQLite stage. Decimal128 columns keep an exact copy in the SQLite stage, and queries are rewritten with sqlglot (new optionalsupersetextra inpyproject.tomlandrequirements-optional.txt) so the column, SUM/AVG/MIN/MAX, GROUP BY, ORDER BY and numeric comparisons use Decimal arithmetic with Decimal128's 34 digits. Other uses of such a column raiseNotSupportedErrorinstead of computing with doubles. Documented in the README.Changed existing tests
21 existing test expectations in 8 files changed because they encoded removed behaviour:
!=matching NULL/missing (now$ninwith NULL), in the comprehensive, parser_delete, parser_general (×2) and nested_fields tests.test_superset_connection); it now raises.test_cursor_delete.py(×3) andtest_cursor_update.py(×1) that wrote the placeholder as a quoted'?'; they now use an unquoted?(5c83229).test_sql_parser_group.pyrewritten for the new aggregate pipeline shape (4ad11ba).Test plan
>=2.0.0,<3.0.0pin resolves to on Python 3.11+) and 2.0.54 (952 passed, 8 transaction tests skipped: they need a replica set), and on the SQLAlchemy 1.4.54 lane (945 passed), across Python 3.9–3.14.tests/test_decimals_time_grains_literal_binds.py(62 tests) cannot be imported on the head before those commits (notime_grainmodule); with that module added, 57 of its tests fail there.pymongosql/.