Renaming Internal Structs #1059

Spaarsh · 2025-03-10T19:51:20Z

Which issue does this PR close?

Closes #853

Rationale for this change

Developers were getting confusing debugging errors such as "Cannot convert Expr to Expr". Hence, the internal struct Expr was renamed to RawExpr for better understanding.

What changes are included in this PR?

The internal struct Expr was renamed to RawExpr.

Are there any user-facing changes?

None. It only changes the internal structs.

timsaucer · 2025-03-10T20:04:27Z

src/expr.rs

+// Define the new RawExpr struct and implement Debug trait
+#[derive(Debug, Clone)]
+pub struct RawExpr {
+    pub expr: Expr,
+}
+
+// Implement conversion from RawExpr to Expr
+impl From<RawExpr> for Expr {
+    fn from(raw_expr: RawExpr) -> Expr {
+        raw_expr.expr
+    }
+}
+
 /// A PyExpr that can be used on a DataFrame
 #[pyclass(name = "Expr", module = "datafusion.expr", subclass)]
 #[derive(Debug, Clone)]
 pub struct PyExpr {
-    pub expr: Expr,
+    pub raw_expr: RawExpr,
 }


I don't think we need another wrapper struct. Rather, I think you need to change the pyo3 class name to RawExpr.

#[pyclass(name = "RawExpr", module = "datafusion.expr", subclass)]

This will probably require updates to the CI script that checks for all classes that require exporting.

You should also be able to test the debug error by going into any of the methods in functions.py and remove the .expr to get the inner object and see the error generated is updated.

Okay, got it. I'll revert this and proceed as you suggested. I'll start with changing RawExpr and the corresponding CI tests and get that reviewed again.

Yep I am getting this error:

Traceback (most recent call last): File "/home/user/datafusion-python/rename.py", line 3, in <module> result = f.isnan(col("a")) File "/home/user/datafusion-python/python/datafusion/functions.py", line 284, in isnan return Expr(f.isnan(expr)) TypeError: argument 'num': 'Expr' object cannot be converted to 'RawExpr'

When I try to run:

from datafusion import col, functions as f result = f.isnan(col("a")) print(result)

Spaarsh · 2025-03-11T17:43:16Z

I'll be renaming all the classes in _internal here:

datafusion-python/src/lib.rs

Line 77 in 9d634de

fn _internal(py: Python, m: Bound<'_, PyModule>) -> PyResult<()> {

I hope that's correct. The comment above it states this for the classes in _internal: "Low-level DataFusion internal package.
The higher-level public API is defined in pure python files under the datafusion directory."

timsaucer · 2025-03-11T17:56:15Z

I'll be renaming all the classes in _internal here:

You'll want to get the submodules as well.

Also, this can be done piecemeal if doing the entire thing in one go is too large a PR.

The CI failure is the one I expected. We will need to update the check for export coverage in the wrappers to have this specific naming convention. Ideally, we would even require the Raw prefix once all of the classes are complete. But to manage the work, it likely makes sense to first get in one PR with the RawExpr handled and CI working, then iterate through the remaining classes, then update the CI check to require the Raw prefix.

Spaarsh · 2025-03-11T18:00:31Z

Sure. I'll fix that CI test right away.

About breaking this into several PRs, I agree. There are small changes required in different places for each renaming. One single PR would make things a lot confusing in case some future dev tries to understand what all changes were made.

timsaucer · 2025-03-11T19:26:40Z

python/datafusion/expr.py

@@ -86,6 +86,7 @@
 Partitioning = expr_internal.Partitioning
 Placeholder = expr_internal.Placeholder
 Projection = expr_internal.Projection
+RawExpr = expr_internal.RawExpr


I don't think we want this in the wrapper. Instead we want to update the CI test to identify that RawExpr is properly covered by the Expr class in this file.

Something along these lines?

# Skip internal context and RawExpr (which is handled by Expr class) if attr in ["_global_ctx", "RawExpr"]: continue # Check if RawExpr functionality is covered by Expr class if attr == "RawExpr" and hasattr(wrapped_obj, "Expr"): expr_class = getattr(wrapped_obj, "Expr") assert hasattr(expr_class, "raw_expr"), "Expr class must provide raw_expr property" continue

timsaucer · 2025-03-11T19:50:35Z

python/tests/test_wrapper_coverage.py

+        # Skip internal context and RawExpr (which is handled by Expr class)
+        if attr in ["_global_ctx", "RawExpr"]:
+            continue
+
+        # Check if RawExpr functionality is covered by Expr class
+        if attr == "RawExpr" and hasattr(wrapped_obj, "Expr"):
+            expr_class = getattr(wrapped_obj, "Expr")
+            assert hasattr(expr_class, "raw_expr"), "Expr class must provide raw_expr property"


Instead of special casing it like this, we should be able to write a more general solution that checks all class names. For the internal representation they can begin with Raw and be converted to their non-raw counterparts.

Yes I am sorry. I thought that I could include that after renaming other classes. I am now simply extracting the base_class name from the Raw attr name via string slicing and then use it to check if the base_class has such an attribute:

if attr in ["_global_ctx"]: continue # Check if Raw* classes have corresponding wrapper classes if attr.startswith("Raw"): base_class = attr[3:] # Remove "Raw" prefix assert hasattr(wrapped_obj, base_class), f"Missing implementation for {attr}" continue

Sorry for being this clumsy. Thanks a lot for your patience.

timsaucer · 2025-03-11T20:56:31Z

python/datafusion/expr.py

@@ -102,6 +102,7 @@

 __all__ = [
    "Expr",
+    "RawExpr",


I think removing this caused the test to fail. I'll add this again and run the tests locally again.

timsaucer · 2025-03-11T20:57:07Z

python/datafusion/expr.py

        """This constructor should not be called by the end user."""
        self.expr = expr

+    @property 
+    def raw_expr(self) -> expr_internal.RawExpr:


I don't think we need this.

timsaucer · 2025-03-11T20:57:45Z

python/tests/test_wrapper_coverage.py

@@ -36,7 +36,12 @@ def missing_exports(internal_obj, wrapped_obj) -> None:
    for attr in dir(internal_obj):
        if attr in ["_global_ctx"]:
            continue
-        assert attr in dir(wrapped_obj)
+


If you apply ruff it should correct these whitespace errors. I recommend turning on pre-commit in your local workspace to catch them before they get to CI.

timsaucer · 2025-03-11T20:59:51Z

python/tests/test_wrapper_coverage.py

@@ -36,7 +36,12 @@ def missing_exports(internal_obj, wrapped_obj) -> None:
    for attr in dir(internal_obj):
        if attr in ["_global_ctx"]:
            continue
-        assert attr in dir(wrapped_obj)


Why remove this assert?

Residual of a previous thing I was trying. Fixed.

timsaucer · 2025-03-12T11:17:18Z

It's looking good. As long as CI passes, I plan to merge.

Spaarsh changed the title ~~Renamed Expr to RawExpr~~ Renaming Internal Structs Mar 10, 2025

Spaarsh marked this pull request as draft March 10, 2025 19:54

timsaucer reviewed Mar 10, 2025

View reviewed changes

Renamed Expr to RawExpr

80611e6

Spaarsh force-pushed the 853/enhancement/renaming-rust-exposed-structs branch from 27ef6d3 to 80611e6 Compare March 11, 2025 14:29

Fixed CI test for exported classes to include RawExpr as well

3c15d55

Spaarsh marked this pull request as ready for review March 11, 2025 19:10

timsaucer requested changes Mar 11, 2025

View reviewed changes

Fixed CI test for exported classes to check if Expr class covers RawExpr

8affdc0

timsaucer requested changes Mar 11, 2025

View reviewed changes

Generalized Raw* class checking

2529109

timsaucer requested changes Mar 11, 2025

View reviewed changes

fixes

42daa1b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Renaming Internal Structs #1059

Renaming Internal Structs #1059

Spaarsh commented Mar 10, 2025 •

edited

Loading

timsaucer Mar 10, 2025

Spaarsh Mar 11, 2025

Spaarsh Mar 11, 2025

Spaarsh commented Mar 11, 2025

timsaucer commented Mar 11, 2025

Spaarsh commented Mar 11, 2025

timsaucer Mar 11, 2025

Spaarsh Mar 11, 2025

timsaucer Mar 11, 2025

Spaarsh Mar 11, 2025 •

edited

Loading

timsaucer Mar 11, 2025

Spaarsh Mar 12, 2025

timsaucer Mar 11, 2025

timsaucer Mar 11, 2025

timsaucer Mar 11, 2025

Spaarsh Mar 12, 2025

timsaucer commented Mar 12, 2025

Renaming Internal Structs #1059

Are you sure you want to change the base?

Renaming Internal Structs #1059

Conversation

Spaarsh commented Mar 10, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spaarsh commented Mar 11, 2025

timsaucer commented Mar 11, 2025

Spaarsh commented Mar 11, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Spaarsh Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timsaucer commented Mar 12, 2025

Spaarsh commented Mar 10, 2025 •

edited

Loading

Spaarsh Mar 11, 2025 •

edited

Loading