Skip to content

Commit dc2ae77

Browse files
Update SKILL.md
1 parent b4f1e65 commit dc2ae77

1 file changed

Lines changed: 15 additions & 7 deletions

File tree

‎.ai/skills/make-pythonic/SKILL.md‎

Lines changed: 15 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -321,12 +321,8 @@ def concat_ws(separator: str, *args: Expr) -> Expr:
321321

322322
### Category C: Arguments That Should Accept str as Column Name
323323

324-
In some contexts a string argument naturally refers to a column name rather than a literal. This is the pattern used by DataFrame methods.
325-
326324
**Type hint pattern:** `Expr | str`
327325

328-
**When to use:** Only when the string contextually means a column name (rare in `functions.py`, more common in DataFrame methods).
329-
330326
```python
331327
# Use _to_raw_expr() from expr.py for this pattern
332328
from datafusion.expr import _to_raw_expr
@@ -336,9 +332,21 @@ def some_function(column: Expr | str) -> Expr:
336332
return Expr(f.some_function(raw))
337333
```
338334

339-
**IMPORTANT:** In `functions.py`, string arguments almost never mean column names. Functions operate on expressions, and column references should use `col()`. Category C applies mainly to DataFrame methods and context APIs, not to scalar/aggregate/window functions. Do NOT convert string arguments to column expressions in `functions.py` unless there is a very clear reason to do so.
335+
#### Column name or literal?
336+
337+
A bare string has to be read either as a column name or as a literal value. Decide by asking what a string *literal* would mean in that position:
338+
339+
1. **The position only makes sense as column data.** A constant there is meaningless, so a string can only be a column name. Accept `Expr | str` and convert with `_to_raw_expr()`. This covers:
340+
- the column inputs of aggregate functions (`sum("a")`, `corr("a", "b")`, `grouping("a")`), because aggregating a constant string is never what the caller wants
341+
- the column arguments of DataFrame and context methods (`select`, `sort`, `aggregate`)
342+
2. **The position is a parameter of the operation, not its data.** Delimiters, patterns, date parts, format strings and `string_agg`'s `delimiter` are examples. A string here is a literal, so use Category A or B.
343+
3. **The position is the data of a scalar or array function.** A literal string is a valid, common input there (`upper("abc")`, `concat(col("a"), "-")`), so a bare string is ambiguous. Keep it `Expr` only and let the caller write `col()` or `lit()`.
344+
345+
If you can't tell which rule applies, use rule 3.
346+
347+
SQL's `count(*)` is a special value, not a column name: `count("*")` means `count()`.
340348

341-
The documented exception is the column inputs of aggregate functions (`sum`, `avg`, `count`, `corr`, the `regr_*` family, and so on). There a string can only mean a column, so they accept `Expr | str` via `_to_raw_expr()`. Their literal arguments (for example `string_agg`'s `delimiter` or `nth_value`'s `n`) are unaffected.
349+
Window functions have not been converted to rule 1 yet. Their inputs (for example `lead`'s `arg`) are still `Expr` only.
342350

343351
## Implementation Steps
344352

@@ -432,7 +440,7 @@ from datafusion.expr import coerce_to_expr, coerce_to_expr_or_none
432440

433441
## What NOT to Change
434442

435-
- **Do not change arguments that represent data columns.** If an argument is the primary data being operated on (e.g., the `string` in `left(string, n)` or the `array` in `array_sort(array)`), it should remain `Expr` only. Users should use `col()` for column references.
443+
- **Do not change the data arguments of scalar and array functions.** If an argument is the primary data being operated on (e.g., the `string` in `left(string, n)` or the `array` in `array_sort(array)`), a string there could be a literal, so it should remain `Expr` only (rule 3 under Category C). Aggregate column inputs are different: see rule 1.
436444
- **Do not change variadic `*args: Expr` parameters.** These represent multiple expressions and should stay as `Expr`.
437445
- **Do not change arguments where the coercion is ambiguous.** If it is unclear whether a string should be a column name or a literal, leave it as `Expr` and let the user be explicit.
438446
- **Do not add coercion logic to simple aliases.** If a function is just `return other_function(...)`, the primary function handles coercion. However, you **must update the alias's type hints** to match the primary function's signature so that type checkers and documentation accurately reflect what the alias accepts.

0 commit comments

Comments
 (0)