Skip to content

Conversation

@scovich
Copy link
Collaborator

@scovich scovich commented Nov 22, 2024

What changes are proposed in this pull request?

Similar to the existing SchemaTransform trait, an ExpressionTransform trait can make it a lot easier to recursively manipulate expressions. Define the trait and introduce an expression depth checker to test it.

This PR affects the following public APIs

Because enum tuple variants cannot be passed as function arguments, we factor out UnaryExpression, BinaryExpression and VariadicExpression structs which the corresponding expression variants can then use. This requires updating match arms throughout the code base.

How was this change tested?

New unit test.

Comment on lines +133 to +135
pub left: Box<Expression>,
/// The right-hand side of the operation.
pub right: Box<Expression>,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

NOTE: While it would be tempting to do Box<BinaryExpression> instead of two boxes here, that would prevent match arms from unpacking it. So we keep the boxes inside the struct, and out of the way.

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Nov 22, 2024
@codecov
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

Attention: Patch coverage is 73.04348% with 62 lines in your changes missing coverage. Please review.

Project coverage is 80.61%. Comparing base (ac8dcdc) to head (6bc9876).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kernel/src/expressions/mod.rs 72.76% 52 Missing and 6 partials ⚠️
ffi/src/expressions/kernel.rs 0.00% 3 Missing ⚠️
kernel/src/engine/arrow_expression.rs 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #530      +/-   ##
==========================================
- Coverage   80.76%   80.61%   -0.15%     
==========================================
  Files          67       67              
  Lines       14076    14278     +202     
  Branches    14076    14278     +202     
==========================================
+ Hits        11368    11510     +142     
- Misses       2134     2188      +54     
- Partials      574      580       +6     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@zachschuermann zachschuermann changed the title Define an ExpressionTramsform trait Define an ExpressionTransform trait Nov 26, 2024
Copy link
Collaborator

@nicklan nicklan left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@zachschuermann zachschuermann left a comment

Choose a reason for hiding this comment

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

LGTM just left a quick question and perhaps we want to consider moving much of the new transform code to a new module?

Comment on lines +600 to +603
depth_limit: usize,
max_depth_seen: usize,
current_depth: usize,
call_count: usize,
Copy link
Member

Choose a reason for hiding this comment

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

out of curiosity, why usize instead of u64 etc.? I always associate usize as the type for indexing into memory, and feel like I would have just defaulted to u64 here (or even something smaller)?

Copy link
Collaborator Author

@scovich scovich Nov 27, 2024

Choose a reason for hiding this comment

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

Not sure about rust, but in c++ size_t (usize equivalent) is the general purpose count/size type.
For example, Iterator::count returns usize.

Copy link
Member

Choose a reason for hiding this comment

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

thanks!

@scovich scovich merged commit 953ceed into delta-io:main Nov 27, 2024
18 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

breaking-change Change that require a major version bump

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants