Skip to content

refactor!: Remove get_ prefix from engine getters #804

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.

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 2 commits into from
Apr 7, 2025

Conversation

scovich
Copy link
Collaborator

@scovich scovich commented Apr 3, 2025

What changes are proposed in this pull request?

Rust doesn't encourage the get_ prefix for getters because it's redundant and anyway a getter is allowed to have the same name as the field it exposes. Remove the prefix from the various engine interfaces. Additionally, rename get_evaluator as new_expression_evaluator to accurately reflect that it is NOT a getter at all, but actually creates a new expression evaluator.

Finally, we also rename ExpressionHandler to EvaluationHandler because that trait is used to create expression evaluators, not expressions. Additionally, future work will differentiate generic "expressions" from "predicates" (boolean-valued expressions with special evaluation semantics), and that will likely necessitate defining a EvaluationHandler::new_predicate_evaluator method alongside the new_expression_evaluator method.

This PR affects the following public APIs

All the methods and traits we change are public.

How was this change tested?

Rename-only operation, no functional changes. Compilation suffices.

Copy link

codecov bot commented Apr 3, 2025

Codecov Report

Attention: Patch coverage is 91.13924% with 7 lines in your changes missing coverage. Please review.

Project coverage is 84.67%. Comparing base (2287455) to head (87bf4e9).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
kernel/src/transaction.rs 66.66% 3 Missing ⚠️
ffi/src/engine_funcs.rs 88.88% 1 Missing ⚠️
kernel/src/scan/mod.rs 50.00% 1 Missing ⚠️
kernel/src/scan/state.rs 75.00% 1 Missing ⚠️
kernel/src/table_changes/log_replay.rs 83.33% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #804      +/-   ##
==========================================
- Coverage   84.67%   84.67%   -0.01%     
==========================================
  Files          83       83              
  Lines       19785    19780       -5     
  Branches    19785    19780       -5     
==========================================
- Hits        16753    16748       -5     
  Misses       2213     2213              
  Partials      819      819              

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Collaborator

@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 though I didn't check for exhaustiveness (grep for old strings).

One question: I'd prefer to normalize the FileSystemClient with the other *Handler abstractions. Perhaps we should rename this to StorageHandler? (happy to open a separate PR - just thought of it as I was reviewing this)

@github-actions github-actions bot added the breaking-change Change that require a major version bump label Apr 3, 2025
@scovich
Copy link
Collaborator Author

scovich commented Apr 3, 2025

I didn't check for exhaustiveness (grep for old strings).

I did. Hopefully I even did it correctly! Lots of doc comments updated, at least.

I'd prefer to normalize the FileSystemClient with the other *Handler abstractions. Perhaps we should rename this to StorageHandler?

Interesting thought. Part of me thinks we should go ahead and make the change here, so all the breaking changes land at once.

How 'bout this -- I'll make the change as a separate commit in this PR. Easy revert if others squawk.

@zachschuermann
Copy link
Collaborator

I didn't check for exhaustiveness (grep for old strings).

I did. Hopefully I even did it correctly! Lots of doc comments updated, at least.

I'd prefer to normalize the FileSystemClient with the other *Handler abstractions. Perhaps we should rename this to StorageHandler?

Interesting thought. Part of me thinks we should go ahead and make the change here, so all the breaking changes land at once.

How 'bout this -- I'll make the change as a separate commit in this PR. Easy revert if others squawk.

sounds great! I'm also happy to open a separate one

@scovich
Copy link
Collaborator Author

scovich commented Apr 3, 2025

I'd prefer to normalize the FileSystemClient with the other *Handler abstractions. Perhaps we should rename this to StorageHandler?

Interesting thought. Part of me thinks we should go ahead and make the change here, so all the breaking changes land at once.
How 'bout this -- I'll make the change as a separate commit in this PR. Easy revert if others squawk.

sounds great! I'm also happy to open a separate one

Hmm, that change is actually bigger than this whole PR. Maybe better to have a separate PR after all. But I can go ahead and post it as stacked on this one, since I already did the work to learn this.

Copy link
Collaborator

@roeap roeap left a comment

Choose a reason for hiding this comment

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

Just making sure as I am not a native speaker ... :).

The rename ExpressionHandler --> EvaluationHandler is motivated by the fact that we also have the null_row API now, which does not actually handle an expression, but still evaluates something?

@scovich
Copy link
Collaborator Author

scovich commented Apr 7, 2025

Just making sure as I am not a native speaker ... :).

The rename ExpressionHandler --> EvaluationHandler is motivated by the fact that we also have the null_row API now, which does not actually handle an expression, but still evaluates something?

That, and also because very soon engines will need to evaluate predicates and not just expressions (see PR description):

future work will differentiate generic "expressions" from "predicates" (boolean-valued expressions with special evaluation semantics), and that will likely necessitate defining a EvaluationHandler::new_predicate_evaluator method alongside the new_expression_evaluator method.

(it would read a bit weird to have ExpressionHandler::new_predicate_evaluator?)

@scovich scovich force-pushed the no-engine-getter-get branch from 67a8b52 to 87bf4e9 Compare April 7, 2025 20:03
@scovich scovich merged commit 4ad2bc6 into delta-io:main Apr 7, 2025
21 checks passed
scovich added a commit that referenced this pull request Apr 7, 2025
## What changes are proposed in this pull request?

Continuing the cleanup started by
#804, rename the class
to eliminate the last vestiges of (ancient) "client" nomenclature.

### This PR affects the following public APIs

The renamed class (and module) are public.

## How was this change tested?

Rename-only operation, no functional changes. Compilation suffices.
zachschuermann pushed a commit to hntd187/delta-kernel-rs that referenced this pull request Apr 8, 2025
## What changes are proposed in this pull request?

Continuing the cleanup started by
delta-io#804, rename the class
to eliminate the last vestiges of (ancient) "client" nomenclature.

### This PR affects the following public APIs

The renamed class (and module) are public.

## How was this change tested?

Rename-only operation, no functional changes. Compilation suffices.
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