Skip to content

[wip] split out default engine into separate crate #941

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

Draft
wants to merge 15 commits into
base: main
Choose a base branch
from

Conversation

zachschuermann
Copy link
Collaborator

What changes are proposed in this pull request?

Splitting out the default engine into a separate crate. This highlighted a number of places where the default engine is tightly coupled with the kernel: namely, violating orphan rule, conflating Error types, using private types.

TODO: list major components

Follow-ups

  1. Naming?
  2. Engine fails to compile if you don’t have arrow-55 or arrow-54 (but we have a default)
  3. Dev dependency on entire delta-kernel-engine (and arrow) - future just do arrow for sync engine?
  4. Specific errors for traits (get kernel error out of engine)
  5. Don’t use EngineResult in acceptance tests (or in examples, anywhere) - migrate to anyhow? Color eyre?
  6. Separate out create_one tests from arrow_expression
  7. Re-enable HDFS test
  8. Move read.rs/write.rs

Future work

TODO

This PR affects the following public APIs

Lots, TODO

How was this change tested?

rely on existing

@github-actions github-actions bot added the breaking-change Change that require a major version bump label May 9, 2025
@zachschuermann zachschuermann added the merge hold Don't allow the PR to merge label May 16, 2025
zachschuermann added a commit that referenced this pull request May 24, 2025
…e else (#957)

## What changes are proposed in this pull request?
Again prompted by the crate split exploration (#941). Notice that the
`SyncEngine` is not really useful to external users, instead just adding
to our public API and complicating our feature flags. We expect users to
reach for the default engine in the case they want a 'prebuilt' solution
with minimal deps and sane defaults (arrow, tokio). The `SyncEngine`
will continue to exist as a test-only engine (for now; and will maybe
evolve in the future).

This PR includes
1. removing `sync-engine` feature flag (in core kernel and FFI), and
making the existing `engine::sync` module `#[cfg(test)]`
2. removing any consumption of the sync engine in (1) examples and (2)
integration tests
3. adding a simple test_util trait `DefaultEngineExtension ` and a
simple method to return a new local default engine (to more easily mimic
SyncEngine API for easy replacement)

### This PR affects the following public APIs
- no more `SyncEngine`, no more `sync-engine` feature flag

## How was this change tested?
Existing
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 merge hold Don't allow the PR to merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant