[GitHub] [airflow] JavierLopezT commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook

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

JavierLopezT commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-752924290


   > ok so i have a different concern with this change.
   > 
   > you are adding a global config option `lowercase_columns` (global in the context of the hook)
   > 
   > but this only applies to the narrow case of the `get_pandas_df` method. e.g. it does not apply to `get_records`. or `fetchall` on a cursor.
   > 
   > so i would vote _not_ to approve this change
   > 
   > since this isn't a snowflake parameter, and can't be applied uniformly, i think it's the kind of change best left to be implemented in your own subclass --- not something in the airflow version.
   > 
   > people will not easily discover the functionality, and will find that it has only limited use, so not worth it IMO.
   
   I agree that it was not ideal to have it as a global config option for just one method. However, I do think that this can be useful for more people, since it's pretty common to migrate from other databases to Snowflake and this will make things easier. 
   
   Thus, I have changed the argument from connection to get_pandas_df method and I have added a little bit of documentation. I also agree that this could be potentially hidden from users, so I would be pleased to write more documentation somewhere else (I don't know where though)


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