Skip to content

fix(cmp): reject not over STR, SYM and GUID vectors - #1114

Open
belowzeroff wants to merge 2 commits into
RayforceDB:devfrom
belowzeroff:fix/issue-976-not-text-guid
Open

belowzeroff wants to merge 2 commits into
RayforceDB:devfrom
belowzeroff:fix/issue-976-not-text-guid

Conversation

@belowzeroff

Copy link
Copy Markdown
Contributor

How it looks from the user's side

Before (dev 28e4c29):

(not (enlist "a" ""))   ; false
(not ['a 'b])           ; false
(not (guid 2))          ; false
(not (list ['a] 1))     ; [false false]

(set T (table [s g] (list (enlist "a" "") (guid 2))))
(at (select {r: (not s) from: T}) 'r)   ; error: type: not: expects a numeric argument, got STR
(at (select {r: (not g) from: T}) 'r)   ; error: type: not: expects a numeric argument, got GUID

(set Y (table [y] (list ['a 'b 'c])))
(at (select {r: (not y) from: Y}) 'r)   ; [false false false]
(count (select {from: Y where: (not y)}))   ; 0
(at (select {r: (not y) from: (table [y] (list (as 'SYMBOL ["a" "" "c"])))}) 'r)
                                        ; error: type: not: expects a numeric argument, got SYM

One boolean for a whole vector in the direct call, while the same not inside
a query is a type error for STR and GUID, and for a SYM column only when it
holds an empty symbol; otherwise it answers booleans.

After:

(not (enlist "a" ""))   ; error: type: not: expects a numeric argument, got STR
(not ['a 'b])           ; error: type: not: expects a numeric argument, got SYM
(not (guid 2))          ; error: type: not: expects a numeric argument, got GUID
(not (list ['a] 1))     ; error: type: not: expects a numeric argument, got SYM
(at (select {r: (not y) from: Y}) 'r)   ; error: type: not: expects a numeric argument, got SYM
(select {from: Y where: (not y)})       ; error: type: not: expects a numeric argument, got SYM

The query form now raises for every STR, SYM and GUID column, whatever it
holds, and agrees with the direct call. Atoms and
lists of atoms are unchanged: (not "") is true, (not 'a) is false,
(not (list "a" "")) is [false true].

Cause

ray_not_fn is element-wise for BOOL, numeric and temporal vectors only. Any
other vector fell through to the atom rule and was tested as one value. The
compiled not raised a type error for STR and GUID columns, but a SYM column
was compiled as a numeric lane of intern ids and answered booleans unless it
held an empty symbol (which sent it to the typed fallback).

Fix

ray_not_fn raises the compiled path's type error for a STR, SYM or GUID
vector (an empty one too, as the compiled path does). The query compiler no longer
accepts not over a SYM column (the same bail it already has for ordering
comparisons on SYM), so the typed fallback raises for any SYM column;
equality and the other comparisons are unchanged. The issue allows
either element-wise or a type error; not has no defined meaning for these
cells, and test/rfl/query/not_bool_column.rfl already pins the compiled
rejection of a symbol column, so the eager path now matches it.

Tests

test/rfl/cmp/not.rfl: each type raises type in the direct call, including
a slice and an empty vector; atoms are unchanged; the query form raises too.
Also a SYM column without empties, with empties, splayed with a sym file,
in where:, and 80000 rows. Comparisons on a SYM column still work. The new
cases fail without the fix.

Scope

This covers STR, SYM and GUID. The TIMESPAN part of the issue is in #1074.
The and/or part was fixed by #1103. With #1074 this finishes #976, so
Closes #976 is right once #1074 has landed; on its own this should say
Refs #976.

not on a STR, SYM or GUID vector fell through to the atom rule and
answered one boolean for the whole vector, while the compiled not rejects
STR and GUID columns with a type error. Raise the same type error from
the eager not, so the two paths agree. not has no defined meaning for the
cells of these types, and a test already pins the compiled rejection of a
symbol column.

The compiled not over a SYM column took the numeric lane path and answered
booleans unless the column held an empty symbol. Bail it to the typed
fallback, as ordering comparisons on SYM already do, so it raises whatever
the column holds. Atoms and the list recursion are unchanged.

@morsel-ray morsel-ray left a comment •

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Source approval for this bounded NOT fix: complete feature diff and the direct/list-recursion/compiler-admission contracts read; immutable snapshots independently checked. Concrete STR/SYM/GUID vectors reject before scalar truthiness, including empty/slices; SYM compiler decline reaches the typed error rather than intern-ID arithmetic. Numeric/scalar paths and comparison admission are unchanged. No local candidate runtime/full-suite/current-base integration claim; author runs retain their scope.

Keeping Refs #976 is important even after #1074 unless broadcasting is independently resolved. My existing DEBUG ELF (same cmp.c blob as accepted ae8e5c0d, not a fresh accepted/candidate build) still gives scalar b8 values for (or 3 [0 2]) / (and 0 [0 2]): [true 'b8 false 'b8]. The issue expects broadcast vectors. This does not block this narrower NOT correction, but please do not treat #1103 + #1074 + this change as demonstrated closure of every #976 case. No new logical-operator policy chosen here.

Nov7 ordinary accepted-base import: checked head e0b8dc5, ordered parents [9f95b31,2d3b66b0145dd3bc677c688251eb753cd070e9f8], tree 8b226ab62663259820b4b85aff60656a840e861f. All four feature blobs unchanged; 27 disjoint accepted imports exact. Merge-tree and independent strict feature-patch application agree. Existing source approval carried, not a new candidate runtime/full-suite/CI or landing claim. #976 remains OPEN: broadcasting remains outside this fix, even after #1074; no automatic closing references. Evidence: review-nov7-0940-intake/1114/accepted-2d3-import/composition-proof.json.

Nov7 local small-shape validation on unchanged feature blobs at e0b8dc5: compatible DEBUG ASan/UBSan compositions replacing cmp/expr TUs, not fresh whole-parent/new-head builds. Two original observer children check STR/SYM/GUID slices at N0/1/2 plus eight unchanged atom/list/numeric controls. Two additional children use flat, unshared SYM columns at N0/1/2/3 and a null-containing case; these actually reproduce the old compiled BOOL lane at N1/2/3 and its new TYPE result, not merely an already-falling-back slice. Both studies independently sealed; no sanitizer diagnostics, LSan off. No sort queries were used; the selected late-head counter stayed zero. The shared private sort TU was common to both compositions. (Scope wording clarified: that counter is not instrumentation of every function in sort.c.)

Empty WHERE skips evaluation and returns an empty table on both binaries for all three types; this is not a new defect or a promise to validate unevaluated rows. No full-suite/floor/performance/fault certification. Evidence: review-nov7-0940-intake/1114/runtime-smallshapes/parent-independent-proof.json and runtime-flat-sym/parent-independent-proof.json. CI/current-base/final landing guards remain separate; #976 broadcasting remains open.

This branch has not been deployed

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants