apache / apache/datafusion-comet

Split the native `core` crate into focused per-concern crates (planner, expressions, readers, writers, storage, operators, shuffle, jni, proto, common)

Open
#5,639 6 comments 0 reactions 1 assignee Claimed by @comphead View on GitHub
Dominant language
Scala
Stars
1.3k
Forks
373
Avg merge
2d 4h
Merged PRs (30d)
198

Description

## Summary

Split the native `core` crate (~31k LOC) into focused, single-concern crates with a strict one-way, acyclic dependency graph. This issue is scoped to the **native Rust project structure and its dependencies only**. It does not change behavior, wire format, or the JVM side.

The target crates are `common`, `proto`, `jni-bridge`, `expr`, `storage`, `operators`, `readers`, `writers`, `shuffle`, and `planner`, with the existing `core` crate (the cdylib) slimmed down to the JNI entry layer.

## Problem

Today `native/core` fuses five concerns into one crate: the planner (a single 7141-line `planner.rs`), readers (parquet plus the scan operators), writers (parquet writer), storage (object store plus cloud credentials), and the JNI entry layer. Editing the Parquet reader recompiles the planner. There is no enforced dependency direction, so any part can reach into any other.

The other native crates (`proto`, `spark-expr`, `shuffle`, `jni-bridge`, `common`) are already separate and their graph is acyclic. The work is to break up `core`.

## Proposed native structure

Each new crate is carved out of today's `core`. The `core` crate keeps its name and remains the cdylib, but shrinks to only the JNI entry points and the memory pools.

| Crate | Holds (from today's `core`) | Depends on |
|---|---|---|
| `comet-common` | errors, schema, arrow convert, query context, tracing, metrics | leaf |
| `comet-proto` | generated protobuf types (exists) | leaf |
| `comet-jni-bridge` | Rust↔JVM callbacks: task mem mgr, metric node, s3 cred, udf bridge, arrow stream (exists) | common |
| `comet-expr` | `spark-expr` plus `execution/expressions/*` | common, jni-bridge |
| `comet-storage` | `parquet/objectstore/{s3,azure}`, `cloud/s3` | common, jni-bridge |
| `comet-operators` | expand, explode, projection, sample, rank_limit, copy | common, expr |
| `comet-readers` | parquet reader, csv/iceberg/shuffle scan, columnar_to_row, schema_adapter, cast_column | common, expr, storage |
| `comet-writers` | parquet_writer | common, storage |
| `comet-shuffle` | shuffle writer, partitioners, IPC (exists) | common, jni-bridge, expr |
| `comet-planner` | `planner.rs`, serde, spark_plan, registries | proto, common, expr, operators, readers, writers, shuffle, storage |
| `core` (cdylib) | `lib.rs`, `jni_api`, parquet JNI entry, memory_pools | planner, jni-bridge, proto |

## Dependency graph

```mermaid
graph TB
subgraph cdylib["cdylib"]
core["core: jni entry + memory_pools"]
end
subgraph assembler["assembler"]
planner["comet-planner"]
end
subgraph execution["execution"]
direction LR
readers["comet-readers"]
writers["comet-writers"]
operators["comet-operators"]
shuffle["comet-shuffle"]
end
subgraph blocks["building blocks"]
direction LR
expr["comet-expr"]
storage["comet-storage"]
end
subgraph ffi["ffi"]
jnibridge["comet-jni-bridge"]
end
subgraph foundation["foundation"]
direction LR
common["comet-common"]
proto["comet-proto"]
end

core --> planner
core --> jnibridge
core --> proto
planner --> proto
planner --> expr
planner --> operators
planner --> readers
planner --> writers
planner --> shuffle
planner --> storage
readers --> expr
readers --> storage
writers --> storage
operators --> expr
shuffle --> expr
expr --> jnibridge
storage --> jnibridge
shuffle --> jnibridge
jnibridge --> common
```

Topological order (proof it is acyclic): `common`, `proto` then `jni-bridge` then `expr`, `storage` then `operators`, `readers`, `writers`, `shuffle` then `planner` then `core`.

**The one rule that keeps it acyclic:** the proto-to-plan build logic lives in `comet-planner`. Content crates (`expr`, `operators`, `readers`, and the rest) never depend back on the planner.

## Granularity choice

This proposal splits execution into `readers` + `writers` + `operators` + `storage` (`shuffle` is already separate). The finer split gives better incremental builds and clearer ownership. If that turns out to be too many crates, collapsing `readers`/`writers`/`operators`/`storage` into one `comet-execution` crate is a cheap merge later, since the dependency direction is unchanged.

## Subtasks

Ordered for a dependency-safe rollout. Each ships with a green build and merges independently. Paths are under `native/core/src` unless noted. Do relocations as pure `git mv` commits so blame follows.

### T1. Consolidate shared types into `comet-common`
Moves:
- `native/jni-bridge/src/errors.rs` (`CometError`, `ExecutionError`, `ExpressionError`) into `comet-common`.
- `native/spark-expr/src/error.rs` (`SparkError`, `SparkErrorWithContext`) and `native/spark-expr/src/query_context.rs` (`QueryContext`) into `comet-common`.
- Repoint the ~20 files importing `crate::execution::operators::ExecutionError` and the `datafusion_comet_spark_expr::{SparkError, QueryContext}` importers.

Depends on: nothing. Result: `common` is the leaf, `jni-bridge -> common`.

### T2. Extract `comet-storage`
Moves:
- `parquet/objectstore/` (`mod.rs`, `s3.rs`, `azure.rs`, `s3_blob_fs_support.rs`).
- `cloud/` (`mod.rs`, `s3/mod.rs`, `s3/credential_bridge.rs`).
- The object-store builders from `parquet/parquet_support.rs`: `prepare_object_store_with_configs`, `create_hdfs_operator`, `is_hdfs_scheme`.

Depends on: T1. Result: `storage -> common, jni-bridge`.

### T3. Form `comet-expr`
Moves into the existing `spark-expr` crate (the expressions home):
- All of `execution/expressions/`: `arithmetic.rs`, `bitwise.rs`, `comparison.rs`, `logical.rs`, `nullcheck.rs`, `list_empty_to_null.rs`, `list_positions.rs`, `partition.rs`, `random.rs`, `strings.rs`, `subquery.rs`, `temporal.rs`, `mod.rs`.
- Only the `PhysicalExpr` impls move. The proto-to-expr `ExpressionBuilder` code that takes `&PhysicalPlanner` (in `arithmetic`, `partition`, `random`, `strings`, `temporal`) stays behind for T7.

Depends on: T1. Result: `expr -> common, jni-bridge`.

### T4. Extract `comet-operators`
Moves from `execution/operators/`:
- `expand.rs`, `explode.rs`, `projection.rs`, `sample.rs`, `rank_limit.rs`, `copy.rs`, and their wiring in `mod.rs`.
- The `OperatorBuilder`/`PhysicalPlanner` coupling in `projection.rs` stays behind for T7.

Depends on: T3. Result: `operators -> common, expr`.

### T5. Extract `comet-writers`
Moves from `execution/operators/`:
- `parquet_writer.rs`, `iceberg_write.rs`.

Depends on: T2. Result: `writers -> common, storage`.

### T6. Extract `comet-readers`
Moves:
- From `parquet/`: `parquet_exec.rs`, `parquet_support.rs` (reader remainder after T2), `schema_adapter.rs`, `cast_column.rs`, `encryption_support.rs`, `eager_page_index_reader_factory.rs`, `name_fold.rs`, `util/` (`jni.rs`, `mod.rs`).
- From `execution/operators/`: `scan.rs`, `csv_scan.rs`, `iceberg_scan.rs`, `iceberg_common.rs`, `shuffle_scan.rs`, `aligned_stream_reader.rs`.
- From `execution/`: `columnar_to_row.rs`.
- The parquet JNI entry in `parquet/mod.rs` (the `#[no_mangle]` methods and `BatchContext`) stays in `core` for T8. Only reader logic moves.

Depends on: T2, T3. Result: `readers -> common, expr, storage`.

### T7. Extract `comet-planner`
Moves from `execution/`:
- `planner.rs`, `planner/` (`delta_scan.rs`, `expression_registry.rs`, `macros.rs`, `operator_registry.rs`), `serde.rs`, `spark_plan.rs`, plus the plan-construction helpers `sort.rs` and `merge_as_partial.rs`.
- The proto-to-expr and proto-to-plan builder impls pulled back from T3 and T4.

Depends on: T2, T3, T4, T5, T6 (`shuffle` already exists). Result: `planner` depends only downward.

### T8. Slim `core` to the JNI entry
Keeps in `core`: `lib.rs`, `execution/jni_api.rs`, the parquet JNI entry in `parquet/mod.rs`, `execution/memory_pools/`, `execution/spark_config.rs`, `debug/`.

Depends on: T7. Result: `core -> planner, jni-bridge, proto`.

## Cross-cutting

`execution/metrics/`, `execution/tracing.rs`, and the arrow-conversion helpers in `execution/utils.rs` move into `comet-common` where they are dependency-free, otherwise they stay in `core`. These can ride along with T1 or T8.

## Parallelization

- Prereq: T1 alone.
- Wave A (after T1): T2 and T3 in parallel.
- Wave B: T4 (needs T3), T5 (needs T2), T6 (needs T2 and T3), in parallel.
- Then T7, then T8.

Because each part is its own crate, Rust's crate system rejects any dependency cycle at compile time, so a stray back-edge fails the build rather than slipping through.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.