Repository navigation
fix: return error tuple on dtype/operator mismatch in Query.where predicates - #27
Merged
Merged
Conversation
…rylabs/delta_query into sp/apply-correct-predicate
stuartjohnpage
requested review from
emmy-bishop,
rdupart and
theblkguy
and
a lite review from Copilot
September 3, 2026 18:12
There was a problem hiding this comment.
🟢 Approval recommended
The functional changes are cohesive, error propagation is handled, and new tests cover both the regression and a positive path, with only a minor docstring inconsistency noted.
Pull request overview
This PR aligns client-side Parquet row filtering with the existing Results.filter/2 behavior by guarding ordering operators (>, <, >=, <=) against non-orderable Explorer dtypes, returning a descriptive {:error, message} instead of raising, and propagating that error up through query execution.
Changes:
- Add dtype/operator validation to
Client.apply_filter/4and return an error tuple for invalid ordering comparisons. - Propagate
Client.parse_parquet_files/3failures throughQuery.process_files/3instead of hard-matching{:ok, df}. - Extract
orderable_dtype?/1intoPredicateParserand add regression/positive tests for Parquet row filtering.
File summaries
| File | Description |
|---|---|
| test/delta_query/client_test.exs | Adds adapter-backed tests covering valid numeric ordering filters and invalid string ordering filters. |
| lib/delta_query/results.ex | Refactors aliases and switches to shared PredicateParser.orderable_dtype?/1 for ordered operator validation. |
| lib/delta_query/query.ex | Ensures process_files/3 passes through Client.parse_parquet_files/3 errors via with. |
| lib/delta_query/predicate_parser.ex | Introduces public orderable_dtype?/1 helper with docs/spec for shared dtype checks. |
| lib/delta_query/client.ex | Returns `{:ok, df} |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Dacello
approved these changes
Sep 3, 2026
emmy-bishop
approved these changes
Sep 3, 2026
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.
Summary
Results.filter/2against ordering operators (>,<,>=,<=) on non-orderable columns, but the same unguarded dispatch remained inClient.apply_filter/4Client.apply_filter/4now performs the same dtype check and returns{:error, "operator >= not supported on :string column 'revision'"}.Query.process_files/3previously hard-matched{:ok, df}fromparse_parquet_files/3; it now passes an error through.Drive-bys
orderable_dtype?/1abstracted outTest plan
mix test— 8 doctests + 110 tests passClient.parse_parquet_files/3withrevision >= 1on a string column returns{:error, msg}wheremsg =~ "operator >=",msg =~ "revision",msg =~ "string"id >= 2on a numeric column through the same path still filters rows correctlyResults.filter/2dtype tests continue to pass against the sharedorderable_dtype?/1