fix: apply the partition limit through the SQL AST instead of appending it to the query text

[eastagiletracker]

This PR proposes applying the limit_partitions filter through the SQL AST instead of appending it to the query text, so that query(..., limit_partitions) stops failing on statements that end in ORDER BY, LIMIT or GROUP BY. We include this PR work along with a full history of your repo at https://eastagiletracker.com/projects/428. You can sign in with your GitHub ID to claim ownership of the project.

What is wrong today

DatabaseManager::query builds the partition restriction by concatenating it onto the end of the SQL string:

let has_where = sql_query.to_uppercase().contains("WHERE");
if has_where {
  format!("{} AND partition_date IN ({})", sql_query, date_list)
} else {
  format!("{} WHERE partition_date IN ({})", sql_query, date_list)
}

A WHERE clause only belongs directly after FROM, so any query whose text continues past that point becomes invalid SQL. SELECT * FROM t ORDER BY date turns into SELECT * FROM t ORDER BY date WHERE partition_date IN (...), and the same happens with a trailing LIMIT, GROUP BY or HAVING. Since limitPartitions is part of the documented mobile surface (query and queryDf in the README, and nativeQuery on both the JNI and the iOS side), an app that passes it together with an ordinary sorted or paged query gets an error back instead of rows.

There are two quieter failures in the same three lines. Searching the raw text for WHERE also matches a string literal, a column alias or a subquery, so SELECT 'nowhere' AS label FROM t is treated as already having a WHERE clause and gets AND ... appended to a statement that has none. And because AND binds tighter than OR, appending the partition condition to a filter that ends in an OR only restricts that last branch: WHERE id = 1 OR id = 3 with a one-partition limit still returns the row from the excluded partition, with no error to notice.

Reproducing it on develop

At 760ce86, with three records written into three consecutive 30-minute partitions:

$ cargo test limit_partitions
query 'SELECT * FROM test_table ORDER BY date DESC' failed: SQL error: ParserError("Expected end of statement, found: WHERE")
query 'SELECT * FROM test_table ORDER BY date LIMIT 1' failed: SQL error: ParserError("Expected end of statement, found: WHERE")

assertion `left == right` failed      # SELECT * FROM test_table WHERE id = 1 OR id = 3, limit_partitions = 1
  left: [3, 1]
 right: [3]

test result: FAILED. 1 passed; 4 failed

The one test that passes there is the control: a plain SELECT * FROM test_table has nothing after the table name, which is the single shape the string concatenation gets right.

The change

inject_partition_filter in sql_query_parser.rs builds the restriction as an InList expression and injects it into the WHERE clause of each SELECT that reads a stored table, reusing the sqlparser this module already parses with, then renders the statement back to SQL. Trailing clauses keep their position because they are separate AST nodes; an existing condition is wrapped in parentheses before the AND is added, so an OR still binds first; and the WHERE-in-the-text guess disappears entirely. A SELECT that reads only a CTE or a derived table is skipped and the filter is pushed into that body instead, where the partition column is still available, and both sides of a UNION are covered. An empty partition list returns the query untouched, which keeps the existing early-exit behaviour.

The public signature of query/query_df is unchanged, and calls that pass None never reach this code at all.

Verification

19 tests come with the change: 14 unit tests on inject_partition_filter covering ORDER BY, LIMIT, GROUP BY/HAVING, an existing WHERE, the OR precedence case, the string-literal false positive, CTEs, derived tables, UNION, JOIN, an empty partition list, a SELECT with no table, and malformed SQL; plus 5 tests that drive the real DatabaseManager::query path end to end over a three-partition table.

Reverting only the call site in db_manager.rs while keeping the new function is what produced the failures quoted above — 4 of the 5 end-to-end tests go red, the unit tests stay green, which is what pins the failures to the injection rather than to the tests.

$ cargo test          # develop at 760ce86
test result: FAILED. 482 passed; 1 failed

$ cargo test          # with this change
test result: FAILED. 501 passed; 1 failed

Same single failure before and after: cloud_sync_test::test_process_sink_parquet_file_*, which is order-dependent — running that module alone on an untouched develop reproduces it, and which member of the family fails changes between runs. Nothing this change touches is involved. cargo fmt --check is clean, and cargo clippy --all-targets reports exactly the same five pre-existing errors in helpers_test.rs and sql_query_parser_test.rs as it does on develop.

One thing noticed while reading the surrounding code, left alone here because it is a separate decision: all_partitions is collected from every table in the database rather than from the tables the query actually references, so a limit applied to a database with unevenly-partitioned tables can select dates that do not exist for the table being read.

How this was managed

This work was tracked as a story on a board imported from this repository's own issues and pull requests — 187 stories in all — and the board stays live: the story for this fix and the board itself.

If you'd rather not receive contributions like this, reply no-more-prs on this pull request and we won't open any further ones on your repositories.

---

Lawrence W. Sinclair
CEO / East Agile
linkedin.com/in/lwsinclair/
eastagile.com