[GitHub] [airflow] marcosmarxm commented on pull request #15367: Implement BigQuery Table Schema Patch Operator

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

marcosmarxm commented on pull request #15367:
URL: https://github.com/apache/airflow/pull/15367#issuecomment-822602690


   > @marcosmarxm
   > Before I change more; I believe these are the three outstanding items from your review. How strongly do you feel about these? As in - before I go ahead and modify I'm keen to present some "why's" for my decisions...
   > 
   > How important would you say it is to move functionality into a hook? Solving the use-case was easy using two already existing hooks and I don't think I need to create an additional hook given I don't implement any new functionality in how we talk with BigQuery
   > 
   > Further I think mutating a mutable object shouldn't be a problem in this case and I am not too keen to deepcopy it for the sake of it.
   > 
   > Lastly the naming of the schema_fields parameter, I wanted to be consistent with naming and input format of that field between this and other operators, do you think it's a problem? Changing it into updates_to_schema_fields or similar is an easy possibility.
   
   1. This is my point of view, but makes more sense you create a function call `update_schema` in the BigQueryHook. Why? Follow the convention, all functions that interact with bigquery are in the Hook not with Operators, in the future if someone search for a method to update_schema in the hook they won't find it. I know that the function you created doesn't interact directly with bigquery but you are creating the action `update_schema` and this interacts. Imagine a user reading the docs: [BigQuery Hook](https://airflow.apache.org/docs/apache-airflow-providers-google/stable/_api/airflow/providers/google/cloud/hooks/bigquery/index.html) and [BigQuery Operators](https://airflow.apache.org/docs/apache-airflow-providers-google/stable/_api/airflow/providers/google/cloud/operators/bigquery/index.html). Maybe you can talk to a PMC/Committer to get better feedback on this topic =D
   2. It's not a problem and maybe tricky is not the best word to describe. It's just a suggestion to be more readable and easy to follow the `execute` flow. Again maybe a PMC/Committer can you better on this also
   3. I read other operators and it's not a problem keeping the `schema_fields`. It's a suggestion to think more about users because keep the same name users think they need to send the info as in other Operators? (so it's a minor in my opinion)
   
   The CI broke because of pylint can you run pre-commit locally to organize imports?


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