Skip to content

Separate RowFn decode and semantic infallibility - #9644

Closed
connortsui20 wants to merge 1 commit into
developfrom
ct/row-fn-infallible
Closed

Separate RowFn decode and semantic infallibility#9644
connortsui20 wants to merge 1 commit into
developfrom
ct/row-fn-infallible

Conversation

@connortsui20

@connortsui20 connortsui20 commented Aug 26, 2026

Copy link
Copy Markdown
Member

Summary

Tracking Issues: #9129, #9130

Separates semantic row totality from input decode totality. ScalarFnVTable::is_infallible requires both guarantees because it permits evaluation of unreferenced values, while decode fallibility no longer selects the nullable-row policy.

Arguably, we should separate these concepts directly on the ScalarFnVTable, but that can come later.

I'll just copy paste this from my conversation with @joseph-isaacs:

Details

so our is_infallible on the scalar fn vtable is overloaded. What we really want to say is "can I evaluate something speculatively" which is a harder constraint than just semantic totality:

  • the operation cannot fail for any valid, well-typed input (totality)
  • we ALSO need to know that evaluating data not referenced by the logical expression cannot introduce an execution error, including unused dictionary values and arbitrary payloads behind nulls

Changes

Adds the required RowFn::DECODE_INFALLIBLE declaration and checks positive declarations against each input tuple selected by dispatch. Marks spatial distance as decode-fallible and adds regression coverage for early decode errors and dense execution of partially valid inputs.

@connortsui20 connortsui20 added the changelog/fix A bug fix label Aug 26, 2026
Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
@codspeed-hq

codspeed-hq Bot commented Aug 26, 2026

Copy link
Copy Markdown

Merging this PR will regress 1 benchmark

⚠️ Unknown Walltime execution environment detected

Using the Walltime instrument on standard Hosted Runners will lead to inconsistent data.

For the most accurate results, we recommend using CodSpeed Macro Runners: bare-metal machines fine-tuned for performance measurement consistency.

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 4 improved benchmarks
❌ 1 regressed benchmark
✅ 2125 untouched benchmarks
⏩ 106 skipped benchmarks1
🗄️ 4 archived benchmarks run2

Warning

Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

Mode Benchmark BASE HEAD Efficiency
WallTime arrow_checked_add_u32_neon[16384] 13.4 µs 20.3 µs -33.9%
WallTime arrow_checked_add_u32_avx512[16384] 21.4 µs 17.7 µs +21.14%
WallTime arrow_checked_add_u32_avx2[16384] 21.4 µs 17.7 µs +20.72%
WallTime multiply_shapes_neon[(128, PerRowPerRow)] 2.2 µs 1.9 µs +13.48%
WallTime add_shapes_neon[(128, PerRowPerRow)] 2.1 µs 1.8 µs +11.75%

Tip

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


Comparing ct/row-fn-infallible (d49a7be) with develop (42a5d3b)

Open in CodSpeed

Footnotes

  1. 106 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports.

  2. 4 benchmarks were run, but are now archived. If they were deleted in another branch, consider rebasing to remove them from the report. Instead if they were added back, click here to restore them.

@connortsui20
connortsui20 enabled auto-merge (squash) August 26, 2026 18:29
@connortsui20
connortsui20 requested a review from gatesn August 26, 2026 19:07

@gatesn gatesn left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is the wrong decomp I think.

You're trying to distinguish functions that can return errors, that may be caught. This is important when e.g. a push-down optimization would hide a failure that otherwise would semantically cause the query to fail.

Vs errors that you don't expect to happen / the user has asserted to ignore. For example, if the user opts in to not validating the WKB, then I think it is perfectly reasonably for us to push this through dictionary encoding and not error.

I wonder if this is related to our work on passing down a "defined" flag to allow ignoring errors in null positions etc.

@connortsui20
connortsui20 disabled auto-merge August 26, 2026 19:36
@connortsui20

Copy link
Copy Markdown
Member Author

Ok we dont actually need this now that we have the filter/scatter execute from #9521, but I still think there is something wrong here (not specific to RowFn, but to ScalarFnVTable in general)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

changelog/fix A bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants