Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
10 changes: 3 additions & 7 deletions .ai/skills/datafusion-ffi/SKILL.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
name: datafusion-ffi
description: Patterns and review checklist for the `datafusion-ffi` crate. Use whenever the user adds, edits, or reviews code under `datafusion/ffi/` — new `FFI_X` wrappers, `ForeignX` implementations, codec changes, or expanding an existing wrapper to cover more of a trait's surface. Also use when reviewing PRs that touch this crate.
description: Patterns and review checklist for the `datafusion-ffi` crate. Use whenever the user adds, edits, or reviews code under `datafusion/ffi/` — new `FFI_X` wrappers, `Foreign<X>` impls, codec changes, or expanding an existing wrapper to cover more of a trait's surface. Also use when reviewing PRs that touch this crate.
---

# DataFusion FFI Skill
Expand Down Expand Up @@ -210,11 +210,9 @@ cargo test -p datafusion-ffi --features integration-tests
To add coverage for a new wrapper:

1. **Add a constructor** in `src/tests/<area>.rs` (or a new file there). Return a populated `FFI_X` from a known-good native type.
2. **Wire it into `ForeignLibraryModule`** in `src/tests/mod.rs`: add a field of type `extern "C" fn(...) -> FFI_X` and populate it in `datafusion_ffi_get_module`. This struct is the cross-library contract. Adding a field is itself an ABI change for the test module; integration tests will rebuild the cdylib automatically.
2. **Wire it into `ForeignLibraryModule`** in `src/tests/mod.rs`: add a field of type `extern "C" fn(...) -> FFI_X` and populate it in `datafusion_ffi_get_module`. This struct is the cross-library contract — adding a field is itself an ABI change for the test module; integration tests will rebuild the cdylib automatically.
3. **Add the test** in `tests/ffi_<area>.rs` under `#[cfg(feature = "integration-tests")] mod tests { … }`. Call `datafusion_ffi::tests::utils::get_module()` to load the cdylib, invoke your constructor through the returned `ForeignLibraryModule`, convert into `Arc<dyn X>`, and exercise every method.

When adding a method to an existing wrapper, reuse an existing fixture and constructor when a small trait-method override can cover it. This applies both to omitted default methods and methods newly added to the trait. Add a dedicated test type or `ForeignLibraryModule` field only when the existing fixtures cannot cover the method.

What integration tests catch that unit tests cannot:

- **Real ABI layout bugs.** Two builds means the consumer's view of `FFI_X` is reconstructed from declaration, not aliased to the producer's memory. Mismatched alignment, padding, niche optimization, or accidentally non-`#[repr(C)]` types surface here.
Expand All @@ -237,7 +235,7 @@ What integration tests catch that unit tests cannot:
- Primitives (`u8`/`u64`/`bool`/`usize`, etc.) and `#[repr(u8)]` FFI enums (`FFI_TableType`, `Volatility`, `InsertOp`, `TableProviderFilterPushDown`).
- A `stabby::string::String` (`SString`) returned by value, with no other args or returns.

Concrete skippable example: `fn name(&self) -> SString` reading a field already validated by another method. Concrete *non*-skippable examples: anything returning `SVec<T>`, `FFI_Option<T>`, `FFI_Result<T>`, `WrappedSchema`, `WrappedArray`, an `FfiFuture`, an `FFI_*` sub-struct, or any `*mut`/`*const` pointer. These exercise alignment / padding / niche-opt across the ABI boundary and need the two-build coverage. When unsure, write the integration test; the cost is one constructor + ~20 lines.
Concrete skippable example: `fn name(&self) -> SString` reading a field already validated by another method. Concrete *non*-skippable examples: anything returning `SVec<T>`, `FFI_Option<T>`, `FFI_Result<T>`, `WrappedSchema`, `WrappedArray`, an `FfiFuture`, an `FFI_*` sub-struct, or any `*mut`/`*const` pointer — those exercise alignment / padding / niche-opt across the ABI boundary and need the two-build coverage. When unsure, write the integration test; the cost is one constructor + ~20 lines.

If you skip the integration test for a layout change, you have effectively shipped untested ABI.

