You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/12/18 13:27:09 UTC
[GitHub] [airflow] JavierLopezT opened a new pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
JavierLopezT opened a new pull request #13152:
URL: https://github.com/apache/airflow/pull/13152
In snowflake, everything is uppercase. In get_pandas_df this could be inconvenient. Sometimes you have to access certain columns, and you don't want to type the name of the columns in uppercase. This is especially inconvenient if you are migrating from Redshift or another DW to Snowflake, because you have to change all your pandas code.
I aim to solve this with this PR. If you include a boolean in the extra field of your snowflake connection as True, all the columns will get lowercase automatically.
I guess this needs test, but I have no idea how to begin. Could anyone help me, please? Thanks
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish edited a comment on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-748350061
ok but i see now you made it optional :)
so maybe that's ok
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-753166870
Let me caveat that I am not a committer and can't merge this anyway, so my opinion is not decisive. I just took a look cus I have experience with snowflake and can help review.
And let me also say that I totally get the impulse to add this -- as someone who has used snowflake for a couple years I also was annoyed by the weird handling of case.
And also, let me share that I know what it's like to try to contribute or propose something and get a "meh".
That all having been said, I just don't think this should be added. It's just too niche, and it's not worth the complexity and confusion it adds. True it's not a _lot_ of complexity, but it's another configurable thing, and it is nonstandard, not part of snowflake api, not part of pandas api, and not supported in other hook methods.
I think it is best to simply accept snowflake as it is. `get_pandas_df` returns the naming that snowflake gives it. If they are lower and quoted in the system, they'll be lower in pandas. And the same is true with fetchall or querying from an IDE or the web UI for that matter.
Snowflake currently has a parameter you can set to make quoted identifiers case insensitive. And when enabled, even quoted identifiers are rendered as upper case. I think the best solution would be if snowflake extended this to make it possible that quoted identifiers are case insensitive _and_ always rendered as lower. But that's an issue for another team :)
Anyway, just my 2 cents. Others very well may feel differently.
----------------------------------------------------------------
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
[GitHub] [airflow] JavierLopezT closed pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
JavierLopezT closed pull request #13152:
URL: https://github.com/apache/airflow/pull/13152
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-754929567
There is a new version of snowflake python connector coming, that we will incorporate very quickly as it solves some really bad behaviour of python connector. I think we should take a look again at the issue when the new version is released.
I also have high hopes we can get much better cooperation between snowflake and open source community using their connectors. They seem to release that this is important for them.
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish edited a comment on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-753166870
Let me caveat that I am not a committer and can't merge this anyway, so my opinion is not decisive. I just took a look cus I have experience with snowflake and can help review.
And let me also say that I totally get the impulse to add this -- as someone who has used snowflake for a couple years I also was annoyed by the weird handling of case.
And also, let me share that I know what it's like to try to contribute or propose something and get a "meh".
That all having been said, I just don't think this should be added. It's just too niche, and it's not worth the complexity and confusion it adds. True it's not a _lot_ of complexity, but it's another configurable thing, and it is nonstandard, not part of snowflake api, not part of pandas api, and not supported in other hook methods.
I think it is best to simply accept snowflake as it is. `get_pandas_df` returns the naming that snowflake gives it. If they are lower and quoted in the system, they'll be lower in pandas. And the same is true with fetchall or querying from an IDE or the web UI for that matter.
Snowflake currently has a parameter you can set to make quoted identifiers case insensitive. And when enabled, even quoted identifiers are rendered as upper case. I think the best solution would be if snowflake extended this to make it possible that quoted identifiers are case insensitive and always rendered as _lower_. But that's an issue for another team :)
Anyway, just my 2 cents. Others very well may feel differently.
----------------------------------------------------------------
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
[GitHub] [airflow] potiuk commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-785324499
Yeah. The new version of python connector is out and we switched to it. But after looking closely - I also think we should not merge this one - similarly to Daniel, this is -0 for me. Since everything we do is python, overriding the get_pandas_df in your own operator is almost as easy as passing a parameter and it gives you much better flexibility.
Also, I think if Snowflake has a captitalised names, the best you can do is to keep it this way throughout the whole "data" journey - to avoid confusion, even for the cases where you have to do any kind of matching/lineage kind of analysis, it simply makes sense to keep it consistent. Adding an "optional parameter" seems like it is not dangerous, but I think we should avoid doing so if the option is "niche" and when you can do it almost as easily by other means.
I think we are all for closing the PR. Are you convinced @JavierLopezT ?1
----------------------------------------------------------------
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
[GitHub] [airflow] JavierLopezT commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-791772817
> Yeah. The new version of python connector is out and we switched to it. But after looking closely - I also think we should not merge this one - similarly to Daniel, this is -0 for me. Since everything we do is python, overriding the get_pandas_df in your own operator is almost as easy as passing a parameter and it gives you much better flexibility.
>
> Also, I think if Snowflake has a captitalised names, the best you can do is to keep it this way throughout the whole "data" journey - to avoid confusion, even for the cases where you have to do any kind of matching/lineage kind of analysis, it simply makes sense to keep it consistent. Adding an "optional parameter" seems like it is not dangerous, but I think we should avoid doing so if the option is "niche" and when you can do it almost as easily by other means.
>
> I think we are all for closing the PR. Are you convinced @JavierLopezT ?1
Sure. I'll close it myself
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish edited a comment on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-753166870
Let me caveat that I am not a committer and can't merge this anyway, so my opinion is not decisive. I just took a look cus I have experience with snowflake and can help review.
And let me also say that I totally get the impulse to add this -- as someone who has used snowflake for a couple years I also was annoyed by the weird handling of case.
And also, let me share that I know what it's like to try to contribute or propose something and get a "meh".
That all having been said, I just don't think this should be added. It's just too niche, and it's not worth the complexity and confusion it adds. True it's not a _lot_ of complexity, but it's another configurable thing, and it is nonstandard -- i.e. it is not part of snowflake api, not part of pandas api, and not supported in other hook methods. Will people even discover the feature?
I think it is best to simply accept snowflake as it is. `get_pandas_df` returns the naming that snowflake gives it. If they are lower and quoted in the system, they'll be lower in pandas. And the same is true with fetchall or querying from an IDE or the web UI for that matter.
Snowflake currently has a parameter you can set to make quoted identifiers case insensitive. And when enabled, even quoted identifiers are rendered as upper case. I think the best solution would be if snowflake extended this to make it possible that quoted identifiers are case insensitive and always rendered as _lower_. But that's an issue for another team :)
Anyway, just my 2 cents. Others very well may feel differently.
In apache language my vote would be -0: "I won't get in the way, but I'd rather we didn't do this."
----------------------------------------------------------------
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
[GitHub] [airflow] eladkal commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
eladkal commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-772396621
I tend to agree with @dstandish
I also don't think this issue is localized only to Snowflake.
I have ETLs where i do something similar with other databases. Nothing prevents you from doing:
```
def snflk_func(**kwargs):
snflk = SnowflakeHook('snowflake_default')
df = snflk.get_pandas_df(sql=sql)
if kwargs['lowercase_columns']:
df.columns = df.columns.str.lower()
# load your df to anywhere you need.
op = PyhtonOperator(task_id='snflk_task',
python_callable=snflk_func,
op_kwargs={'lowercase_columns': True},
dag=dag
)
```
When you use hooks either you wrap it with python function or you inherit from and can overwrite the `get_pandas_df`. In both cases it's very easy to handle the problem you presented.
If this PR proceed forward my only comment is that at least don't call it `lowercase_columns`. It's preferred to call it something like `column_case` that accept lowercase as option. I for example in my code convert to [snakecase](https://en.wikipedia.org/wiki/Snake_case) so if it's accepted better to keep the option open for extentions.
Like @dstandish I'm - 0 for this PR but that is just me.
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-748350061
ok but you made it optional :)
so no concerns with me :)
----------------------------------------------------------------
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
[GitHub] [airflow] github-actions[bot] commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-748100222
[The Workflow run](https://github.com/apache/airflow/actions/runs/430604318) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
----------------------------------------------------------------
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
[GitHub] [airflow] JavierLopezT edited a comment on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
JavierLopezT edited a comment 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). WDYT?
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish edited a comment on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-753166870
Let me caveat that I am not a committer and can't merge this anyway, so my opinion is not decisive. I just took a look cus I have experience with snowflake and can help review.
And let me also say that I totally get the impulse to add this -- as someone who has used snowflake for a couple years I also was annoyed by the weird handling of case.
And also, let me share that I know what it's like to try to contribute or propose something and get a "meh".
That all having been said, I just don't think this should be added. It's just too niche, and it's not worth the complexity and confusion it adds. True it's a _tiny_ amount of complexity. But it's another configurable thing, and it is nonstandard -- i.e. it is not part of snowflake api, not part of pandas api, and not supported in other hook methods. Will people even discover the feature?
I think it is best to simply accept snowflake as it is. `get_pandas_df` returns the naming that snowflake gives it. If they are lower and quoted in the system, they'll be lower in pandas. And the same is true with fetchall or querying from an IDE or the web UI for that matter.
Snowflake currently has a parameter you can set to make quoted identifiers case insensitive. And when enabled, even quoted identifiers are rendered as upper case. I think the best solution would be if snowflake extended this to make it possible that quoted identifiers are case insensitive and always rendered as _lower_. But that's an issue for another team :)
Anyway, just my 2 cents. Others very well may feel differently.
In [apache language](https://www.apache.org/foundation/voting.html) my vote would be -0: "I won't get in the way, but I'd rather we didn't do this."
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish edited a comment on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-753166870
Let me caveat that I am not a committer and can't merge this anyway, so my opinion is not decisive. I just took a look cus I have experience with snowflake and can help review.
And let me also say that I totally get the impulse to add this -- as someone who has used snowflake for a couple years I also was annoyed by the weird handling of case.
And also, let me share that I know what it's like to try to contribute or propose something and get a "meh".
That all having been said, I just don't think this should be added. It's just too niche, and it's not worth the complexity and confusion it adds. True it's not a _lot_ of complexity, but it's another configurable thing, and it is nonstandard -- i.e. it is not part of snowflake api, not part of pandas api, and not supported in other hook methods. Will people even discover the feature?
I think it is best to simply accept snowflake as it is. `get_pandas_df` returns the naming that snowflake gives it. If they are lower and quoted in the system, they'll be lower in pandas. And the same is true with fetchall or querying from an IDE or the web UI for that matter.
Snowflake currently has a parameter you can set to make quoted identifiers case insensitive. And when enabled, even quoted identifiers are rendered as upper case. I think the best solution would be if snowflake extended this to make it possible that quoted identifiers are case insensitive and always rendered as _lower_. But that's an issue for another team :)
Anyway, just my 2 cents. Others very well may feel differently.
In [apache language](https://www.apache.org/foundation/voting.html) my vote would be -0: "I won't get in the way, but I'd rather we didn't do this."
----------------------------------------------------------------
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
[GitHub] [airflow] JavierLopezT commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
JavierLopezT commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-785116632
> There is a new version of snowflake python connector coming, that we will incorporate very quickly as it solves some really bad behaviour of python connector. I think we should take a look again at the issue when the new version is released.
>
> I also have high hopes we can get much better cooperation between snowflake and open source community using their connectors. They seem to release that this is important for them.
Hi Jarek. Any news about this?
----------------------------------------------------------------
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
[GitHub] [airflow] dstandish commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish commented on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-748349379
it's not true that everything in snowflake is uppercase. snowflake renders unquoted identifiers as uppercase. but quoted identifiers it leaves as is.
you can do `create table "MyCrazyTable" ...` and then when you do `select * from mycrazytable` it will fail.
so this would be no problem if you are never quoting identifiers in your code. however, if that's not true it could cause problems.
perhaps a better example is this:
```create temp table _my_table ( "col1" int, col1 int);```
with your change you might run into trouble
----------------------------------------------------------------
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
[GitHub] [airflow] JavierLopezT commented on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by 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
[GitHub] [airflow] dstandish edited a comment on pull request #13152: Convert columns in get_pandas_df to lowercase SnowflakeHook
Posted by GitBox <gi...@apache.org>.
dstandish edited a comment on pull request #13152:
URL: https://github.com/apache/airflow/pull/13152#issuecomment-748350061
ok but i see now you made it optional :)
so no concerns with me :)
----------------------------------------------------------------
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