Copilot commented on code in PR #1532:
URL:
https://github.com/apache/datafusion-python/pull/1532#discussion_r3254795490
##########
python/datafusion/expr.py:
##########
@@ -150,6 +150,9 @@
TransactionStart = expr_internal.TransactionStart
TryCast = expr_internal.TryCast
Union = expr_internal.Union
+HigherOrderFunction = expr_internal.HigherOrderFunction
+Lambda = expr_internal.Lambda
+LambdaVariable = expr_internal.LambdaVariable
Review Comment:
These new public expression wrapper exports are not added to the existing
import/module coverage in `python/tests/test_imports.py`, which explicitly
imports and checks the module for the other `datafusion.expr` classes. Adding
them there would catch regressions in the wrapper export path and `__module__`
metadata.
##########
Cargo.toml:
##########
@@ -71,3 +71,12 @@ codegen-units = 2
# We cannot publish to crates.io with any patches in the below section.
Developers
# must remove any entries in this section before creating a release candidate.
[patch.crates-io]
+datafusion = { git = "https://github.com/apache/datafusion";, rev =
"47655fd6c9ef060d73497987e6ccb98e57196508" }
Review Comment:
The PR description says these patches pin DataFusion to commit
`3d06bedcc9afbd65781ac1de28741c36140d2cbb`, but the manifest and lockfile pin
`47655fd6c9ef060d73497987e6ccb98e57196508`. Please update the PR description or
the pinned revision so reviewers and release notes identify the actual upstream
commit being tested.
##########
python/datafusion/functions.py:
##########
@@ -273,6 +274,7 @@
"percent_rank",
"percentile_cont",
"pi",
+ "position",
Review Comment:
Since this change is meant to expose `position` through `from
datafusion.functions import *`, please add a regression test for the
star-export/`__all__` behavior. Existing tests only verify direct submodule
imports for a couple of functions, so this exact export path could regress
unnoticed.
##########
python/datafusion/dataframe.py:
##########
@@ -523,6 +523,32 @@ def select_exprs(self, *args: str) -> DataFrame:
"""
return self.df.select_exprs(*args)
+ def alias(self, alias: str) -> DataFrame:
Review Comment:
This adds a new public `DataFrame.alias()` API, but there is no
corresponding Python test exercising qualifier-based selection/self-join
behavior. The repository already has dataframe API coverage under
`python/tests/test_dataframe.py`; adding a regression test would guard the
intended alias semantics and the Python/Rust binding path.
##########
python/datafusion/functions.py:
##########
@@ -184,6 +184,7 @@
"ifnull",
"in_list",
"initcap",
+ "instr",
Review Comment:
Since this change is meant to expose `instr` through `from
datafusion.functions import *`, please add a regression test for the
star-export/`__all__` behavior. Existing tests only verify direct submodule
imports for a couple of functions, so this exact export path could regress
unnoticed.
##########
python/datafusion/__init__.py:
##########
@@ -133,6 +138,8 @@
"SessionContext",
"Table",
"TableFunction",
+ "TableProviderFactory",
+ "TableProviderFactoryExportable",
Review Comment:
The new top-level exports are not covered by the existing import tests, even
though this change is specifically about making these classes reachable from
`datafusion`. Please add an import/star-export regression test so a future
`__all__` or re-export change does not silently drop them again.
--
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]
