Copilot commented on code in PR #1484:
URL:
https://github.com/apache/datafusion-python/pull/1484#discussion_r3059185111
##########
python/datafusion/functions.py:
##########
@@ -1179,18 +1197,20 @@ def ln(arg: Expr) -> Expr:
return Expr(f.ln(arg.expr))
-def log(base: Expr, num: Expr) -> Expr:
+def log(base: Expr | float, num: Expr) -> Expr:
Review Comment:
The new type hints for `log`, `power`, and `pow` only allow `float` for
`base`/`exponent`, but the coercion (`Expr.literal(...)`) will also accept
`int` cleanly and the upstream signatures typically coerce “numeric”/“integer”
for these parameters. To avoid incorrect/overly-restrictive typing for users
and type-checkers, widen these hints to `Expr | int | float` (or at least
include `int`) for the numeric literal parameters.
```suggestion
def log(base: Expr | int | float, num: Expr) -> Expr:
```
##########
python/datafusion/functions.py:
##########
@@ -2276,22 +2359,29 @@ def datepart(part: Expr, date: Expr) -> Expr:
return date_part(part, date)
-def date_part(part: Expr, date: Expr) -> Expr:
+def date_part(part: Expr | str, date: Expr) -> Expr:
"""Extracts a subfield from the date.
+ Args:
+ part: The part of the date to extract. Must be one of ``"year"``,
+ ``"month"``, ``"day"``, ``"hour"``, ``"minute"``, ``"second"``,
etc.
Review Comment:
The docstrings describe `part` as an enum-like literal (e.g., `"year"`,
`"month"`, …), but the signatures still accept `Expr | str`. If `Expr` is
supported only for backward compatibility (and/or the Rust implementation
requires a literal scalar), please document that expectation explicitly (e.g.,
“must be a literal scalar string; column expressions are not supported”) to
avoid misleading users. Alternatively (more breaking), consider narrowing the
type to `str` if non-literal expressions are not meaningful here.
##########
python/datafusion/functions.py:
##########
@@ -1400,22 +1431,24 @@ def position(string: Expr, substring: Expr) -> Expr:
return strpos(string, substring)
-def power(base: Expr, exponent: Expr) -> Expr:
+def power(base: Expr, exponent: Expr | float) -> Expr:
"""Returns ``base`` raised to the power of ``exponent``.
Examples:
>>> ctx = dfn.SessionContext()
>>> df = ctx.from_pydict({"a": [2.0]})
>>> result = df.select(
- ... dfn.functions.power(dfn.col("a"), dfn.lit(3.0)).alias("pow")
+ ... dfn.functions.power(dfn.col("a"), 3.0).alias("pow")
... )
>>> result.collect_column("pow")[0].as_py()
8.0
"""
+ if not isinstance(exponent, Expr):
+ exponent = Expr.literal(exponent)
return Expr(f.power(base.expr, exponent.expr))
-def pow(base: Expr, exponent: Expr) -> Expr:
+def pow(base: Expr, exponent: Expr | float) -> Expr:
Review Comment:
The new type hints for `log`, `power`, and `pow` only allow `float` for
`base`/`exponent`, but the coercion (`Expr.literal(...)`) will also accept
`int` cleanly and the upstream signatures typically coerce “numeric”/“integer”
for these parameters. To avoid incorrect/overly-restrictive typing for users
and type-checkers, widen these hints to `Expr | int | float` (or at least
include `int`) for the numeric literal parameters.
##########
python/datafusion/functions.py:
##########
@@ -1400,22 +1431,24 @@ def position(string: Expr, substring: Expr) -> Expr:
return strpos(string, substring)
-def power(base: Expr, exponent: Expr) -> Expr:
+def power(base: Expr, exponent: Expr | float) -> Expr:
Review Comment:
The new type hints for `log`, `power`, and `pow` only allow `float` for
`base`/`exponent`, but the coercion (`Expr.literal(...)`) will also accept
`int` cleanly and the upstream signatures typically coerce “numeric”/“integer”
for these parameters. To avoid incorrect/overly-restrictive typing for users
and type-checkers, widen these hints to `Expr | int | float` (or at least
include `int`) for the numeric literal parameters.
##########
python/datafusion/functions.py:
##########
@@ -1179,18 +1197,20 @@ def ln(arg: Expr) -> Expr:
return Expr(f.ln(arg.expr))
-def log(base: Expr, num: Expr) -> Expr:
+def log(base: Expr | float, num: Expr) -> Expr:
"""Returns the logarithm of a number for a particular ``base``.
Examples:
>>> ctx = dfn.SessionContext()
>>> df = ctx.from_pydict({"a": [100.0]})
>>> result = df.select(
- ... dfn.functions.log(dfn.lit(10.0), dfn.col("a")).alias("log")
+ ... dfn.functions.log(10.0, dfn.col("a")).alias("log")
... )
>>> result.collect_column("log")[0].as_py()
2.0
"""
+ if not isinstance(base, Expr):
+ base = Expr.literal(base)
return Expr(f.log(base.expr, num.expr))
Review Comment:
The new type hints for `log`, `power`, and `pow` only allow `float` for
`base`/`exponent`, but the coercion (`Expr.literal(...)`) will also accept
`int` cleanly and the upstream signatures typically coerce “numeric”/“integer”
for these parameters. To avoid incorrect/overly-restrictive typing for users
and type-checkers, widen these hints to `Expr | int | float` (or at least
include `int`) for the numeric literal parameters.
##########
python/datafusion/functions.py:
##########
@@ -2276,22 +2359,29 @@ def datepart(part: Expr, date: Expr) -> Expr:
return date_part(part, date)
-def date_part(part: Expr, date: Expr) -> Expr:
+def date_part(part: Expr | str, date: Expr) -> Expr:
"""Extracts a subfield from the date.
+ Args:
+ part: The part of the date to extract. Must be one of ``"year"``,
+ ``"month"``, ``"day"``, ``"hour"``, ``"minute"``, ``"second"``,
etc.
+ date: The date expression to extract from.
+
Examples:
>>> ctx = dfn.SessionContext()
>>> df = ctx.from_pydict({"a": ["2021-07-15T00:00:00"]})
>>> df = df.select(dfn.functions.to_timestamp(dfn.col("a")).alias("a"))
>>> result = df.select(
- ... dfn.functions.date_part(dfn.lit("year"),
dfn.col("a")).alias("y"))
+ ... dfn.functions.date_part("year", dfn.col("a")).alias("y"))
>>> result.collect_column("y")[0].as_py()
2021
"""
+ if not isinstance(part, Expr):
+ part = Expr.literal(part)
return Expr(f.date_part(part.expr, date.expr))
Review Comment:
The docstrings describe `part` as an enum-like literal (e.g., `"year"`,
`"month"`, …), but the signatures still accept `Expr | str`. If `Expr` is
supported only for backward compatibility (and/or the Rust implementation
requires a literal scalar), please document that expectation explicitly (e.g.,
“must be a literal scalar string; column expressions are not supported”) to
avoid misleading users. Alternatively (more breaking), consider narrowing the
type to `str` if non-literal expressions are not meaningful here.
##########
python/datafusion/functions.py:
##########
@@ -2300,25 +2390,30 @@ def extract(part: Expr, date: Expr) -> Expr:
return date_part(part, date)
-def date_trunc(part: Expr, date: Expr) -> Expr:
+def date_trunc(part: Expr | str, date: Expr) -> Expr:
"""Truncates the date to a specified level of precision.
+ Args:
+ part: The precision to truncate to. Must be one of ``"year"``,
+ ``"month"``, ``"day"``, ``"hour"``, ``"minute"``, ``"second"``,
etc.
+ date: The date expression to truncate.
+
Examples:
>>> ctx = dfn.SessionContext()
>>> df = ctx.from_pydict({"a": ["2021-07-15T12:34:56"]})
>>> df = df.select(dfn.functions.to_timestamp(dfn.col("a")).alias("a"))
>>> result = df.select(
- ... dfn.functions.date_trunc(
- ... dfn.lit("month"), dfn.col("a")
- ... ).alias("t")
+ ... dfn.functions.date_trunc("month", dfn.col("a")).alias("t")
... )
>>> str(result.collect_column("t")[0].as_py())
'2021-07-01 00:00:00'
"""
+ if not isinstance(part, Expr):
+ part = Expr.literal(part)
return Expr(f.date_trunc(part.expr, date.expr))
Review Comment:
The docstrings describe `part` as an enum-like literal (e.g., `"year"`,
`"month"`, …), but the signatures still accept `Expr | str`. If `Expr` is
supported only for backward compatibility (and/or the Rust implementation
requires a literal scalar), please document that expectation explicitly (e.g.,
“must be a literal scalar string; column expressions are not supported”) to
avoid misleading users. Alternatively (more breaking), consider narrowing the
type to `str` if non-literal expressions are not meaningful here.
##########
python/datafusion/functions.py:
##########
@@ -2300,25 +2390,30 @@ def extract(part: Expr, date: Expr) -> Expr:
return date_part(part, date)
-def date_trunc(part: Expr, date: Expr) -> Expr:
+def date_trunc(part: Expr | str, date: Expr) -> Expr:
Review Comment:
The docstrings describe `part` as an enum-like literal (e.g., `"year"`,
`"month"`, …), but the signatures still accept `Expr | str`. If `Expr` is
supported only for backward compatibility (and/or the Rust implementation
requires a literal scalar), please document that expectation explicitly (e.g.,
“must be a literal scalar string; column expressions are not supported”) to
avoid misleading users. Alternatively (more breaking), consider narrowing the
type to `str` if non-literal expressions are not meaningful here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
