timsaucer commented on code in PR #1759:
URL:
https://github.com/apache/datafusion-python/pull/1759#discussion_r4097715485
##########
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:
Added a positive check that each protocol imports from
`datafusion.extensions` (e1cdb64e, trimmed in cc3ff0c3). Declining the absence
assertions: re-exporting a name is additive and breaks no caller, so a test
that a name is *missing* only adds friction for a later deliberate export. Root
presence of `SessionExtensionComponents` is already pinned by `test_context.py`
importing it from `datafusion`.
##########
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:
Added `test_session_extension_components_is_keyword_only` in cc3ff0c3. It
fails when `kw_only=True` is removed.
##########
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:
Kept the promise and added the exception for writes a hook makes to the
context it is handed, linking to `extension_bundles_transaction`, where it is
already explained (cc3ff0c3).
##########
python/datafusion/extensions.py:
##########
@@ -57,13 +58,50 @@
)
__all__ = [
+ "PhysicalOptimizerRuleExportable",
Review Comment:
Fixed in e1cdb64e; `llms.txt` now lists `PhysicalOptimizerRuleExportable`.
--
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]
