Skip to content

Rename scalar function strictness contract - #8930

Merged
connortsui20 merged 1 commit into
developfrom
agent/rename-scalar-fn-strictness
Jul 27, 2026
Merged

Rename scalar function strictness contract#8930
connortsui20 merged 1 commit into
developfrom
agent/rename-scalar-fn-strictness

Conversation

@connortsui20

@connortsui20 connortsui20 commented Jul 23, 2026

Copy link
Copy Markdown
Member

Changes is_null_sensitive to is_strict and flips all current implementations (as is_strict implies !is_null_sensitive).

A function is strict only when any null input forces a null output and its return dtype propagates that nullability.

Also fixes Between and Dynamic scalar functions as they were both set to strict when they should have been non-strict, so now they are is_strict = false.

Rationale for this change

Dictionary pushdown rewrites f(dict(codes, values), constant) as dict(codes, f(values, constant)). At a null code, the original evaluates f(null, constant), while the rewritten dictionary is always null. This is only sound when nulling any one argument forces a null result while the other arguments remain unchanged.

is_null_sensitive documented the unary case but left this multi-argument contract ambiguous. Kleene AND/OR show the difference: masking both inputs produces null, but false AND null = false and true OR null = true. That ambiguity caused the bug fixed in #8929.

Additionally, we have noticed that a large majority of our scalar functions have strict semantics, and so in the future (hopefully soon) we can lift out a lot of the shared null logic.

What changes are included in this PR?

Functions that are strict, previously treated as non-strict

These functions are strict, so the old null-sensitivity classification was overly conservative:

  • ArrayExpr and RowIdx inherited the old conservative default. These functions take no arguments, so the strictness condition does not apply.
  • Mask inherited the old conservative default, but a null input remains null and its mask argument is required to be non-nullable.
  • VariantGet inherited the old conservative default, but a null Variant input produces a null output.
  • GetItem and Select preserve the parent struct validity and propagate its nullability.
  • Merge only accepts non-nullable inputs, so the strictness law is vacuously satisfied for well-typed arguments.
Functions that are non-strict, previously treated as strict

These functions are non-strict, so the old classification allowed unsound dictionary pushdown:

  • Between: its comparisons are combined with Kleene AND; for example, 10 BETWEEN null AND 5 is false, not null.
  • DynamicComparison: when the dynamic RHS is absent, it returns its configured default even when the LHS is null.
Remaining non-strict functions
  • Binary for Kleene AND/OR
  • CaseWhen and Zip
  • Cast
  • FillNull, IsNull, and IsNotNull
  • ForeignScalarFnVTable
  • ListContains
  • Pack
  • RowEncode and RowSize
  • StatFn
Remaining strict functions
  • Binary for all operators except AND/OR
  • ByteLength, ExtStorage, Like, ListLength, ListSum, and Not
  • Literal, Root, and RowCount (nullary)
  • JsonToVariant
  • GeoContains, GeoDistance, GeoEnvelope, and GeoIntersects
  • CosineSimilarity, InnerProduct, L2Denorm, and L2Norm

Base automatically changed from agent/fix-kleene-dict-pushdown to develop July 23, 2026 17:43
@connortsui20
connortsui20 force-pushed the agent/rename-scalar-fn-strictness branch 2 times, most recently from 15f1ae9 to 60b7f57 Compare July 23, 2026 19:02
@vortex-data vortex-data deleted a comment from github-actions Bot Jul 23, 2026
@vortex-data vortex-data deleted a comment from github-actions Bot Jul 23, 2026
@vortex-data vortex-data deleted a comment from codspeed-hq Bot Jul 23, 2026
@connortsui20 connortsui20 added the changelog/feature A new feature label Jul 23, 2026
@connortsui20
connortsui20 marked this pull request as ready for review July 23, 2026 19:49

@joseph-isaacs joseph-isaacs 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.

good direction. just want to ensure it's all sound

Comment thread vortex-array/src/scalar_fn/vtable.rs Outdated
Comment thread vortex-array/src/scalar_fn/vtable.rs Outdated
@joseph-isaacs

joseph-isaacs commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Can we rewrite the description to just mention strict and non-strict results. mentioning is_null_sensitive = true here is confusing!

Signed-off-by: Connor Tsui <connor.tsui20@gmail.com>
Comment on lines +341 to +343
fn is_strict(&self, _options: &Self::Options) -> bool {
true
}

@joseph-isaacs joseph-isaacs Jul 27, 2026

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 not the default, is it?

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 changed the default to false on the vtable?

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.

Add this file is the wrong vtable lol

@joseph-isaacs joseph-isaacs added changelog/break A breaking API change and removed changelog/feature A new feature labels Jul 27, 2026
@connortsui20
connortsui20 force-pushed the agent/rename-scalar-fn-strictness branch 2 times, most recently from 1a807fc to a3494a5 Compare July 27, 2026 15:27
@codecov

codecov Bot commented Jul 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 60.13072% with 61 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.27%. Comparing base (caedc63) to head (a3494a5).
⚠️ Report is 7 commits behind head on develop.

Files with missing lines Patch % Lines
vortex-array/src/arrays/scalar_fn/vtable/mod.rs 0.00% 3 Missing ⚠️
vortex-array/src/scalar_fn/fns/mask/mod.rs 0.00% 3 Missing ⚠️
vortex-array/src/scalar_fn/fns/stat.rs 0.00% 3 Missing ⚠️
vortex-array/src/scalar_fn/fns/variant_get/mod.rs 0.00% 3 Missing ⚠️
vortex-array/src/scalar_fn/foreign.rs 0.00% 3 Missing ⚠️
vortex-layout/src/layouts/row_idx/expr.rs 0.00% 3 Missing ⚠️
vortex-array/src/scalar_fn/fns/case_when.rs 0.00% 2 Missing ⚠️
vortex-array/src/scalar_fn/fns/cast/mod.rs 0.00% 2 Missing ⚠️
vortex-array/src/scalar_fn/fns/ext_storage.rs 0.00% 2 Missing ⚠️
...ortex-array/src/scalar_fn/fns/list_contains/mod.rs 0.00% 2 Missing ⚠️
... and 19 more

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@connortsui20
connortsui20 enabled auto-merge (squash) July 27, 2026 15:38
@connortsui20
connortsui20 merged commit 318a350 into develop Jul 27, 2026
230 of 242 checks passed
@connortsui20
connortsui20 deleted the agent/rename-scalar-fn-strictness branch July 27, 2026 15:59
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