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

 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


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


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

Its removal is causing the tests to fail.

i added a line in the test:

if isinstance(internal_attr, list): assert isinstance(wrapped_attr, list) for val in internal_attr: if isinstance(val, str) and val.startswith("Raw"): #<---- added this line since we check for all Raw* classes in the previous part of the code continue assert val in wrapped_attr

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

python/datafusion/expr.py

        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

        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.

python/tests/test_wrapper_coverage.py

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

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

Spaarsh · 2025-03-12T12:12:43Z

Now the test_wrapper_coverage.py checks for modules in this way:

Skip checks if it is global_context (No changes made in this part)
For classes started with Raw, it breaks down the class name to extract the wrapper class and checks if that class has the given Raw class
Later, when the conditional statement of isinstance is checked for, the Raw classes are skipped since they have been checked for in the earlier part of the test and, more importantly, unlike other classes that return true for isinstance, Raw classes are only internally exposed.

timsaucer · 2025-03-12T13:38:46Z

python/tests/test_wrapper_coverage.py

+                if isinstance(val, str) and val.startswith("Raw"):
+                    continue


Don't we need an assert here to make certain this exposed class is in the list of the wrapper?

That's being covered in the previous part of the test. Here:

elif attr.startswith("Raw"): base_class = attr[3:] # Remove "Raw" prefix assert hasattr(wrapped_obj, base_class)

Though I think I should change the variable name from base_class to wrapper_class. Perhaps I should move the aforementioned part into the isinstance conditionional block.

The previous part catches that the class is exposed, but this portion of the test was, if my memory is correct, to ensure that everything that is in __all__ in the internal is also in the wrapper. Possibly I need to document all of the reasons for the portions of that test.

The 'RawExpr' isn't in __all__ as you had suggested not to in the comment here. So now the isinstance block only checks for the non-raw classes, essentially the wrapper classes like Expr.

Possibly I need to document all of the reasons for the portions of that test.

That would be helpful. I've added comments next to the new sections as well.

This is more cohesive and well-structured, I have already ran the Ci test for this locally and it works, will push it if you approve this:

for attr in dir(internal_obj): if attr in ["_global_ctx"] or attr.startswith("Raw"): #<---Skip Raw classes for now continue [...] if isinstance(internal_attr, list): assert isinstance(wrapped_attr, list) for val in internal_attr: if isinstance(val, str) and val.startswith("Raw"): #<---check for Raw classes and their corresponding wrapper classes here # Check if Raw* classes have corresponding wrapper classes wrapper_class = val[3:] # Remove "Raw" prefix assert hasattr(wrapped_obj, wrapper_class), f"Missing wrapper class: {wrapper_class} for Raw class: {val}" continue assert val in wrapped_attr

When you say that you tested it locally and it works, have you verified that it also fails when it is supposed to fail? I won't have time to review this again today, but I hope to get to it tomorrow.

Actually, I did not fail when I removed Expr from the __all__. So essentially, if some wrapper class is not included in __all__ even then its Raw class shall pass the test. To prevent this, I added the assert wrapper_class in dir(internal_obj) before the assert hasattr(wrapped_obj, wrapper_class), f"Missing wrapper class: {wrapper_class} for Raw class: {val}"

Now, if the wrapper_class isn't included in the internal_obj, then the corresponding Raw shall fail:

FAILED python/tests/test_wrapper_coverage.py::test_datafusion_missing_exports - AssertionError: Missing wrapper class: Expr for Raw class: RawExpr

I won't have time to review this again today, but I hope to get to it tomorrow.

Sure. I'll see if I can find out any other gaps in my implementation till then.

…ped Raw classes are checked

timsaucer · 2025-03-14T12:56:27Z

Thank you for all the work on this.

Spaarsh · 2025-03-14T12:57:55Z

Thank you for all the work on this.

I wasn't of much help in the tests part. Thanks for your patience!!

Spaarsh · 2025-03-14T13:03:32Z

@timsaucer any suggestions on what other internal class to rename next? If I am correct, all the classes in _internal here should be renamed next:

datafusion-python/src/lib.rs

Line 77 in 9d634de

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

and their corresponding submodules.

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

fixes

f995cb8

timsaucer reviewed Mar 12, 2025

View reviewed changes

Spaarsh and others added 5 commits March 14, 2025 02:05

fixed the CI test to not look for Raw classes in the datafusion module

4200a39

Add additional text to unit test describing operation and ensure wrap…

580ecd5

…ped Raw classes are checked

New ruff rule on main

03d3398

Merge branch 'main' into 853/enhancement/renaming-rust-exposed-structs

5eebc45

Resolve ruff errors

a669bcf

timsaucer merged commit 55141ba into apache:main Mar 14, 2025
17 checks passed

Renaming Internal Structs #1059

Renaming Internal Structs #1059

Uh oh!

Conversation

Spaarsh commented Mar 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spaarsh commented Mar 11, 2025

Uh oh!

timsaucer commented Mar 11, 2025

Uh oh!

Spaarsh commented Mar 11, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spaarsh Mar 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

timsaucer commented Mar 12, 2025

Uh oh!

Spaarsh commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Spaarsh Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

timsaucer commented Mar 14, 2025

Uh oh!

Uh oh!

Spaarsh commented Mar 14, 2025

Uh oh!

Spaarsh commented Mar 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Spaarsh commented Mar 10, 2025 •

edited

Loading

Spaarsh Mar 11, 2025 •

edited

Loading

Spaarsh commented Mar 12, 2025 •

edited

Loading

Spaarsh Mar 12, 2025 •

edited

Loading