Conversation
matteocavo
left a comment
There was a problem hiding this comment.
Ho controllato la PR e la direzione mi sembra giusta: i test mirati passano e anche i due --dry-run reali citati nel body reggono.
C'è però un caso che oggi mi sembra ancora bloccante prima del merge.
Nel nuovo sql_dry_run, il placeholder di raw_input viene costruito usando:
clean.read.columns, se presenti;- gli identifier tra doppi apici trovati nel SQL.
Questo però genera un falso positivo su clean.sql validi che usano colonne non quotate senza dichiararle in clean.read.columns.
Repro minimo che ho provato:
clean.sql:select x as value from raw_inputmart.sql:select * from clean_input
Con la PR attuale, run all --dry-run fallisce con un binder error su x, anche se quel clean.sql è valido in esecuzione reale.
Quindi, per come la vedo io, la PR è quasi pronta ma non ancora da mergiare così com'è:
serve almeno coprire questo caso o restringere meglio il contratto del dry-run per non introdurre falsi positivi.
|
Il finding di matteocavo era corretto e il caso repro e' ora coperto. Il commit Il repro di matteocavo:
Il nuovo test Una nota sui limiti residui: il fallback e' deliberatamente approssimativo — se il clean SQL ha un errore reale che produce un binder error su una colonna non di raw_input, il retry potrebbe in teoria aggiungerla al placeholder e far passare il dry-run quando non dovrebbe. E' un falso negativo possibile ma raro in pratica, e gia' documentato nel commento del codice. Il dry-run resta una rete di sicurezza per gli errori comuni, non una validazione completa. Per il resto la PR mi sembra pronta. |
Cosa cambia
Verifica
Note
Closes #49