[GitHub] [airflow] dstandish removed a comment on issue #5688: [AIRFLOW-5073] Treat NULL as fail in SQL sensor

[GitBox <gi...@apache.org>]

dstandish removed a comment on issue #5688: [AIRFLOW-5073] Treat NULL as fail in SQL sensor
URL: https://github.com/apache/airflow/pull/5688#issuecomment-522292839
 
 
   >  don't you think changing the default behavior would be a good idea? 
   
   So, disclosure, I don't use sql sensor.  But here are my thoughts.
   I agree the current design is quite awkward: `0` and `''` mean "not done", yet `NULL` means "done".
   
   Having looked into this more, if anything I'd actually vote do _not_ add `allow_null` parameter; just add `None` to the "Not done" list, i.e. as though this were a bugfix.  The sensor is already confusing enough and if someone really needs `None` to mean "done" they can add a success callable e.g. `lambda x: str(x) == 'None'.  But I'm not that well versed in norms re changing behavior in a project like this.
   
   In any case, I think it is misleading / confusing to say "treat null as fail".  In the context of sensors, I think fail means raise exception.  E.g. if the failure callback evaluates to True then an exception is raised.  What we're talking about here though is the "keep poking" list, i.e. whether a `None` results in a True or False, i.e. success or keep poking.  Maybe this is nit picky but I think something like "Do not interpret NULL as success" would be more accurate.
   
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services