Skip to content

Preserve extension literals in DataFusion comparisons - #11067

Merged
connortsui20 merged 3 commits into
developfrom
ct/ext-types/datafusion-boundary
Nov 2, 2026
Merged

connortsui20 merged 3 commits into
developfrom
ct/ext-types/datafusion-boundary

Conversation

@connortsui20

@connortsui20 connortsui20 commented Oct 30, 2026 •

Copy link
Copy Markdown
Member

Summary

Depends on #11016.

DataFusion literals can hold an extension column's Arrow storage value without its extension metadata. Uses the other operand's field metadata to reconstruct Vortex extension scalars, so UUID and JSON filters can still be pushed down with matching dtypes.

Changes

Uses one binary operand conversion path, with field context for comparison literals. Comparison and IN-list literals use the registered Arrow importer, preserving explicit literal metadata and timestamp units. Inherited extension names and payloads come from the same field, including when the payload is absent, and import errors retain their original cause.

Tests cover literal metadata, including stale payloads, static filters, and dynamic filter updates with residual evaluation. Formatting passes with CI's pinned nightly-2026-09-10 for vortex-datafusion. Tests, Clippy, and benchmarks were not run locally for this layer.

API Changes

Exporting a non-temporal extension scalar as a bare DataFusion ScalarValue now returns an error instead of discarding its extension type. Scalar statistics that cannot preserve the extension type are omitted.

@connortsui20
connortsui20 added this pull request to stack #11017 October 30, 2026 20:36
@connortsui20 connortsui20 added the changelog/break A breaking API change label Oct 30, 2026 — with ChatGPT Codex Connector
connortsui20 added a commit that referenced this pull request Oct 30, 2026
The phase graph of the extension plan now shows each PR of phase 0, its
state and the order constraints. Phase 1 depends on U4 and U5, and its
strict reads wait for the spatial fix. Phase 0 holds #10953, the spatial
fix and the importer change, and the structural part of X29 moves out of
phase 6. The PR state lists #11013 as merged and adds #11067.

In the type roadmap, N6 becomes the storage declaration (X25 to X30),
with hard edges from N1 and into N3, and the report matches.

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20
connortsui20 force-pushed the ct/ext-types/datafusion-boundary branch from 7443a28 to 7694d56 Compare October 30, 2026 20:51
Comment on lines +145 to +148
vortex_bail!(
"DataFusion ScalarValue requires a dedicated extension representation, got {}",
self.dtype()
);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I believe this is the main breaking change, and I think that the current version is just wrong? We can't just pull out the storage scalar.

@connortsui20
connortsui20 marked this pull request as ready for review October 30, 2026 20:57
Base automatically changed from ct/ext-types/comparison-consistency to develop November 2, 2026 15:56
connortsui20 added a commit that referenced this pull request Nov 2, 2026
## Summary

- Tracking Issue: #7683.

Extension comparisons could compare raw storage on the array path while
scalar execution rejected the same operands. Applies one dtype
requirement across comparison paths.

## Changes

Checks logical dtypes at binding and before comparison fast paths or
encoding kernels run. Regression tests cover batch, row, and constant
execution, encoded timestamps, and DuckDB filter pushdown through SQL
logic tests. Broader documentation is split into #11099. DataFusion
literal conversion is handled separately in #11067.

Local pre-commit license and formatting checks passed. Tests and
benchmarks were not run after the review fixes. Workspace-wide Clippy
started through the pre-push hook and was stopped without a result.

## API Changes

Comparison operands must have matching logical dtypes, ignoring
nullability. Comparisons between an extension and its storage dtype, or
between different extension dtypes, now return an error. This includes
timestamps with different units or time zones.

---------

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20
connortsui20 force-pushed the ct/ext-types/datafusion-boundary branch from 7694d56 to 4197528 Compare November 2, 2026 15:56
@connortsui20
connortsui20 enabled stacked auto-merge (squash) November 2, 2026 15:58
@connortsui20
connortsui20 merged commit 3f12ae4 into develop Nov 2, 2026
74 of 75 checks passed
@connortsui20
connortsui20 deleted the ct/ext-types/datafusion-boundary branch November 2, 2026 16:09
@codspeed

codspeed Bot commented Nov 2, 2026

Copy link
Copy Markdown

Merging this PR will degrade performance by 10.35%

1 warning

⚠️ Walltime results may be unreliable

Some benchmarks used the Walltime instrument outside CodSpeed Macro Runners, which can make the results inconsistent.

Learn more about Macro Runners

See affected benchmarks

❌ 1 regressed benchmark
✅ 2317 untouched benchmarks

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
❌ Walltime lt_i64_nullable_avx512 7.4 µs 8.3 µs -10.35%

Tip

Investigate this regression by commenting @codspeedbot fix this regression on this PR, or directly use the CodSpeed MCP with your agent.


Comparing ct/ext-types/datafusion-boundary (4197528) with develop (dafc832)

Open in CodSpeed

@connortsui20 connortsui20 mentioned this pull request Nov 2, 2026
9 tasks done
connortsui20 added a commit that referenced this pull request Nov 2, 2026
U2, U3, U4, U6, U7 and U8 merged on 2026-11-02 in #11012, #11014 and
#11016, and the DataFusion literal fix #11067 merged right after U2. So
order constraint 1 holds, and phase 1 starts from develop. The graph
shows these nodes as merged, and parts 2 and 4 name the PR that fixed
each finding.

#10953 now keeps i32 days and widens only the days part to i64 when a
value does not fit. It has no review, and it conflicts with the
DateTimeParts compare kernel that U2 changed. #11056 still waits on its
request for changes. #10661 replaces the closed #10482 for file sort
statistics.

In the type roadmap, only the N1 sentence changes.

Signed-off-by: Connor Tsui <[email protected]>
@connortsui20 connortsui20 mentioned this pull request Nov 2, 2026
12 of 49 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/break A breaking API change

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants