[GitHub] [airflow] potiuk commented on pull request #23132: add numpy as prerequisites for apache-airflow-providers-oracle

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

potiuk commented on PR #23132:
URL: https://github.com/apache/airflow/pull/23132#issuecomment-1104473583

   First of all thanks for the investigation and finding. It's cool. But I think the proposal in it's current state is not good. 
   
   First of all - this is not how it works.  the index is automatically generated based on "setup.py" - so it will be overridden next time we generate the index, it will override the change. So any change that changes the permanent list of dependencies should go there.
   
   Secondly I **think** this is a not the best solution. When I look at the Oracle Hook numpy is used in one  place in insert_rows
   
   ```python
                   elif isinstance(cell, float) and numpy.isnan(cell):  # coerce numpy NaN to NULL
                       lst.append('NULL')
                   elif isinstance(cell, numpy.datetime64):
   ```
   
   Those are all usages of numpy in oracle hook.
   
   I think it does not really justify the usage :).
   
   There are several ways it can be improved:
   
   1) Changing numpy to local import in insert_rows will immediately make it "work" in terms of connection availabe in UI - this will not fix insert_row() but for example bulk_insert will work
   
   2) Possibly whatever isnan and numpy.datetime64 usage can be avoided or at least workarounded. If someone would not use numpy at all, you could rewrite the method to skip numpy checks if numpy is not installed. Not sure how - but I think adding "huge" numpy dependency to check the type of data passed seems terribly wrong.


-- 
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.

To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org