Skip to content

feat: Add Lance contrib build gate - #5728

Merged
sunchao merged 1 commit into
apache:mainfrom
wirybeaver:xuanyili/lance-build-gate
Sep 13, 2026
Merged

feat: Add Lance contrib build gate#5728
sunchao merged 1 commit into
apache:mainfrom
wirybeaver:xuanyili/lance-build-gate

Conversation

@wirybeaver

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Part of #4632.

Rationale for this change

Native Lance reads need an optional contrib boundary that does not add Lance dependencies or runtime behavior to default Comet builds. This separates the build and planner wiring from the later native reader implementation, following the existing Delta build-gate pattern.

What changes are included in this PR?

  • Adds an optional comet-contrib-lance crate and contrib-lance Cargo feature.
  • Adds the typed Lance scan payload and a feature-gated native planner dispatch.
  • Adds an inert Lance planner entry point that returns NotImplemented, allowing Spark fallback until the real reader lands.
  • Does not depend on the Lance Rust crate and does not change default Comet runtime behavior.

The native Lance reader prototype in #4633 will be rebased on top of this PR.

How are these changes tested?

  • cargo check -p datafusion-comet --locked
  • cargo check -p datafusion-comet --features contrib-lance --locked
  • cargo check -p datafusion-comet --no-default-features --locked
  • cargo check -p datafusion-comet --no-default-features --features contrib-lance --locked
  • cargo clippy -p datafusion-comet --no-default-features --features contrib-lance --locked -- -D warnings
  • cargo fmt --all -- --check
  • git diff --check

No runtime test is added because this PR deliberately introduces no executable Lance read path; the feature-gated planner returns NotImplemented.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correctness

Reviewed 2a99f468ab35ce47be332476597b10b835a825b8 against base f97fb4519cad2a4f6fe435d66aa17eacbd919a9b, using the PR delta from merge base c348f775d5b20d91d1989756b72f62f366d67d32. No verified P1/P2 issue found in this build-boundary change.

Previously there was no Lance-specific native entry point. This adds an optional comet-contrib-lance dependency, a contrib-lance feature, three protobuf messages, and a matching native dispatcher. Both the module and dispatcher call are feature-gated. The crate depends only on the existing DataFusion 55.0.0 and Comet protobuf packages; the lockfile adds the local crate and its core dependency edge without changing external packages. The protobuf addition leaves existing definitions and field numbers unchanged.

Existing Spark scan selection, expression semantics, null handling, numeric boundaries and error modes are unchanged: there is no JVM Lance producer or service registration, and the reader remains a stub. No version-specific maintained Spark implementation is changed or needed for that comparison; this does not establish Spark 3.4/3.5/4.0/4.1 runtime compatibility for a future Lance reader.

The stub's NotImplemented result becomes GeneralError, propagates through native executePlan and is rethrown by CometExecIterator. It is an error path, not automatic Spark fallback. Because this PR has no JVM producer or registration that emits a Lance scan, it does not change existing Spark scan selection.

Validation

Local checks passed for the exact diff, formatting of all changed Rust files, protobuf descriptor compilation, dependency paths, static feature activation with defaults on/off, and lockfile isolation. These do not type-check or link the native crate. The PR description reports four cargo check combinations and one Clippy invocation; I did not independently run those builds or any JNI/Spark tests.

At the final GitHub CI check on 2026-09-05 23:58:18 UTC, there were no head checks and four Actions required approval. There were also no merge checks in the supplied snapshot. The existing CI and Delta gate do not enable contrib-lance, so default CI would not establish feature-enabled compilation. No workflow execution is credited to this review.

Performance

The default feature set does not reach the optional Lance crate, and the new native dispatch is absent unless the feature is enabled. No Lance reader dependency, I/O, or per-row work is introduced. With the feature enabled, the added work is confined to contrib plan dispatch and payload/schema handling before the deliberate stub error. There is no executable read path or performance claim to benchmark here; no runtime speed or binary-size measurement was made.

Design

The split follows the existing Delta boundary: core decodes the envelope and converts Spark schema types, while the contrib owns format-specific planning and returns a DataFusion execution plan. This avoids a contrib-to-core dependency cycle. Keeping the reader out of this preparatory change makes the default-runtime claim easy to inspect. The typed payload reserves the data needed by a later reader without changing the existing generic ContribScan envelope.

