Repository navigation
feat(mp_filtercheck): accept SAS temporal literals in RAW_VALUE - #460
Conversation
Date, datetime and time literals (eg '20APR2042'd, '01JAN1960:00:00:42'dt, '00:00:42't) are valid values for a numeric column, but the check rejected them: it strips quoted strings and then flags any non-digit remainder, so the trailing d/dt/t failed the test. The macro header carried this as a TODO. Temporal literals are now accepted for numeric columns with the comparison operators and inside IN / NOT IN / BETWEEN lists. They remain invalid for character columns, and other literal suffixes (hex, name) are still rejected. While adding the list cases, the value loop was corrected: the return sat inside the per-value loop, so only the first value of a numeric IN / NOT IN / BETWEEN list was ever checked. Every value is now validated, and a bad one produces a bad record rather than passing silently.
There was a problem hiding this comment.
Reviewed sasjs/core #460 (feat(mp_filtercheck): accept SAS temporal literals in RAW_VALUE) at head 6314dd1 - the full diff, base/mp_filtercheck.sas, the generated all.sas bundle, the new tests, and CI status.
-
.github/workflows/run-tests.yml did not execute for this head: the Node.js CI run (id 38881470675) concluded action_required with no jobs started, so the new cases in tests/base/mp_filtercheck.test.sas are unverified and mergeable_state is unstable. Not a green pipeline.
-
base/mp_filtercheck.sas:275: the added
return;at the end of the IN/NOT IN/BETWEEN block is unconditional, but the per-value check above it only runs when vtype='N'. Previously thereturnsat inside theif vtype='N'loop, so character-column IN/BETWEEN lists fell through to the generic string check at lines 280-324. The comment ("checked one by one above") does not hold for vtype='C' - char lists are now accepted without that validation (a bare-name char list such as(a,b)used to be name/type checked). mp_filtervalidate still runs afterwards, so impact is limited; either gate the return on vtype='N' or confirm char IN/BETWEEN needs no check. -
base/mp_filtercheck.sas:98-102: re_temporal is added to the length statement but is not in the
drop(only tmp is), so it is emitted as an extra column in the &outds bad-records dataset. Add it to the drop list if the output shape matters. -
base/mp_filtercheck.sas:248-254: removing
goto endstepfrom the list type-mismatch branch is a behaviour change - a compared-type mismatch inside an IN/BETWEEN list no longer skips the rest of that record's checks. Consistent with the check-every-value fix, but not called out in the PR description. -
base/mp_filtercheck.sas:233-235: empty
if prxmatch(...) then do; end;block -if prxmatch(...) then ;(or inverting the condition) reads better.
The temporal-literal regex (^'[^']*'(dt|d|t)$, case-insensitive) and the placement of the scalar early-return after the GROUP_LOGIC/OPERATOR_NM checks look correct, and the four new test cases cover the behaviour the PR claims. all.sas is the generated bundle, so the line references above are against base/.
5 findings above for review.
- gate the early return on vtype='N'. The value-by-value path is numeric only, and a character IN / BETWEEN list fell through to the generic check before - it still does. The unconditional return silently changed that. - restore `goto endstep` on a compared-type mismatch inside a list, so the remaining values of that record are not re-checked after it is already flagged. - drop re_temporal from the output, so the working regex id does not become a column of the bad records table. - remove the empty `then do; end;` block - the temporal test is inverted. Two tests added: a character IN list is still validated, and re_temporal is not in the bad records table.
|
Thanks - all four code findings are addressed in 072fdc9.
|
There was a problem hiding this comment.
Reviewed the current diff at head 093258f, scoped to the delta since the previous review at 6314dd1 (base/mp_filtercheck.sas, the generated all.sas, and tests/base/mp_filtercheck.test.sas).
- The Node.js CI test workflow still has not run: the run for this head (id 38885589519) concluded action_required with no jobs started, so the SAS test suite - including the two new cases - remains unverified and mergeable_state is unstable.
1 finding above for review.
|
On the CI point (finding 1) - I cannot clear the approval gate from here, but I did run the test file against a real Viya engine so the new cases are not unverified. Method: built the branch with a Viya target ( Result: 15 assertions, all PASS, and no SAS errors in the log.
The two cases added in the review fix are what proves the gate: I have not attached the log - it carries estate-identifying detail - but I can share the relevant excerpt if useful. |
|
🎉 This PR is included in version 5.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What
mp_filtercheckrejects SAS date, datetime and time literals inRAW_VALUE, even though a temporal literal is a valid operand for a numeric column - both in aWHEREclause and in the comparison the macro itself generates. The macro header carried this as an open item:The check strips quoted strings and then flags any non-digit remainder, so the trailing
d/dt/tfails the test:Change
Temporal literals are now accepted for numeric columns:
=,NE,>,<,>=,<=,GE,LE);IN/NOT INlist;BETWEEN.They remain invalid for character columns, and other literal suffixes (hex
'...'x, name'...'n) are still rejected - the pattern is anchored to the temporal suffixes only.Also fixed
While adding the list cases, the value loop was corrected. The
returnsat inside the per-value loop, so only the first value of a numericIN/NOT IN/BETWEENlist was ever validated - a bad second value passed silently. Every value is now checked, and a bad one produces a bad record.Why
A client can now send the readable form of a filter. Data Controller shows the resolved
WHEREclause above the grid, so a date filter readSOME_DATE = 34322(the internal day count) rather than a date. Sending'20APR2042'dgives a clause that is both valid and legible, and the rows returned are unchanged.Tests
tests/base/mp_filtercheck.test.sasgains four cases:IN,BETWEEN) against numeric columns produce no bad records;'20APR2042'x) is still rejected;INlist is now caught.sasjs lintreports no errors for the changed files.