refactor: de-duplicate small helpers in the DataFrame/Expr API#223
Open
nielspardon wants to merge 1 commit into
Open
refactor: de-duplicate small helpers in the DataFrame/Expr API#223nielspardon wants to merge 1 commit into
nielspardon wants to merge 1 commit into
Conversation
Follow-up cleanup from the review of substrait-io#204 (DRY, no behavior change): consolidate four small helpers in the substrait.dataframe / substrait.narwhals layer that re-implemented logic living elsewhere, so the two copies cannot drift apart. - Extension merge: replace expr._merge_extensions_into (which deduped on strict proto equality) with a shared utils.merge_extensions_into that reuses merge_extension_urns / merge_extension_declarations, keying on the same anchor/name identity as builders.plan._merge_extensions. - Sort direction: drop the duplicate (descending, nulls_last) -> SortDirection mapping; keep a single _SORT_DIRECTIONS + sort_direction() in dataframe.expr, used by both dataframe.expr and dataframe.frame. - Multi-URN lookup: add ExtensionRegistry.find_function(name, signature, urns), a generalization of lookup_function / list_functions_across_urns over an ordered URN subset, so _resolve_over_urns resolves in one registry call and recovers the winning extension via FunctionEntry.urn instead of looping. - Alias: extract the referred_expr[0].output_names[0] rewrite into a shared builders.extended_expression.alias, used by both Expr.alias and the Narwhals expression wrapper. Also replace the empty Exception("") in merge_extension_declarations with an informative NotImplementedError that names the unsupported mapping type. Closes substrait-io#208
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up cleanup from the review of #204 (DRY, no behavior change). Four small helpers in the
substrait.dataframe/substrait.narwhalslayer re-implemented logic that already existed elsewhere; this consolidates each onto a single source of truth so the copies cannot drift apart.Changes
dataframe.expr._merge_extensions_into(which deduped on strict proto equality) with a sharedutils.merge_extensions_intothat reusesmerge_extension_urns/merge_extension_declarations, keying on the same anchor/name identity asbuilders.plan._merge_extensions. Used byExpr.order_byandExpr.over.(descending, nulls_last) -> SortDirectionmapping. A single_SORT_DIRECTIONS+sort_direction()now lives indataframe.exprand is used by bothdataframe.expranddataframe.frame.ExtensionRegistry.find_function(name, signature, urns=None), a generalization oflookup_function(one URN) andlist_functions_across_urns(all URNs) over an ordered URN subset._resolve_over_urnsnow resolves in one registry call and recovers the winning extension viaFunctionEntry.urninstead of loopinglookup_functionper URN.referred_expr[0].output_names[0]rewrite into a sharedbuilders.extended_expression.alias, used by bothExpr.aliasand the Narwhals expression wrapper.Also replaces the empty
Exception("")inmerge_extension_declarationswith an informativeNotImplementedErrornaming the unsupported mapping type.Notes
The extension-merge and multi-URN changes are behavior-preserving: extension anchors are assigned deterministically per URN/overload and declaration names embed the full overload signature, so the identity-based dedup and the previous proto-equality dedup agree on all real inputs, and
find_functionselects the same winning URN as the previous per-URN loop.Testing
pytest: 528 passed, 30 skippedruff checkandruff format --check: cleanmerge_extensions_into(identity dedup, multiple sources) andfind_function(single-URN parity withlookup_function, candidate-order priority, no-match, all-URNs).Closes #208
🤖 Generated with AI