Skip to content

refactor: de-duplicate small helpers in the DataFrame/Expr API#223

Open
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:worktree-issue-208-dedupe-api-helpers
Open

refactor: de-duplicate small helpers in the DataFrame/Expr API#223
nielspardon wants to merge 1 commit into
substrait-io:mainfrom
nielspardon:worktree-issue-208-dedupe-api-helpers

Conversation

@nielspardon

Copy link
Copy Markdown
Member

Follow-up cleanup from the review of #204 (DRY, no behavior change). Four small helpers in the substrait.dataframe / substrait.narwhals layer re-implemented logic that already existed elsewhere; this consolidates each onto a single source of truth so the copies cannot drift apart.

Changes

  • Extension merge — replace dataframe.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. Used by Expr.order_by and Expr.over.
  • Sort direction — drop the duplicate (descending, nulls_last) -> SortDirection mapping. A single _SORT_DIRECTIONS + sort_direction() now lives in dataframe.expr and is used by both dataframe.expr and dataframe.frame.
  • Multi-URN lookup — add ExtensionRegistry.find_function(name, signature, urns=None), a generalization of lookup_function (one URN) and list_functions_across_urns (all URNs) over an ordered URN subset. _resolve_over_urns now resolves in one registry call and recovers the winning extension via FunctionEntry.urn instead of looping lookup_function per URN.
  • 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 replaces the empty Exception("") in merge_extension_declarations with an informative NotImplementedError naming 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_function selects the same winning URN as the previous per-URN loop.

Testing

  • pytest: 528 passed, 30 skipped
  • ruff check and ruff format --check: clean
  • New unit tests for merge_extensions_into (identity dedup, multiple sources) and find_function (single-URN parity with lookup_function, candidate-order priority, no-match, all-URNs).

Closes #208

🤖 Generated with AI

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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

De-duplicate small helpers in substrait.api (extension merge, sort direction, multi-URN lookup, alias)

1 participant