Replace ScalarFnVTable::is_fallible with is_infallible - #9511
Conversation
Flip the polarity of the fallibility hook so the conservative default (`false`) is the easy one to satisfy: a scalar function now opts in to being infallible rather than opting out of being fallible. Rename the `DynScalarFn` and `ScalarFnSignature` methods to match, flip every vtable implementation, and rename the `label_is_fallible` expression analysis to `label_is_infallible` (a tree is infallible only when all of its nodes are). Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DaVCc8ZmJ13JtPGQYMM3o
Rename the labeling analysis to `label_infallible` to match its sibling `label_strict`, document it in the same shape, and fix the comment fragments in the `GetItem` and `Select` fallibility overrides. Signed-off-by: Claude <noreply@anthropic.com> Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011DaVCc8ZmJ13JtPGQYMM3o
Merging this PR will improve performance by 14.58%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Simulation | take[duplicates/repeated/primitive/nonnull/chunks=16/indices=1000] |
262.6 µs | 228.9 µs | +14.73% |
| ⚡ | Simulation | compress_fsst[(500, 64, 8)] |
599.2 µs | 523.7 µs | +14.43% |
Tip
Curious why this is faster? Comment @codspeedbot explain why this is faster on this PR, or directly use the CodSpeed MCP with your agent.
Comparing ct/scalar-fn-is-infallible (968391d) with develop (623fd06)
Footnotes
-
442 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. ↩
robert3005
left a comment
There was a problem hiding this comment.
I trust this is mechanical refactor
|
just to be sure ill run sql benchmarks (for correctness) |
Polar Signals Profiling ResultsLatest Run
Powered by Polar Signals Cloud |
Benchmarks: PolarSignals Profiling 📖Commits: PR datafusion / vortex-file-compressed / ns (1.012x ➖, 0↑ 1↓)
No file size changes detected. |
Benchmarks: TPC-H SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.004x ➖, 0↑ 0↓)
datafusion / parquet / ns (0.999x ➖, 1↑ 2↓)
duckdb / vortex-file-compressed / ns (0.991x ➖, 0↑ 0↓)
duckdb / parquet / ns (0.997x ➖, 0↑ 0↓)
File Size Changes (9 files changed, -43.9% overall, 0↑ 9↓)
Totals:
|
Benchmarks: FineWeb NVMe 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (1.030x ➖, 0↑ 1↓)
datafusion / parquet / ns (0.986x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.002x ➖, 4↑ 2↓)
duckdb / parquet / ns (0.992x ➖, 0↑ 0↓)
File Size Changes (2 files changed, -46.3% overall, 0↑ 2↓)
Totals:
|
Benchmarks: TPC-DS SF=1 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.995x ➖, 0↑ 0↓)
datafusion / parquet / ns (1.004x ➖, 0↑ 3↓)
duckdb / vortex-file-compressed / ns (1.003x ➖, 4↑ 4↓)
duckdb / parquet / ns (0.997x ➖, 5↑ 2↓)
File Size Changes (25 files changed, -43.5% overall, 0↑ 25↓)
Totals:
|
Benchmarks: Clickbench Sorted on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.969x ➖, 2↑ 1↓)
datafusion / parquet / ns (1.001x ➖, 0↑ 1↓)
duckdb / vortex-file-compressed / ns (1.019x ➖, 1↑ 2↓)
duckdb / parquet / ns (0.988x ➖, 0↑ 0↓)
File Size Changes (201 files changed, -42.8% overall, 57↑ 144↓)
Totals:
|
Benchmarks: TPC-H SF=10 on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.997x ➖, 0↑ 1↓)
datafusion / parquet / ns (0.994x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (0.981x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.026x ➖, 0↑ 1↓)
File Size Changes (9 files changed, -44.0% overall, 0↑ 9↓)
Totals:
|
Benchmarks: Clickbench on NVME 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.995x ➖, 2↑ 0↓)
datafusion / parquet / ns (0.993x ➖, 2↑ 1↓)
duckdb / vortex-file-compressed / ns (0.983x ➖, 4↑ 0↓)
duckdb / parquet / ns (1.002x ➖, 0↑ 0↓)
File Size Changes (101 files changed, -39.2% overall, 0↑ 101↓)
Totals:
|
Benchmarks: Statistical and Population Genetics 📖Commits: PR How to read Verdict and Engines
duckdb / vortex-file-compressed / ns (0.930x ➖, 4↑ 1↓)
duckdb / parquet / ns (0.982x ➖, 0↑ 0↓)
File Size Changes (2 files changed, -32.3% overall, 0↑ 2↓)
Totals:
|
Benchmarks: FineWeb S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.772x ➖, 4↑ 0↓)
datafusion / parquet / ns (1.030x ➖, 0↑ 0↓)
duckdb / vortex-file-compressed / ns (1.014x ➖, 0↑ 0↓)
duckdb / parquet / ns (1.299x ➖, 2↑ 2↓)
|
Benchmarks: TPC-H SF=1 on S3 📖Commits: PR How to read Verdict and Engines
datafusion / vortex-file-compressed / ns (0.872x ➖, 3↑ 1↓)
datafusion / parquet / ns (1.082x ➖, 0↑ 3↓)
duckdb / vortex-file-compressed / ns (0.974x ➖, 2↑ 1↓)
duckdb / parquet / ns (1.003x ➖, 0↑ 0↓)
|
Rationale for this change
Flips the polarity of the fallibility hook so the conservative answer is the default. A scalar function now opts in to being infallible by overriding
is_infallibleto returntrue, instead of opting out of the fallible default.What changes are included in this PR?
Renames
ScalarFnVTable::is_fallibletois_infalliblealong with theDynScalarFnandScalarFnSignaturemirrors, and inverts every implementation and call site (dictionary push-down, the dict layout pushdown split, andExpressionTakeRule). Also renames thelabel_is_fallibleexpression analysis tolabel_infallibleso it matcheslabel_strict, and inverts its tests in place.What APIs are changed? Are there any user-facing changes?
ScalarFnSignature::is_falliblebecomesis_infallibleandlabel_is_falliblebecomeslabel_infallible, both with flipped return values. Out-of-treeScalarFnVTableimplementations must rename theiris_fallibleoverride and invert the result, and an override that returnedtruecan now rely on the default.Generated by Claude Code