comphead opened a new issue, #5639:
URL: https://github.com/apache/datafusion-comet/issues/5639
## Summary
Comet's native `core` crate has grown into a ~31k-line monolith that fuses
five distinct concerns (planner, readers, writers, storage, and the JNI entry
layer). This makes rebuilds slow, keeps a single 7141-line `planner.rs` at the
center of the codebase, and blurs the boundaries that the JVM side already
expresses cleanly through packages. This issue proposes reorganizing both the
Rust and JVM sides around one shared vocabulary of seven axes, with a strictly
downward dependency flow modeled on Apache Spark's module layout.
This is a structural refactor. It changes no behavior.
## Motivation
1. **`core` is a monolith.** Five concerns live in one crate. Editing the
Parquet reader recompiles the planner. The planner itself is one 7141-line file
doing four jobs (expression building, operator building, file-scan parsing,
shuffle-writer config).
2. **Concerns are named on the JVM but not on the native side.** The JVM
already separates `rules` (planner), `serde` (expressions), `parquet`
(readers), objectstore/cloud (storage), shuffle (writers), and `Native` (jni)
by package. The native side does not, so the same concept has no shared name
across the language boundary.
3. **Three layering inversions block clean extraction.**
- Core error types (`CometError`, `ExecutionError`) live in `jni-bridge`,
so all error handling transitively depends on JNI.
- `SparkError` and `QueryContext` live in the `spark-expr` crate, making
it a de-facto foundation crate that lower layers reach up into.
- Expressions have two homes: the `spark-expr` crate and
`core/execution/expressions/`.
4. **A decomposition is already in flight.** `ExpressionRegistry` and
`OperatorRegistry` (with `macros.rs`) let expressions and operators
self-register instead of living in a giant match. Only `projection` and
`arithmetic` are migrated. This registry is the mechanism to finish the split
without the planner needing a hard dependency on every operator.
## Current native structure
```mermaid
graph TD
core["comet (core): ~31k LOC<br/>planner + readers + writers + storage +
jni-entry + runtime"]
core --> proto[comet-proto]
core --> common[comet-common]
core --> jnibridge[comet-jni-bridge]
core --> sparkexpr[spark-expr]
core --> shuffle[comet-shuffle]
sparkexpr --> jnibridge
sparkexpr -. "error/context types live here (inversion)" .-> common
jnibridge -. "error types live here (inversion)" .-> common
```
## Goals
- One shared axis vocabulary across native and JVM.
- A strictly acyclic, downward dependency flow.
- Shrink `core` to a thin JNI and orchestration shell.
- Finish the registry-based decomposition already started.
## Non-goals
- No behavior or wire-format changes.
- No broad split of the JVM `spark` module into Maven modules. The
multi-version source layout (`spark-3.4` through `spark-4.2` plus shims) makes
that too costly. Package discipline is used instead.
## Shared vocabulary: one name per axis, on both sides
| Axis | Native crate (target) | JVM package (target) |
|---|---|---|
| proto | `comet-proto` (exists) | generated into `o.a.comet.proto` |
| expressions | `comet-expr` (`spark-expr` plus
`core/execution/expressions`) | `o.a.comet.serde` and `o.a.comet.expressions` |
| planner | `comet-planner` (new, from `planner.rs`) | `o.a.comet.rules` and
`o.a.spark.sql.comet` execs |
| readers | `comet-readers` (new) | `o.a.spark.sql.comet` scan execs and
`o.a.comet.parquet` |
| writers | `comet-writers` and `comet-shuffle` (shuffle exists) |
`o.a.spark.sql.comet.execution.shuffle` and write execs |
| storage | `comet-storage` (new) | `o.a.comet.objectstore` and
`o.a.comet.cloud.s3` |
| jni | `comet` shell and `comet-jni-bridge` (exists) |
`o.a.comet.{Native,NativeBase}` and `vector.NativeUtil` |
## Target native crate graph
Arrows mean "depends on". Foundation sits at the bottom, the JNI shell at
the top. `core` shrinks to the shell.
```mermaid
graph TD
shell["comet (cdylib)<br/>JNI entry points, memory_pools runtime"]
planner["comet-planner<br/>PhysicalPlanner, serde, spark_plan, registries"]
readers["comet-readers<br/>parquet / csv / iceberg scan, columnar_to_row"]
writers["comet-writers<br/>parquet write"]
shuffle["comet-shuffle<br/>shuffle write, partitioners"]
operators["comet-operators<br/>expand, explode, projection, sample,
rank_limit, copy"]
expr["comet-expr<br/>all Spark-compatible expressions"]
storage["comet-storage<br/>object_store build, S3/Azure, credential glue"]
jnibridge["comet-jni-bridge<br/>Rust to JVM callbacks"]
common["comet-common<br/>errors, schema, arrow-convert, query_context,
tracing, metrics"]
proto["comet-proto<br/>generated protobuf"]
shell --> planner
shell --> jnibridge
planner --> proto
planner --> expr
planner --> readers
planner --> writers
planner --> shuffle
planner --> operators
planner --> storage
readers --> expr
readers --> storage
writers --> storage
shuffle --> expr
operators --> expr
expr --> jnibridge
storage --> jnibridge
jnibridge --> common
expr --> common
readers --> common
writers --> common
storage --> common
operators --> common
```
## Target JVM package dependency direction
No Maven split. Enforce the direction below with one ArchUnit or Scalafix
dependency test. Grow the near-empty `common` module into a contract tier (like
Spark's `sql/api`) that holds the `Native` signatures, `DataTypeSupport`, and
the exception classes.
```mermaid
graph TD
planner["rules + Comet*Exec<br/>PLANNER"]
serde["serde + expressions<br/>EXPRESSIONS"]
readers["scan execs<br/>READERS"]
writers["shuffle + write execs<br/>WRITERS"]
storage["objectstore + cloud.s3<br/>STORAGE"]
jni["Native / NativeBase / NativeUtil<br/>JNI"]
proto["generated protobuf<br/>PROTO"]
common["common: CometConf, Native contract, exceptions<br/>CONTRACT TIER"]
planner --> serde
planner --> readers
planner --> writers
serde --> proto
readers --> storage
writers --> storage
readers --> jni
writers --> jni
storage --> jni
serde --> common
planner --> common
jni --> common
```
## Cross-cutting residuals (need an explicit home)
- Native: `metrics`, `tracing`, `utils` move into `comet-common`.
`memory_pools` stays in the shell because it is executor runtime bound to the
task-memory-manager callback.
- JVM: the Arrow `vector` layer, the `codegen` batch-kernel path, Arrow and
Python interop, metrics, and testing infra. The largest is the version-shim
tree, which is the strongest argument against a broad Maven split. Give it a
`shims` bucket per axis rather than a module.
## Migration plan
Each phase ships with a green build and can merge independently. Do every
file relocation as a pure `git mv` commit with no logic change so blame follows
and review stays trivial.
- [ ] **Phase 1: fix the layering inversions.** Move `CometError` and
`ExecutionError` from `jni-bridge` into `comet-common`. Move `SparkError` and
`QueryContext` from `spark-expr` into `comet-common`.
- [ ] **Phase 2: extract `comet-storage`.** Move
`parquet/objectstore/{s3,azure}` and `cloud/s3`.
- [ ] **Phase 3: extract `comet-readers` and `comet-writers`.** Split the
`operators/` grab-bag along the read/write line. Move the pure transforms into
`comet-operators`.
- [ ] **Phase 4: consolidate expressions.** Fold
`core/execution/expressions/` into `comet-expr`.
- [ ] **Phase 5: extract `comet-planner`.** Split `planner.rs` by concern
(expression building, operator building, file-scan and partition parsing,
shuffle-writer config) as it moves. Finish the `ExpressionRegistry` and
`OperatorRegistry` migration.
- [ ] **Phase 6: reduce `core` to the JNI shell.**
- [ ] **Phase 7 (JVM): adopt the axis vocabulary and add the
dependency-direction enforcement test.** Consolidate the two known overlaps
(`serde/operator/*` serializes both scans and sinks, scan execs double as
operator wrappers). Extract `comet-storage` and proto as modules only if the
boundary holds.
## Risks and tradeoffs
- More Rust crates means a larger build graph. In exchange, incremental
rebuilds shrink and the 7141-line file problem disappears. Net win for a
31k-line monolith.
- Cyclic dependencies are a real risk. Keep dependencies one-way (planner to
building blocks), which the existing registry supports.
- A broad JVM Maven split fights the version-shim structure and is out of
scope.
- Large moves churn open PRs. The `git mv`-only commit rule keeps this
manageable.
- Note for consistency: the JVM side generates protobuf at build time, while
the native side commits `native/proto/src/generated/*.rs`. Spark commits
neither. Reconciling this is optional follow-up.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
