[developer-rpai]

Fixes #277

Problem

The plausibleValueHigh field-level checks ("no dates in the future") hard-code GETDATE() in the control-file thresholds (DATEADD(dd,1,GETDATE()), YEAR(GETDATE())+1). Running the same checks over the same data on different days produces different results, violating the basic expectation that analysis X on data Y is reproducible regardless of run date.

What this PR does

• Adds a futureDate parameter to executeDqChecks():
  • User-supplied date is used as-is (validated, coerced to YYYY-MM-DD).
  • Default for live runs is MAX(observation_period_end_date) queried once from the CDM, so the reference date is inherent in the data and stable across runs.
  • In sqlOnly mode with no futureDate, the legacy GETDATE() behavior is kept (sqlOnly only generates SQL; it never produces results).
• Replaces the GETDATE() expressions in the v5.2/v5.3/v5.4 field-level control files (138 thresholds) with an @futureDate placeholder, resolved in .runCheck before SQL rendering.
• Documents futureDate in man/executeDqChecks.Rd, adds a NEWS.md entry, and adds testthat coverage: placeholder-resolution unit tests plus a guard asserting no plausibleValueHigh threshold references GETDATE() anymore.

Design notes

• Implements the approach sketched by @clairblacketer in the issue discussion (a future_date parameter defaulting to a data-inherent date; observation_period_end_date over cdm_source.source_release_date since it is the more conservative "future" bound and always present in the CDM).
• The placeholder is resolved in R rather than relying on nested SqlRender parameters, so rendering behavior is identical across dialects.
• In the sqlOnly legacy path the fallback renders as DATEADD(dd,1,CAST(GETDATE() AS DATE)) (date-truncated, vs the old datetime GETDATE()); the check template casts to date for date/datetime fields anyway.

Testing

• New tests/testthat/test-futureDate.R (placeholder resolution + control-file guard).
• I could not execute the full testthat suite locally (no R toolchain in this environment); relying on CI for R CMD check.

[MaximMoinat]

Thanks @developer-rpai.

Coincidentally I was working on a new check for future dates. The current (mis)use of the plausibleValueHigh check is confusing as it is an 'atemporal' check and for a non-expert hard to interpret as the check description is implicit about what it checks ("value larger than 'getdate()'").

That said, I agree we need to have a futureDate parameter, the current definition using the current date is not great. The best date to use for me would be the cdm_source.source_release_date, if available.

For the default futureDate, I would suggest to keep it the same (current date) as not to change behaviour too much. Also, setting default to MAX(observation_period_end_date) will make all observation period dates by definition pass. And observation periods in the future is a common issue we are seeing.

[katy-sadowski]

@MaximMoinat I agree with you re: breaking this out into a new future date check. cdm_source.source_release_date was also my first thought for the default value. Obviously if someone didn't fill this out (another DQD failure), or filled it out wrong, this will cause issues.

@developer-rpai thanks so much for the contribution!

[MaximMoinat]

Agree, we just need a fallback date if release date is missing. Either we use current date, or don't execute the check and mark as 'NA'.

[developer-rpai]

@MaximMoinat Went with the NA-skip option.

Rationale: the entire purpose of #277 is determinism. A current-date fallback would reintroduce exactly the run-to-run variance this PR removes, and without a release date there is no honest definition of "future" — executing the check against an invented date is worse than not running it.

What changed in this push:

• Default futureDate is now cdm_source.source_release_date (replacing the MAX(observation_period_end_date) default).
• When source_release_date is missing/NULL (or unqueryable), the affected checks are not executed: .runCheck() records the row via .recordFutureDateSkipped() (queryText = NA, explanatory warning), and .calculateNotApplicableStatus() marks it notApplicable = 1 with reason "cdm_source.source_release_date is missing; future-date check was not executed." No Sys.Date()/GETDATE() fallback remains in the execution path. (sqlOnly mode keeps the legacy GETDATE() rendering since it only generates SQL.)
• An explicit futureDate argument still overrides everything, unchanged.
• Tests added for the skip → not-applicable marking; NEWS.md and the man page updated.

Caveat: no R toolchain in this environment, so this is a careful manual syntax review only — testthat and R CMD check will need to run on repo CI.

cc @katy-sadowski

[MaximMoinat]

Thanks again @developer-rpai. Overall, I think this is the right logic.

Two review notes:

• It adds 1 day to the future date (either from user defined parameter, or from : "DATEADD(dd,1,@futureDate)". This may be confusing.
• Not sure why the sql-only still needs to use getdate(). Would be

A few notes before merging @katy-sadowski:

• We should put this on roadmap for v3, and determine right order to merge with other work.
• The current implementation is very verbose, with lots of implementation notes in comments and one-off functions. Some ideas to simplify:
  • As suggested before, make this into separate check. Then we can get cdm_source_date with simple join.
  • Add NA logic into the calculateNotApplicableStats
• Both comments and log messages mention multiple times the original issue Update OMOP_CDMv5.3.1_Concept_Level.csv by adding rows for flowsheet measurement #227. I don't think that is helpful.
• There is a new result field futureDateSkipped in the main 'CheckResults' table.

[developer-rpai]

@MaximMoinat Good questions — clarifying both:

The +1 day DATEADD: the futureDate parameter is a date string ('YYYY-MM-DD', midnight). The check flags values strictly after the reference date, so without the +1, any timestamp later on the futureDate itself (e.g. 2026-10-01 14:30:00 vs futureDate = '2026-10-01') would be flagged as "in the future." The DATEADD(dd,1,...) gives the reference day itself a pass — only values beyond end-of-day count. That said, I agree the bare DATEADD(dd,1,@futureDate) in the threshold expression is opaque. I can either (a) keep the semantics and name it explicitly in the check description, or (b) drop the +1 and document that the reference date is exclusive. Preference?

sqlOnly and GETDATE(): sqlOnly mode renders SQL without executing anything, so there is no database to read cdm_source.source_release_date from — the lookup the execution path uses is not available. When the caller passes an explicit futureDate, sqlOnly uses it; GETDATE() is only the fallback when no futureDate is given, preserving the legacy rendering. Happy to make futureDate required in sqlOnly mode instead if you would rather have it fail loudly than silently fall back — let me know.

On the pre-merge notes (separate check, NA consolidation, comment cleanup) — those are maintainer-side restructuring calls; I will defer to you and @katy-sadowski on how far to take them before v3.

[MaximMoinat]

This makes it clear we need a dedicated check for future dates. Thanks for your analysis.

[developer-rpai]

Rebased onto latest main, merge conflicts resolved (upstream's CRAN threshold refresh touched the same CSV rows; kept their row content with the @futureDate substitution). Also applied the same substitution to the new OMOP_CDMv5.5 field-level file so v5.5 runs are deterministic as well, and extended the no-GETDATE() test to cover 5.5.