Skip to content

Conversation

kevinzwang
Copy link
Member

Changes Made

Reverts the changes in #4998 for v0.6 after further discussion.

Related Issues

Checklist

  • Documented in API Docs (if applicable)
  • Documented in User Guide (if applicable)
  • If adding a new documentation page, doc is added to docs/mkdocs.yml navigation
  • Documentation builds and is formatted properly (tag @/ccmao1130 for docs review)

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Greptile Summary

This PR reverts changes from #4998 that introduced an explicit eval method for UDFs (User Defined Functions) decorated with @daft.func. The revert restores the previous behavior where UDFs automatically detect argument types and execute appropriately: when called with literal arguments (non-Expression objects), they execute immediately and return the actual result; when called with Expression objects, they return a Daft Expression for use in DataFrame operations.

The changes span the UDF module's core implementation and test files. In row_wise.py and generator.py, the standalone eval methods are removed and the eager evaluation logic is restored to the __call__ method using get_expr_args() to detect if any arguments are Expression objects. When len(expr_args) == 0, the UDF executes immediately via self._inner(*args, **kwargs). Proper type overloads are added to support both return types (T for eager execution, Expression for lazy execution).

The test files are updated to remove .eval() method calls, demonstrating that UDFs now work directly with literal arguments (e.g., my_stringify_and_sum(1, 2) instead of my_stringify_and_sum.eval(1, 2)). The docstring in __init__.py is updated to clarify this dual behavior pattern.

This change integrates with Daft's expression system by maintaining the automatic detection mechanism that allows the same UDF to work seamlessly in both eager Python execution contexts and lazy DataFrame expression contexts, providing better API ergonomics and backward compatibility.

Confidence score: 4/5

  • This PR is safe to merge with low risk as it reverts to well-tested previous behavior
  • Score reflects straightforward revert with comprehensive test coverage and clear implementation
  • Pay attention to UDF implementation files (row_wise.py, generator.py) to ensure overload signatures are correct

5 files reviewed, no comments

Edit Code Review Bot Settings | Greptile

@kevinzwang kevinzwang changed the title featrevert daft.func behavior on literal arguments feat!: revert daft.func behavior on literal arguments Aug 29, 2025
@kevinzwang kevinzwang enabled auto-merge (squash) August 29, 2025 22:19
@kevinzwang kevinzwang merged commit 1379d46 into main Aug 29, 2025
97 of 100 checks passed
@kevinzwang kevinzwang deleted the kevin/undo-eval branch August 29, 2025 23:32
Copy link

codecov bot commented Aug 29, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.07%. Comparing base (ae638e5) to head (767a5a8).
⚠️ Report is 22 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #5087      +/-   ##
==========================================
+ Coverage   75.27%   76.07%   +0.79%     
==========================================
  Files         949      950       +1     
  Lines      132520   130512    -2008     
==========================================
- Hits        99761    99293     -468     
+ Misses      32759    31219    -1540     
Files with missing lines Coverage Δ
daft/udf/__init__.py 96.00% <ø> (ø)
daft/udf/generator.py 88.88% <100.00%> (ø)
daft/udf/row_wise.py 86.53% <100.00%> (ø)

... and 18 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Jay-ju pushed a commit to Jay-ju/Daft that referenced this pull request Sep 1, 2025
)

## Changes Made

Reverts the changes in Eventual-Inc#4998 for v0.6 after further discussion.

## Related Issues

<!-- Link to related GitHub issues, e.g., "Closes Eventual-Inc#123" -->

## Checklist

- [x] Documented in API Docs (if applicable)
- [x] Documented in User Guide (if applicable)
- [x] If adding a new documentation page, doc is added to
`docs/mkdocs.yml` navigation
- [x] Documentation builds and is formatted properly (tag @/ccmao1130
for docs review)
venkateshdb pushed a commit to venkateshdb/Daft that referenced this pull request Sep 6, 2025
)

## Changes Made

Reverts the changes in Eventual-Inc#4998 for v0.6 after further discussion.

## Related Issues

<!-- Link to related GitHub issues, e.g., "Closes Eventual-Inc#123" -->

## Checklist

- [x] Documented in API Docs (if applicable)
- [x] Documented in User Guide (if applicable)
- [x] If adding a new documentation page, doc is added to
`docs/mkdocs.yml` navigation
- [x] Documentation builds and is formatted properly (tag @/ccmao1130
for docs review)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants