Repository navigation
fix(dynamodb, cosmosdb, mongodb): push filters down only where the source evaluates them as SQL does - #14419
Conversation
✅ Pull with Spice PassedPassing checks:
|
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
DynamoDB and Cosmos DB can apply limits before retained inexact filters, causing valid matching rows to be omitted.
Review effort: Balanced
Findings: 2
Open (2)
What changed in this PR
Improves SQL-correct filter pushdown for DynamoDB, Cosmos DB, and MongoDB.
Changes:
- Adds semantics-aware DynamoDB and Cosmos DB filter/request planning.
- Pins MongoDB pushdown fixes from the provider fork.
- Adds differential integration tests and documentation.
| File | Description |
|---|---|
Cargo.lock |
Updates locked dependencies. |
Cargo.toml |
Updates the table-providers fork pin. |
connector-cosmosdb/.../filter.rs |
Implements Cosmos DB filter translation. |
connector-cosmosdb/.../filter/tests.rs |
Tests Cosmos filter semantics. |
connector-cosmosdb/.../mod.rs |
Registers filter support. |
connector-cosmosdb/.../provider.rs |
Adds query, projection, partition, and limit pushdown. |
connector-dynamodb/src/arrow.rs |
Tightens date and timestamp parsing. |
connector-dynamodb/src/filter.rs |
Implements DynamoDB filter translation. |
connector-dynamodb/src/filter/tests.rs |
Tests translation edge cases. |
connector-dynamodb/src/lib.rs |
Registers the filter module. |
connector-dynamodb/src/provider.rs |
Integrates request, sort, and limit pushdown. |
connector-dynamodb/src/request_builder.rs |
Builds bounded Query and Scan requests. |
connector-dynamodb/src/request_builder/tests.rs |
Tests request planning. |
connector-dynamodb/src/request_plan.rs |
Expands request-plan metadata and variants. |
connector-dynamodb/src/table_schema.rs |
Adds key types and translation metadata. |
connector-dynamodb/src/utils.rs |
Removes the superseded filter visitor. |
crates/runtime/Cargo.toml |
Adds Cosmos test dependencies. |
runtime/tests/cosmosdb/mod.rs |
Registers the Cosmos round-trip suite. |
runtime/tests/cosmosdb/pushdown_roundtrip.rs |
Adds Cosmos differential tests. |
runtime/tests/dynamodb/mod.rs |
Registers the DynamoDB suite. |
runtime/tests/dynamodb/pushdown_roundtrip.rs |
Adds DynamoDB differential tests. |
runtime/tests/integration.rs |
Registers the shared harness. |
runtime/tests/mongo/mod.rs |
Registers the MongoDB suite. |
runtime/tests/mongo/pushdown_roundtrip.rs |
Adds MongoDB differential tests. |
runtime/tests/pushdown_roundtrip.rs |
Provides shared comparison utilities. |
docs/dev/cosmosdb.md |
Documents supported pushdown behavior. |
docs/dev/fork_patches.md |
Records the MongoDB fork patch. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Core query-correctness logic spans a large connector change and an external temporary fork pin, preventing confident automated approval.
Review effort: Balanced
Findings: None
Resolved since last review (1)
|
Addressed #14419 (review). That review had no line-comment thread. Its headline was that the query-correctness logic spans a large connector change and a temporary fork pin, which it leaves to a human reviewer, so I'm not requesting another Copilot pass. The fork pin. spiceai/datafusion-table-providers#77 has to merge first. This PR then repoints Correctness. At this head,
#14419 (comment) is Cursor Bugbot's notice that its spend limit was reached. It asks nothing of this PR. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
… from A flattened column such as m.x.y is read from the map m, a map attribute m.x, or an attribute named m.x.y, but the projection named only the first segment of the column's name. An item whose value sat under a top-level attribute with a dot in its name came back without it, so m.x.y read as NULL and a filter on it dropped the row. The projection now names every prefix of the name that ends before a dot, and the whole name. The DynamoDB round-trip suite writes an item of each kind and requires all of them to read m.x.y. The local copy shares the projection, so a round trip cannot tell.
…lan snapshot The EXPLAIN snapshot of dynamodb_timestamp_filter_pushdown still held trunk's request, which compared each timestamp bound as a string in a single offset. This branch compares the stored local date and time within a day of the bound instead, and leaves the exact comparison to the FilterExec above the scan, so the merge queue failed the test on every try. The new snapshot is the plan the test prints against amazon/dynamodb-local holding the items init_test_table writes, which is byte for byte the plan the merge queue printed against the real table.
7c3ef2d
9403797 to
7c3ef2d
Compare
|
Addressed #14419 (review) as Copilot's quota-limit error on 207b2f6, which reviews nothing: "Copilot was unable to review this pull request because the user who requested the review has reached their quota limit." All four requests since 2026-09-29 20:12Z got this reply (on 1f26067, 9403797, 7c3ef2d and 207b2f6). Another request would come from the same account, so I haven't sent one. Copilot's last review with content was of 2a49ffa, with no findings: #14419 (review). The diff hasn't changed since the approvals of 9403797. They were dismissed at 06:00Z by a rewrite of the branch history. 207b2f6 then merged trunk at 197e35d with no conflict. Against its base, the PR adds and removes the same 8803 lines at both heads. The only two lines that differ are unchanged context, next to the imports that trunk's #14412 edited: The merge-queue test this PR failed on should still pass after the DataFusion bump (#14430) that this merge brings in. The seventh line, Not run: the test itself on 207b2f6. It needs the AWS table whose key only CI holds, so the next queue run is that check.
|
Copilot pointed out that s3[parquet]-federated and abfs_standard_versioned[parquet]-federated still carried the swapped RightAnti plan. A scan of the trunk b97f85e TPC-H sweep found nine more arms failing tpch_q16 on the same RightAnti -> null-aware LeftAnti change. Each snapshot is the new result that arm's trunk run printed. Left out: mongodb-federated (#14419 scope, as before), oracle and oracle[catalog] (older drift beyond the anti join), and mysql-arrow / postgres-arrow (their q16 plan keeps RightAnti).


