feat: Add pow expression #5237
feat: Add pow expression #5237
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Summary
This PR implements power/exponentiation functionality (pow and power) across the Daft library stack. The implementation adds support for computing base^exponent operations through several layers:
Core Array Operations: New pow.rs module in daft-core implements power operations for Float32Array and Float64Array using native powf() methods, with automatic casting of integer types to floats.
Function Registration: The Pow struct is registered in the numeric functions module (daft-functions) as a scalar UDF that handles type checking and ensures the exponent is a scalar value.
Python API: Both pow() and power() methods are added to the Series class and Expression class, providing dual naming conventions for compatibility. The type stub file is updated to include the new PySeries.pow() method.
Spark Connect Integration: The pow function is registered in the math module with both 'pow' and 'power' aliases for Spark compatibility.
The implementation follows established patterns in the codebase by using the scalar UDF framework, proper type coercion (casting integers to Float64), and maintaining Float32 precision where possible. The function accepts two arguments where the exponent must be a scalar float value, which is consistent with common mathematical operation design patterns.
Confidence score: 1/5
- This PR has critical implementation issues that will cause runtime failures and inconsistent behavior
- Score reflects multiple serious bugs including parameter mismatches, incorrect argument parsing, and duplicate conflicting registrations
- Pay close attention to
src/daft-functions/src/numeric/pow.rs,daft/expressions/expressions.py, andsrc/daft-connect/src/functions/math.rs
8 files reviewed, 13 comments
| parent.add_fn(Log10); | ||
| parent.add_fn(Ln); | ||
| parent.add_fn(Log1p); | ||
| parent.add_fn(Pow); |
There was a problem hiding this comment.
logic: The pow function is registered in Rust but missing from the Python daft/functions/numeric.py module and daft/functions/__init__.py, making it inaccessible to users.
|
Fixing up the PR and then adding tests |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5237 +/- ##
==========================================
+ Coverage 74.24% 74.60% +0.36%
==========================================
Files 973 976 +3
Lines 125213 124558 -655
==========================================
- Hits 92959 92925 -34
+ Misses 32254 31633 -621
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The latest changes have completely refactored the pow and power expression implementations to follow the established architectural pattern used throughout the Daft codebase.
The key changes include:
-
Expression methods refactor: The
powandpowermethods inExpressionclass now delegate todaft.functions.powanddaft.functions.powerrespectively, following the same pattern as other mathematical operations likesqrt,exp, andlog. This eliminates the previous direct native function calls and addresses all parameter inconsistencies from earlier reviews. -
Functions module implementation: Added
powandpowerfunctions todaft/functions/numeric.pythat call the builtin scalar function "pow". Both functions are identical and properly exported throughdaft/functions/__init__.py. -
Rust backend modernization: The Rust implementation has been updated to use generic
DataArray<T>withDaftFloatTypetrait bounds instead of separate Float32/Float64 implementations. The functions module now uses the modern#[derive(FunctionArgs)]proc macro pattern. -
Testing: Added representation tests for both
powandpowermethods that verify correct string output and deep copy behavior. -
Spark Connect cleanup: Removed pow function registration from Spark Connect integration, marking them as TODO items since the functionality is now handled at the core expression level.
This architectural change centralizes mathematical operations in the core expression system rather than having them scattered across integration layers, ensuring consistent behavior across all entry points. The implementation now matches the established patterns used by other mathematical functions in the codebase.
Confidence score: 2/5
- This PR has critical parameter ordering inconsistency that will cause runtime errors
- Score reflects serious API design flaw where functions module expects (base, exp) but Expression methods pass (self, exp)
- Pay close attention to the function parameter ordering mismatch between
daft/functions/numeric.pyand the Rust implementation
Context used:
Rule - Import statements should be placed at the top of the file rather than inline within functions or methods. (link)
9 files reviewed, 2 comments
| def pow(base: Expression, expr: Expression) -> Expression: | ||
| """The base^expr of a numeric expression.""" | ||
| return Expression._call_builtin_scalar_fn("pow", base, expr) | ||
|
|
||
|
|
||
| def power(base: Expression, expr: Expression) -> Expression: | ||
| """The base^expr of a numeric expression.""" | ||
| return Expression._call_builtin_scalar_fn("pow", base, expr) |
There was a problem hiding this comment.
style: Both pow and power functions have identical docstrings and implementations. Consider differentiating the docstrings or adding a note about their relationship for clarity.
| let s = self.cast(&DataType::Float64)?; | ||
| Ok(s.f64()?.pow(exp)?.into_series()) | ||
| } | ||
| DataType::Float32 => Ok(self.f32()?.pow(exp as f32)?.into_series()), |
There was a problem hiding this comment.
style: Casting f64 to f32 may cause precision loss for the exponent when working with Float32 series
1 participant
Changes Made
Related Issues
#4704
Checklist
docs/mkdocs.ymlnavigation