Expand Down Expand Up @@ -315,8 +313,6 @@ If a method's body is non-trivial, the consumer-side default is non-trivial too.
- **Logical `Expr` / `LogicalPlan`**: serialize via `datafusion-proto` using the embedded `FFI_LogicalExtensionCodec`. Same for physical plans → `FFI_PhysicalExtensionCodec`.
- **Enums** (`Volatility`, `TableType`, `InsertOp`, `TableProviderFilterPushDown`): `#[repr(u8)]`, with `From<Native> for FFI_X` and `From<&FFI_X> for Native`. Always write a round-trip unit test that exercises every variant.
- **Errors**: every `FFI_X` method that can fail returns `FFI_Result<T>`. Use the `sresult!`, `sresult_return!`, `df_result!` macros from `src/util.rs` — do not roll your own.
- **Infallible trait methods**: if an FFI call can fail but the native trait cannot return the error, log the transport error before returning the trait's fallback (`None`, `false`, or a default). Never discard it with `.ok()`, `.unwrap_or_default()`, or equivalent.
- **Owned FFI returns**: consume an owned `FFI_X` with `From<FFI_X>` instead of converting through `&FFI_X` and cloning across the boundary. Keep the borrowed conversion's local marker fast path.

## Async, sessions, and task context

Expand Down
35 changes: 0 additions & 35 deletions .ai/skills/pr_review/SKILL.md

This file was deleted.

2 changes: 1 addition & 1 deletion .asf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ github:
- "cargo test (macos-aarch64)"
- "Verify Vendored Code"
- "Check cargo fmt"
- "Check GitHub Actions install tooling"
- "clippy"
- "check Cargo.toml formatting"
- "check configs.md and ***_functions.md is up-to-date"
Expand Down Expand Up @@ -115,3 +114,4 @@ github:
# https://datafusion.apache.org/
publish:
whoami: asf-site

2 changes: 1 addition & 1 deletion .github/ISSUE_TEMPLATE/feature_request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ body:
description: Please describe what you are trying to do.
placeholder: >
A clear and concise description of what the problem is. Ex. I'm always frustrated when [...]
(This section helps DataFusion developers understand the context and *why* for this feature, in addition to the *what*)
(This section helps Arrow developers understand the context and *why* for this feature, in addition to the *what*)
- type: textarea
attributes:
label: Describe the solution you'd like
Expand Down
4 changes: 0 additions & 4 deletions .github/dependabot.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,10 +68,6 @@ updates:
interval: "weekly"
open-pull-requests-limit: 10
labels: [auto-dependencies]
groups:
codeql-actions:
patterns:
- "github/codeql-action/*"
- package-ecosystem: "pip"
directory: "/docs"
schedule:
Expand Down
11 changes: 3 additions & 8 deletions .github/pull_request_template.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,12 @@ We generally require a GitHub issue to be filed for all bug fixes and enhancemen
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.

Please explain the problem you are trying to solve in terms of the user-visible
behavior, rather than the implementation.

For example, "The code in `foo.rs` doesn't handle nulls" is a symptom of the
implementation. "COUNT(DISTINCT) returns wrong results when the column contains
nulls" is the user-visible problem.
-->

## What changes are included in this PR?

<!--
There is no need to duplicate the description in the issue here, but it is sometimes worth providing a summary of the individual changes in this PR.
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->

## Are these changes tested?
Expand All @@ -40,6 +33,8 @@ If tests are not included in your PR, please explain why (for example, are they

<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->

<!--
If there are any breaking changes to public APIs, please add the `api change` label.
-->
4 changes: 2 additions & 2 deletions .github/workflows/audit.yml
Original file line number Diff line number Diff line change
Expand Up @@ -43,9 +43,9 @@ jobs:
security_audit:
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
- name: Install cargo-audit
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
uses: taiki-e/install-action@50414676f9f5d50a65992c6dd2ed02641263226c # v2.82.10
with:
tool: cargo-audit
- name: Run audit check
Expand Down
4 changes: 2 additions & 2 deletions .github/workflows/breaking_changes_detector.yml
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Checkout
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
fetch-depth: 0

Expand Down Expand Up @@ -89,7 +89,7 @@ jobs:

- name: Install cargo-semver-checks
if: steps.changed_crates.outputs.packages != ''
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
uses: taiki-e/install-action@50414676f9f5d50a65992c6dd2ed02641263226c # v2.82.10
with:
tool: cargo-semver-checks

Expand Down
6 changes: 3 additions & 3 deletions .github/workflows/codeql.yml
Original file line number Diff line number Diff line change
Expand Up @@ -40,16 +40,16 @@ jobs:

steps:
- name: Checkout repository
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
persist-credentials: false

- name: Initialize CodeQL
uses: github/codeql-action/init@5595ccaf912efad79be6eef63a5619ff05969be3 # v4
uses: github/codeql-action/init@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v4
with:
languages: actions

- name: Perform CodeQL Analysis
uses: github/codeql-action/analyze@5595ccaf912efad79be6eef63a5619ff05969be3 # v4
uses: github/codeql-action/analyze@8aad20d150bbac5944a9f9d289da16a4b0d87c1e # v4
with:
category: "/language:actions"
8 changes: 3 additions & 5 deletions .github/workflows/dependencies.yml
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ jobs:
container:
image: amd64/rust
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
submodules: true
fetch-depth: 1
Expand All @@ -61,10 +61,8 @@ jobs:
container:
image: amd64/rust
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
- name: Install cargo-machete
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
with:
tool: cargo-machete@0.9
run: cargo install cargo-machete --version ^0.9 --locked
- name: Detect unused dependencies
run: cargo machete --with-metadata
25 changes: 11 additions & 14 deletions .github/workflows/dev.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,19 @@ jobs:
runs-on: ubuntu-latest
name: Check License Header
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
- name: Install HawkEye
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
with:
tool: hawkeye@6.2.0
# This CI job is bound by installation time, use `--profile dev` to speed it up
run: cargo install hawkeye --version 6.2.0 --locked --profile dev
- name: Run license header check
run: ci/scripts/license_header.sh

prettier:
name: Use prettier to check formatting of documents
runs-on: ubuntu-slim
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
- uses: actions/setup-node@6044e13b5dc448c55e2357c09f80417699197238 # v6.2.0
with:
node-version: "20"
- name: Prettier check
Expand All @@ -60,13 +59,13 @@ jobs:
name: Check Markdown Links
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
- name: Load tool versions
run: |
source ci/scripts/utils/tool_versions.sh
echo "LYCHEE_VERSION=${LYCHEE_VERSION}" >> "$GITHUB_ENV"
- name: Install lychee
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
uses: taiki-e/install-action@50414676f9f5d50a65992c6dd2ed02641263226c # v2.82.10
with:
tool: lychee@${{ env.LYCHEE_VERSION }}
- name: Run markdown link check
Expand All @@ -76,23 +75,21 @@ jobs:
name: Validate required_status_checks in .asf.yaml
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
- run: pip install pyyaml
- run: python3 ci/scripts/check_asf_yaml_status_checks.py

typos:
name: Spell Check with Typos
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
persist-credentials: false
# Version fixed on purpose. It uses heuristics to detect typos, so upgrading
# it may cause checks to fail more often.
# We can upgrade it manually once a while.
- name: Install typos
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
with:
tool: typos@1.37.0
- name: Install typos-cli
run: cargo install typos-cli --locked --version 1.37.0
- name: Run typos check
run: ci/scripts/typos_check.sh
13 changes: 5 additions & 8 deletions .github/workflows/docs.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -34,28 +34,25 @@ jobs:
runs-on: ubuntu-latest
steps:
- name: Checkout docs sources
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0

- name: Checkout asf-site branch
uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
ref: asf-site
path: asf-site

- name: Setup uv
uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0
uses: astral-sh/setup-uv@f98e06938123ccabd21905ea5d0069192241f9f1 # v8.3.1

- name: Install dependencies
run: uv sync --package datafusion-docs
- name: Install Graphviz
- name: Install dependency graph tooling
run: |
set -x
sudo apt-get update
sudo apt-get install -y graphviz
- name: Install cargo-depgraph
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
with:
tool: cargo-depgraph@1.6
cargo install cargo-depgraph --version ^1.6 --locked

- name: Build docs
run: |
Expand Down
11 changes: 4 additions & 7 deletions .github/workflows/docs_pr.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -45,23 +45,20 @@ jobs:
name: Test doc build
runs-on: ubuntu-latest
steps:
- uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0
with:
submodules: true
fetch-depth: 1
- name: Setup uv
uses: astral-sh/setup-uv@c771a70e6277c0a99b617c7a806ffedaca235ff9 # v9.0.0
uses: astral-sh/setup-uv@f98e06938123ccabd21905ea5d0069192241f9f1 # v8.3.1
- name: Install doc dependencies
run: uv sync --package datafusion-docs
- name: Install Graphviz
- name: Install dependency graph tooling
run: |
set -x
sudo apt-get update
sudo apt-get install -y graphviz
- name: Install cargo-depgraph
uses: taiki-e/install-action@6c6fd71fe4fb72c3697d269963d0e15df8adedad # v2.85.10
with:
tool: cargo-depgraph@1.6
cargo install cargo-depgraph --version ^1.6 --locked
- name: Build docs html and check for warnings
run: |
set -x
Expand Down
Loading
Loading