fix: le SQL temple reproduit la semantique NaN/Inf du kernel R#41
Conversation
…13) Le CASE WHEN ne gerait que NULL ; or NaN est un float ordinaire dans DuckDB (NaN = NaN vrai, NaN > tout) : des Parquet contenant NaN/Inf obtenaient des verdicts inverses en lazy (faux PASS sur NaN/Inf unilateral, faux FAIL sur infinis de meme signe). - case_tol : bilateral NA/NaN -> na_equal, unilateral NA/NaN -> FALSE, infinis de meme signe -> TRUE, infini restant -> FALSE, sinon comparaison finie (le cap du fp_correction non fini est structurel : la branche finie n'est atteinte qu'avec une reference finie) - detection backend : isnan()/isinf() sur DuckDB ; SQLite (pas de stockage NaN, insere NULL) garde les regles NULL et detecte l'infini par comparaison a DBL_MAX - colonnes d'egalite numeriques : regles NA nan-aware (eq_num_cols), alignees sur le chemin local ou is.na(NaN) est TRUE - tests d'equivalence avec le kernel R comme oracle (grille complete NaN/Inf/NA x na_equal, DuckDB + SQLite, egalite numerique NaN, bout-en-bout reprex de l'issue)
There was a problem hiding this comment.
Pull request overview
This PR fixes a correctness gap on the lazy (SQL/dbplyr) path where NaN/Inf were not handled like the local R tolerance kernel, causing local vs lazy verdict divergences (notably on DuckDB with Parquet-origin NaN/Inf). It updates the templated SQL boolean generation to mirror compute_tolerance_col() semantics and adds targeted equivalence tests using the R kernel as oracle.
Changes:
- Update
add_bool_cols_sql()to implement NaN/Inf-aware tolerance/equality CASE logic and addeq_num_colsto safely apply NaN-aware rules only to numeric equality columns. - Thread numeric equality column detection from
compare_datasets_from_yaml()into the SQL template generation. - Add new test coverage for NaN/Inf equivalence (DuckDB + SQLite) and document the behavior in
NEWS.md.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/testthat/test-nan-inf-sql-equivalence.R | Adds equivalence tests (R kernel as oracle) for NaN/Inf/NA behavior on lazy backends. |
| R/tolerance.R | Updates templated SQL boolean generation to match R kernel NaN/Inf semantics; adds numeric-equality NaN-aware path. |
| R/compare_datasets_from_yaml.R | Detects numeric equality columns and passes them to add_bool_cols_sql(). |
| NEWS.md | Documents the NaN/Inf semantics fix for lazy SQL booleans and numeric equality columns. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nan_sql <- function(x) { | ||
| if (is_duckdb) { | ||
| sprintf("isnan(%s)", x) | ||
| } else { | ||
| "FALSE" | ||
| } | ||
| } |
There was a problem hiding this comment.
Partiellement valide, traite par documentation honnete plutot que par du code non teste : PostgreSQL stocke bien NaN, mais il n'existe pas de predicat NaN portable en SQL (en PG, NaN = NaN est TRUE et x <> x ne detecte rien, comme DuckDB). Cabler un cas PG sans le tester en CI serait pire que l'assumer : la roxygen documente desormais la limitation (NaN detection cablee DuckDB uniquement ; un backend non-DuckDB stockant NaN garde les regles NULL-only = semantique pre-fix). Extension possible le jour ou un backend PG teste rejoint la matrice.
| for (na_equal in c(TRUE, FALSE)) { | ||
| expected <- compute_tolerance_col( | ||
| grid$cand, grid$ref, | ||
| abs_tol = 0.1, rel_tol = 0, na_equal = na_equal | ||
| )$ok | ||
| got <- lazy_tol_ok(con, grid$cand, grid$ref, | ||
| abs_tol = 0.1, rel_tol = 0, na_equal = na_equal) | ||
| expect_identical(got, expected) | ||
| } |
There was a problem hiding this comment.
Valide et applique : le test SQLite couvre desormais la grille en tolerance relative (abs = 0, rel = 0.5), symetrique du test DuckDB - une reference infinie ne doit pas laisser passer un candidat fini via un seuil infini malgre la detection DBL_MAX.
Reponse a la review Copilot de la PR : - le test SQLite couvre desormais aussi la tolerance relative (une reference infinie ne doit pas laisser passer un candidat fini via un seuil infini, detection DBL_MAX) - limitation connue documentee : la detection NaN n'est cablee que pour DuckDB (backend lazy teste) ; un backend non-DuckDB qui stocke NaN (PostgreSQL) garde les regles NULL-only sur ces valeurs
…nan-inf-sql # Conflicts: # NEWS.md
Closes #13
Probleme
Le CASE WHEN temple (
add_bool_cols_sql) ne gerait que SQL NULL. Or NaN n'est pas NULL dans DuckDB : NaN est un float ordinaire (NaN = NaNvrai, NaN ordonne au-dessus de tout). Des Parquet contenant NaN/Inf lus nativement obtenaient des verdicts inverses du kernel R en lazy :L'angle mort etait structurel :
test-lazy-sql-bool.Rutilisait l'ancien chemin dplyr comme oracle, qui avait le meme trou.Correctif
case_tol()reproduit le kernel : NA/NaN bilateral -> na_equal, unilateral -> FALSE, infinis de meme signe -> TRUE, infini restant -> FALSE, sinon comparaison finie (le cap du fp_correction non fini est structurel : la branche finie n'est atteinte qu'avec une reference finie)isnan()/isinf()sur DuckDB ; SQLite (pas de stockage NaN : insere NULL) garde les regles NULL et detecte l'infini par comparaison a DBL_MAXcase_eq()+ nouveau parametreeq_num_cols: les colonnes d'egalite numeriques recoivent les regles NA nan-aware (alignees sur le chemin local ouis.na(NaN)est TRUE) ;isnan()sur un varchar serait une erreur de type SQLTests (TDD red-first) - kernel R comme oracle
test-nan-inf-sql-equivalence.R: grille 7x7 {1, 2.5, NA, NaN, Inf, -Inf, 0} x na_equal {T,F} x {abs, rel} sur DuckDB ; grille 6x6 sans NaN sur SQLite ; egalite numerique NaN vs oracle local ; bout-en-bout (reprex de l'issue) avec comptages par colonne verifies sur les deux chemins. 10 rouges avant fix.Suite complete : 890 PASS / 0 FAIL sur la pile.
Review interne
pr-reviewer : APPROVE. Les 5 branches du CASE tracees contre le kernel ; DBL_MAX verifie empiriquement sur RSQLite ; isnan/isinf sur INTEGER auto-cast verifie ; injection impossible (identifiants via dbQuoteIdentifier, numeriques %.17g). Nit applique (arguments nommes sur l'appel modifie).