Skip to content

Renaming Internal Structs #1059

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 11 commits into from
Mar 14, 2025
Merged

Renaming Internal Structs #1059

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
80611e6
Renamed Expr to RawExpr
Spaarsh Mar 11, 2025
3c15d55
Fixed CI test for exported classes to include RawExpr as well
Spaarsh Mar 11, 2025
8affdc0
Fixed CI test for exported classes to check if Expr class covers RawExpr
Spaarsh Mar 11, 2025
2529109
Generalized Raw* class checking
Spaarsh Mar 11, 2025
42daa1b
fixes
Spaarsh Mar 12, 2025
f995cb8
fixes
Spaarsh Mar 12, 2025
4200a39
fixed the CI test to not look for Raw classes in the datafusion module
Spaarsh Mar 13, 2025
580ecd5
Add additional text to unit test describing operation and ensure wrap…
timsaucer Mar 14, 2025
03d3398
New ruff rule on main
timsaucer Mar 14, 2025
5eebc45
Merge branch 'main' into 853/enhancement/renaming-rust-exposed-structs
timsaucer Mar 14, 2025
a669bcf
Resolve ruff errors
timsaucer Mar 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions python/datafusion/expr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -193,7 +193,7 @@ class Expr:
:ref:`Expressions` in the online documentation for more information.
"""

def __init__(self, expr: expr_internal.Expr) -> None:
def __init__(self, expr: expr_internal.RawExpr) -> None:
"""This constructor should not be called by the end user."""
self.expr = expr

Expand Down Expand Up @@ -383,7 +383,7 @@ def literal(value: Any) -> Expr:
value = pa.scalar(value, type=pa.string_view())
if not isinstance(value, pa.Scalar):
value = pa.scalar(value)
return Expr(expr_internal.Expr.literal(value))
return Expr(expr_internal.RawExpr.literal(value))

@staticmethod
def string_literal(value: str) -> Expr:
Expand All @@ -398,13 +398,13 @@ def string_literal(value: str) -> Expr:
"""
if isinstance(value, str):
value = pa.scalar(value, type=pa.string())
return Expr(expr_internal.Expr.literal(value))
return Expr(expr_internal.RawExpr.literal(value))
return Expr.literal(value)

@staticmethod
def column(value: str) -> Expr:
"""Creates a new expression representing a column."""
return Expr(expr_internal.Expr.column(value))
return Expr(expr_internal.RawExpr.column(value))

def alias(self, name: str) -> Expr:
"""Assign a name to the expression."""
Expand Down
9 changes: 9 additions & 0 deletions python/tests/test_wrapper_coverage.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,13 @@ def missing_exports(internal_obj, wrapped_obj) -> None:
for attr in dir(internal_obj):
if attr in ["_global_ctx"]:
continue

# Check if Raw* classes have corresponding wrapper classes
elif attr.startswith("Raw"):
base_class = attr[3:] # Remove "Raw" prefix
assert hasattr(wrapped_obj, base_class)
continue

assert attr in dir(wrapped_obj)

internal_attr = getattr(internal_obj, attr)
Expand All @@ -51,6 +58,8 @@ def missing_exports(internal_obj, wrapped_obj) -> None:
if isinstance(internal_attr, list):
assert isinstance(wrapped_attr, list)
for val in internal_attr:
if isinstance(val, str) and val.startswith("Raw"):
continue
Copy link
Member

@timsaucer timsaucer Mar 12, 2025

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@Spaarsh Spaarsh Mar 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member

@timsaucer timsaucer Mar 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Spaarsh Spaarsh Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Spaarsh Spaarsh Mar 12, 2025

Choose a reason for hiding this comment

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

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

Copy link
Member

@timsaucer timsaucer Mar 12, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Spaarsh Spaarsh Mar 12, 2025

Choose a reason for hiding this comment

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

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.

assert val in wrapped_attr
elif hasattr(internal_attr, "__dict__"):
missing_exports(internal_attr, wrapped_attr)
Expand Down
2 changes: 1 addition & 1 deletion src/expr.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ pub mod window;
use sort_expr::{to_sort_expressions, PySortExpr};

/// A PyExpr that can be used on a DataFrame
#[pyclass(name = "Expr", module = "datafusion.expr", subclass)]
#[pyclass(name = "RawExpr", module = "datafusion.expr", subclass)]
#[derive(Debug, Clone)]
pub struct PyExpr {
pub expr: Expr,
Expand Down