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
timsaucer merged 11 commits into from
Mar 14, 2025
+45 −20
Merged

Renaming Internal Structs #1059

Show file tree
Hide file tree
Changes from 2 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
10 changes: 6 additions & 4 deletions python/datafusion/expr.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@
Partitioning = expr_internal.Partitioning
Placeholder = expr_internal.Placeholder
Projection = expr_internal.Projection
RawExpr = expr_internal.RawExpr
Copy link
Member

@timsaucer timsaucer Mar 11, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@Spaarsh Spaarsh Mar 11, 2025

Choose a reason for hiding this comment

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

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

Repartition = expr_internal.Repartition
ScalarSubquery = expr_internal.ScalarSubquery
ScalarVariable = expr_internal.ScalarVariable
Expand All @@ -102,6 +103,7 @@

__all__ = [
"Expr",
"RawExpr",
Copy link
Member

@timsaucer timsaucer Mar 11, 2025

Choose a reason for hiding this comment

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

To remove.

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.

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

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.

Its removal is causing the tests to fail.

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.

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

"Column",
"Literal",
"BinaryExpr",
Expand Down Expand Up @@ -193,7 +195,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 +385,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 +400,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
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