📝 Summary
On trunk, filter pushdown returned wrong rows for MongoDB and DynamoDB and failed some valid queries, and Cosmos DB pushed nothing at all. Each connector now pushes a filter down as
Exactonly where the source selects exactly the rows SQL keeps from the rows the connector converts. Otherwise it pushes anInexactsuperset thatDataFusionfilters again, and it pushes much more than before.MongoDB (the translator lives in the fork, spiceai/datafusion-table-providers#77, which this pins):
$ne/$nin/$not/$normatch null and missing fields, soage <> 30returned the rows with noage.ObjectIds, documents and symbols into strings.-0.0compare apart from Arrow.s > 'Z'return nothing.SELECT *failed withPath collisionwhen a field is a document in some documents and a scalar in others.DynamoDB:
NOT,<>andNOT INare two-valued in DynamoDB, so they matched missing attributes.-, andx IN (1, NULL), failed the query outright.INlist now becomes one Query per partition. Sort-key predicates become the key condition, with the rest checked item by item.ORDER BYthe sort key setsScanIndexForward, and a limit stops reading.Cosmos DB: projection, a parameterized
WHERE(comparisons,IN,BETWEEN, null tests,LIKE 'prefix%',starts_with) and limits are pushed down. Each condition is guarded by the JSON type it compares, because the emulator and the service disagree on null and undefined. It also keeps a value its column cannot hold, so the query fails as it would unfiltered. An equality on the partition key therefore still reads every partition: a partition key of another JSON type lies in a logical partition of its own.The
starts_withguards translate onlyDataFusion's own function, never a user function registered under that name, and no other function is pushed.Each connector gets a differential round-trip suite. Every query runs against the federated dataset and against an Arrow acceleration of the same source, which
DataFusionevaluates locally, and the suite requires the same rows, compared cell by cell, plus that each case expected to push down did. The shared harness iscrates/runtime/tests/pushdown_roundtrip/mod.rs.🔗 Related
10b7fe71d7onspiceai-54, which this pins. Its tree is that ofc6cbc3a3ca, the branch head the evidence below was taken at.📚 Docs
docs/dev/cosmosdb.md: a Push-down section, and the criteria table no longer says push-down is missing.docs/dev/fork_patches.md: the new pin, and a row for the MongoDB patches with this PR's round-trip suite as their guard.👀 Notes for Reviewers
Evidence. Every query ran against the federated dataset and against an Arrow acceleration of the same source, on
mongo:8.0,amazon/dynamodb-localand the Cosmos DB Linux emulator (vnext-preview). Trunk isd62271b7a2.TRY_CAST, distant dates, dotted names with a catch-allunnest_depth: 1, a field that is a document and a stringPath collision at address.citytimestampcolumn, year-2262 date-ExpressionAttributeNames contains invalid keypk IN ('a', NULL)Invalid FilterExpression: Syntax error-0.0bounds, microsecond layout,+15:00offset, dotted map key, unpadded monthWHEREand projection in the planFor example, on trunk, over a collection collated
{locale: "en", strength: 2}:On this branch the same query returns the truth rows, with
collation=[simple]in the plan.Adversarial reviews of this branch found more cases, each reproduced against the database before it was fixed. Among them: an
Int64sort key holding9007199254740992.1passing anINcheck throughf64rounding; Cosmos DB'sx = i64::MINoverflowing a guard intoc["x"] > 9223372036854775807; seven 100-valueINlists reaching DynamoDB at 4,869 bytes after a second optimizer pass;NOT ((a OR b)), which DynamoDB rejects as redundant parentheses. Copilot's reviews found four more. The fork's filter that matches nothing,{_id: {$exists: false}}, matched every document of a view that projects_idaway. It is{_id: {$in: []}}now, and the MongoDB suite queries such a view. Cosmos DB's single-partition read forpk = 'a'skipped a partition key of another JSON type, so the query returned rows where the unfiltered scan fails to decode that key. It is removed, and the Cosmos DB suite checks such a container. A DynamoDB condition on a flattened column likem.x.ychecked only the nested path. It therefore dropped an item holding the column at two paths (m["x.y"]andm.x.y), which unnesting refuses. The condition now keeps an item with something at any other split of the name, and the DynamoDB suite checks such an item. Its projection also named only the first segment of a flattened column's name, so a column held under a top-level attribute such asm.xread as NULL; that predates this PR, and the projection now names every prefix of the name.Behavior changes. These are bug fixes, but they change results for malformed or out-of-range data:
2024- 1-01,2024-1-02T…) now reads as NULL, as any other unparsable string does.datetoo distant for a declared microsecond or nanosecond column reads as NULL instead of wrapping.ORDER BY … LIMITsorts inDataFusion. Exact sort pushdown, through per-type queries merged in order, can follow.Tests.
connector-dynamodb220,connector-cosmosdb45, and the fork's MongoDB module 164.cargo test -p runtime --test integration --features mongodb,dynamodb,cosmosdb,duckdb -- --include-ignored pushdown_round_tripspasses. That is 165 MongoDB cases (137 pushed), 117 DynamoDB (105 pushed) and 56 Cosmos DB (46 pushed). Over a container whose partition keys are of two JSON types, the Cosmos DB suite also requires a pushed-down equality on the partition key to fail on decoding, as the unfiltered scan does. The DynamoDB suite likewise requires a pushed-down condition to fail on an item that unnesting readsm.x.yfrom twice, and every item to readm.x.yfrom whichever attribute holds it.integration.yml. The Cosmos DB suite is#[ignore]d because the emulator is a large image, and CI doesn't enablecosmosdbanyway.EXPLAINsnapshot of a timestamp filter,dynamodb_timestamp_filter_pushdown, now shows the widened bounds:#n0 >= :v0 AND #n1 <= :v1over2024-11-30T12:34:56.123and2024-12-02T12:34:56.123~, where trunk compared2024-12-01T17:34:56.123+05:00as a string. The merge queue runs that module against a table in AWS and failed on the old snapshot. The new one is the plan the test prints againstamazon/dynamodb-localholding the same items, which matched the queue's output byte for byte.Signed off by lukekim via remote — lint+test passed in 6922s. The remote sign-off of 9403797, that snapshot and a merge of trunk, is running.Found along the way, not fixed here:
SELECT truncate(1.5, 0)fails on trunk withTruncation width must be a positive Int64, got Float64. The round-trip suites usebucketto check that a Spice function stays out of the pushed filter.