Copilot commented on code in PR #1786:
URL:
https://github.com/apache/datafusion-python/pull/1786#discussion_r4219092013
##########
python/datafusion/user_defined.py:
##########
@@ -1333,7 +1380,7 @@ def _create_table_udf(
@staticmethod
def _create_table_udf_decorator(
- name: str | None = None,
+ name: str,
*,
with_session: bool = False,
) -> Callable[[Callable[..., Any]], TableFunction]:
Review Comment:
Making `name` required here is an API-breaking change for decorator usage
patterns that previously allowed omitting the name (and defaulting it). This is
also inconsistent with the other UDF/UDWF decorator helpers in this module that
accept `name: str | None = None`. Consider restoring `name: str | None = None`
(and deriving a default name when `None`), or introducing overloads to support
both explicit-name and inferred-name forms without breaking existing callers.
##########
python/datafusion/context.py:
##########
@@ -2196,7 +2196,7 @@ def read_csv(
schema_infer_max_records=schema_infer_max_records,
file_extension=file_extension,
table_partition_cols=table_partition_cols,
- file_compression_type=file_compression_type,
+ file_compression_type=file_compression_type or "",
Review Comment:
Replacing `None` with an empty string changes behavior and can introduce
hard-to-debug failures if the Rust/internal layer treats `\"\"` as an invalid
compression type rather than 'unset/default'. Prefer passing `None` through if
the internal API supports it; otherwise, consider mapping `None` to the
explicit default value expected by the internal API (or
documenting/centralizing the sentinel handling) and rejecting `\"\"` when the
user explicitly supplies it.
##########
.github/workflows/build.yml:
##########
@@ -81,6 +81,10 @@ jobs:
uv run --no-project ruff check --output-format=github python/
uv run --no-project ruff format --check python/
+ - name: Run ty
+ run: |
+ uv run --no-project ty check --output-format github
Review Comment:
This invocation likely bypasses the pinned `ty==0.0.84` declared in
`pyproject.toml` (because `--no-project` avoids using the project’s dependency
set). That undermines the stated goal of pinning `ty` to prevent new
diagnostics from breaking CI unexpectedly. Consider running `ty` from the
project environment (e.g., syncing/installing the `dev` group in CI and using
`uv run ty check ...` without `--no-project`), or explicitly running a pinned
tool version via uv so CI/pre-commit are deterministic.
##########
python/datafusion/functions/spark.py:
##########
@@ -56,14 +56,14 @@ def _filter_raw(filter: Expr | None) -> Any:
return filter.expr if filter is not None else None
-def _coerce_i32(value: Expr | int | None) -> Expr | None:
- """Coerce a native ``int`` to an int32 literal, passing ``Expr``/``None``
through.
+def _coerce_i32(value: Expr | int) -> Expr:
+ """Coerce a native ``int`` to an int32 literal, passing ``Expr`` through.
Several Spark datetime and interval builders require 32-bit integer
inputs, so a bare ``int`` must become an int32 literal rather than the
int64 default that :meth:`Expr.literal` would produce.
"""
Review Comment:
This change removes support for `None` (previously accepted and passed
through). If any Spark helper functions call `_coerce_i32(...)` with an
optional argument (common for builder-style APIs), this will now raise at
runtime. Either (a) keep the prior contract (`Expr | int | None -> Expr |
None`) and handle `None` explicitly, or (b) ensure all call sites validate
non-None before calling and update any public function signatures/docs
accordingly.
##########
python/datafusion/functions/spark.py:
##########
@@ -56,14 +56,14 @@ def _filter_raw(filter: Expr | None) -> Any:
return filter.expr if filter is not None else None
-def _coerce_i32(value: Expr | int | None) -> Expr | None:
- """Coerce a native ``int`` to an int32 literal, passing ``Expr``/``None``
through.
+def _coerce_i32(value: Expr | int) -> Expr:
+ """Coerce a native ``int`` to an int32 literal, passing ``Expr`` through.
Several Spark datetime and interval builders require 32-bit integer
inputs, so a bare ``int`` must become an int32 literal rather than the
int64 default that :meth:`Expr.literal` would produce.
"""
Review Comment:
This change removes support for `None` (previously accepted and passed
through). If any Spark helper functions call `_coerce_i32(...)` with an
optional argument (common for builder-style APIs), this will now raise at
runtime. Either (a) keep the prior contract (`Expr | int | None -> Expr |
None`) and handle `None` explicitly, or (b) ensure all call sites validate
non-None before calling and update any public function signatures/docs
accordingly.
##########
python/datafusion/context.py:
##########
@@ -1292,7 +1292,7 @@ def register_csv(
delimiter=delimiter,
schema_infer_max_records=schema_infer_max_records,
file_extension=file_extension,
- file_compression_type=file_compression_type,
+ file_compression_type=file_compression_type or "",
Review Comment:
Replacing `None` with an empty string changes behavior and can introduce
hard-to-debug failures if the Rust/internal layer treats `\"\"` as an invalid
compression type rather than 'unset/default'. Prefer passing `None` through if
the internal API supports it; otherwise, consider mapping `None` to the
explicit default value expected by the internal API (or
documenting/centralizing the sentinel handling) and rejecting `\"\"` when the
user explicitly supplies it.
--
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]
