Copilot commented on code in PR #1759:
URL:
https://github.com/apache/datafusion-python/pull/1759#discussion_r4097334947
##########
python/datafusion/__init__.py:
##########
@@ -92,12 +92,7 @@
)
from .dataframe_formatter import configure_formatter
from .expr import Expr, WindowFrame
-from .extensions import (
- QueryPlannerExportable,
- SessionComponentsExportable,
- SessionExtensionComponents,
- SessionPlannerExportable,
-)
+from .extensions import SessionExtensionComponents
Review Comment:
Replace the deleted positive export test with coverage for the new boundary:
assert these protocols are absent from `datafusion.__all__` and the package
attributes, remain available from `datafusion.extensions`, and
`SessionExtensionComponents` remains at the root. Otherwise this PR's narrowed
public surface can be accidentally widened again without any test failing.
##########
python/datafusion/extensions.py:
##########
@@ -110,7 +148,7 @@ def _not_a_codec_iterable(field: str, value: object) -> str:
)
-@dataclass(frozen=True)
+@dataclass(frozen=True, kw_only=True)
Review Comment:
Please add Python coverage that positional construction raises `TypeError`
while keyword construction succeeds. This keyword-only constructor is a new
public API guarantee, but every current test already passes keywords, so
removing `kw_only=True` would leave the suite green.
##########
docs/source/contributor-guide/ffi-internals.md:
##########
@@ -111,6 +111,53 @@ library would serialize, and would do it with the codecs
it was imported with.
The extension-facing consequence — install codecs before a layered planner, and
prefer `with_extensions` — is documented at {ref}`planner_codec_rebinding`.
+(ffi_internals_commit_order)=
+
+## Why `with_extensions` commits last
+
+`with_extensions` promises that a bundle which raises leaves the session as it
+was. Keeping that promise is an ordering constraint on the implementation, not
+a property of any one step, because the planner is bound on the shared
+`SessionState` rather than on the returned handle.
Review Comment:
“A bundle which raises” is broader than the actual guarantee: a hook can
register a table and then raise, and that mutation is explicitly not rolled
back. Scope this promise to failures in the declarative component-installation
path so the contributor invariant agrees with the documented exception.
##########
python/datafusion/extensions.py:
##########
@@ -57,13 +58,50 @@
)
__all__ = [
+ "PhysicalOptimizerRuleExportable",
Review Comment:
The supported `docs/source/llms.txt` integration point still enumerates only
the previous four `datafusion.extensions` names. Add
`PhysicalOptimizerRuleExportable` there so agent-facing documentation reflects
this newly exported protocol.
--
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]