Abstraction & complexity

The change adds one small crate and one narrow bridge, reusing the existing dispatcher rather than introducing another registration mechanism. The unused planner parameter is consistent with the current contrib dispatch signature. I found no new abstraction that requires simplification in this scope.

@andygrove andygrove added enhancement New feature or request build Build environment area:scan Parquet scan / data reading labels Sep 6, 2026

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Correctness

This is a CI follow-up on unchanged head 2a99f468ab35ce47be332476597b10b835a825b8 and base f97fb4519cad2a4f6fe435d66aa17eacbd919a9b. The existing approval still stands. There are no new P1/P2 source findings or changes to Spark semantics or fallback behavior.

The title check now executed and failed because Add Lance contrib build gate lacks the required prefix. Please use type: message or type(scope): message, for example feat: Add Lance contrib build gate. This is a title-format issue and does not change the source assessment.

The Spark 4.1 build job also failed. Native compilation completed, but finalizing the native-library artifact returned HTTP 403. Spark setup and JVM test-class compilation were then skipped. Both completed logs identify merge f89f0e59f920c965e51a21b4ecb921f40235de63, whose parents match the reviewed base and head. Two later jobs also failed on artifact-service HTTP 403 responses. The Spark 3.5 SQL shard could not list/download its JVM artifact and skipped its tests. The Spark 3.5 execution job reported 766 tests passing and zero failing before artifact finalization failed. Those logs identify the same executed merge. Broader CI remains unfinished as of 2026-09-08T16:56:39 UTC. These results do not establish feature-enabled Lance compilation or runtime correctness.

Performance

The source is unchanged, so this update adds no allocation, copying or runtime overhead. No new benchmark evidence or measured speedup is claimed.

Design

The previous assessment of the optional build boundary remains unchanged. The title-format correction is a PR metadata update. The artifact-upload failure provides no new evidence against that design.

Abstraction & complexity

No new abstraction or indirection was introduced after the approval. These CI results do not justify additional source machinery, and there is no new complexity finding.

@wirybeaver wirybeaver changed the title Add Lance contrib build gate feat: Add Lance contrib build gate Sep 10, 2026
@wirybeaver

Copy link
Copy Markdown
Contributor Author

@sunchao The PR title is fixed.

@sunchao

sunchao commented Sep 11, 2026

Copy link
Copy Markdown
Member

@wirybeaver could you rebase the PR? after the CI passed I'll merge it

@wirybeaver
wirybeaver force-pushed the xuanyili/lance-build-gate branch from 2a99f46 to 6c4a4d3 Compare September 12, 2026 20:16
@wirybeaver

Copy link
Copy Markdown
Contributor Author

@sunchao Rebased just a while ago.

@sunchao sunchao left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Re-reviewed 6c4a4d3ec95838602a1dc8bdd23eb222fd13bc17 against de1eb4f86c12af0895784c93e1f152c705f6ef0e after the rebase and title correction. No new or remaining verified P1/P2 findings; the existing approval still stands.

The nine-file contribution has the same added and removed lines as the earlier revision; the four newly added Lance files are byte-identical. The other changes come from the newer base. The lockfile now resolves DataFusion 55.1.0 from that base; Lance's 55.0.0 requirement permits it, and this contribution adds no external lockfile packages. The optional feature boundary and inert reader stub are unchanged. The title now matches the repository's conventional-title format.

Exact-source, feature-reachability, lockfile and protobuf-preservation checks passed. Protobuf descriptor compilation also passed. These checks do not establish Rust compilation, linkage, JNI behavior or feature-enabled Lance execution; I did not run those builds or tests.

As of 2026-09-12 23:17:10 UTC, CI, CodeQL and the Delta gate require approval and have no jobs. Only labeling has passed. The previous revision's test results and artifact-service failures do not validate this rebased head. Fresh default and contrib-lance build validation remains outstanding; the current workflows do not enable contrib-lance.

@sunchao
sunchao added this pull request to the merge queue Sep 13, 2026
Merged via the queue into apache:main with commit e893b43 Sep 13, 2026
66 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:scan Parquet scan / data reading build Build environment enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants