feat(sql): add parameterized query support#6692
feat(sql): add parameterized query support#6692Lucas61000 wants to merge 6 commits intoEventual-Inc:mainfrom
Conversation
Greptile SummaryThis PR adds parameterized query support to
Confidence Score: 3/5Not safe to merge: the feature's primary goal (injection prevention) is unmet, doctests are broken, and ::TYPE casts cause false errors. A P0 security issue (unescaped single quotes enabling SQL injection), two P1 logic issues (broken docstring examples, ::TYPE cast false positives), and broken doctests that would fail make doctests in CI collectively block this PR. daft/sql/sql.py — _format_sql_value (single-quote escaping), _apply_parameters (:(\w+) regex for casts), and all three parameterized docstring examples.
|
| Filename | Overview |
|---|---|
| daft/sql/sql.py | Adds _apply_parameters and _format_sql_value helpers plus a params arg to sql(); has a P0 SQL-injection bug (unescaped single quotes), P1 broken doctests, and P1 false-positive named-param detection on ::TYPE casts. |
| tests/sql/test_parameterized_queries.py | Adds 8 tests covering all three placeholder styles and error cases with correct params= keyword usage, but no test covers injection-safety (strings with single quotes). |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["daft.sql(sql, params=...)"] --> B{params is None?}
B -- Yes --> E[Pass SQL as-is to _sql_exec]
B -- No --> C["_apply_parameters(sql, params)"]
C --> D{type of params?}
D -- dict --> F["re.sub replace_named\n⚠️ Also matches ::TYPE casts"]
D -- list/tuple --> G{Detect style}
G --> H[has_dollar]
G --> I[has_question]
G --> J["has_named\n⚠️ False-positive on ::TYPE"]
H & I & J --> K{style_count > 1?}
K -- Yes --> L[raise ValueError: Cannot mix]
K -- No --> M{has_dollar?}
M -- Yes --> N[replace_indexed]
M -- No --> O[replace_question]
N & O & F --> P["_format_sql_value\n⚠️ No single-quote escaping"]
P --> Q[Substituted SQL]
Q --> E
E --> R[DataFrame result]
Reviews (1): Last reviewed commit: "feat(sql): add parameterized query suppo..." | Re-trigger Greptile
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
- Fix doctest examples: use params= keyword argument and correct expected output (age >= 25 instead of age > 25 for Alice) - Fix ::TYPE casts being mistaken for named parameters using negative lookbehind (?<!:) - Raise ValueError when surplus ? parameters are provided - Add tests for type cast detection and surplus parameters
Changes Made
Add parameterized query support to
daft.sql()to prevent SQL injection attacks.paramsargument supporting three parameter styles:?(sequential substitution)$1,$2,$3... (indexed substitution):name(named substitution from dict)Related Issues
Closes #4156