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 2022/05/13 08:54:21 UTC

[GitHub] [airflow] jaegwonseo opened a new pull request, #23695: Sql to gcs with exclude columns

jaegwonseo opened a new pull request, #23695:
URL: https://github.com/apache/airflow/pull/23695

   issue : #23694 
   add arg exclude columns to BaseSQLToGCSOperator 
   to exclude the content of certain columns, such as personal information


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

[GitHub] [airflow] turbaszek commented on a diff in pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

turbaszek commented on code in PR #23695:
URL: https://github.com/apache/airflow/pull/23695#discussion_r874103703


##########
airflow/providers/google/cloud/transfers/sql_to_gcs.py:
##########
@@ -103,6 +104,7 @@ def __init__(
         gcp_conn_id: str = 'google_cloud_default',
         delegate_to: Optional[str] = None,
         impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
+        exclude_columns: List[str] = [],

Review Comment:
   Please do not use `[]` as default argument. Use `None` and in constructor do `exclude_columns = exclude_columns or []` as we do in many places.
   
   https://florimond.dev/en/posts/2018/08/python-mutable-defaults-are-the-source-of-all-evil/



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

[GitHub] [airflow] jaegwonseo commented on a diff in pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

jaegwonseo commented on code in PR #23695:
URL: https://github.com/apache/airflow/pull/23695#discussion_r874526859


##########
airflow/providers/google/cloud/transfers/sql_to_gcs.py:
##########
@@ -165,7 +168,9 @@ def _write_local_data_files(self, cursor):
             names in GCS, and values are file handles to local files that
             contain the data for the GCS objects.
         """
-        schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        org_schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        schema = [column for column in org_schema if column not in self.exclude_columns]
+

Review Comment:
   If schema is changed to set type, the order of the columns is changed.
   It seems better to use a list to keep the order of the columns.
   
   however, it seems good to receive the input value as a set to prevent duplication of the input exclude column.
    
    
   ```python
               delegate_to: Optional[str] = None,
               impersonation_chain: Optional[Union[str, Sequence[str]]] = None,
               exclude_columns=None,
               **kwargs,
       ) -> None:
           super().__init__(**kwargs)
           if exclude_columns is None:
               exclude_columns = set()
   ```
    using exclude_columns as a set to prevent duplication 
   



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

[GitHub] [airflow] eladkal commented on pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

eladkal commented on PR #23695:
URL: https://github.com/apache/airflow/pull/23695#issuecomment-1125879625

   gotcha
   
   > Dozens of tables need to be transferred to bigquery while running dw, but since columns are added or deleted from time to time in service tables, the operating cost is too high to change the required columns by changing the dag every time.
   
   That's reasonable


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

[GitHub] [airflow] jaegwonseo commented on pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

jaegwonseo commented on PR #23695:
URL: https://github.com/apache/airflow/pull/23695#issuecomment-1223852769

   @zzsza 
   i'm gonna open a new issue and test it there.
   


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

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

boring-cyborg[bot] commented on PR #23695:
URL: https://github.com/apache/airflow/pull/23695#issuecomment-1133889833

   Awesome work, congrats on your first merged pull request!
   


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

[GitHub] [airflow] boring-cyborg[bot] commented on pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

boring-cyborg[bot] commented on PR #23695:
URL: https://github.com/apache/airflow/pull/23695#issuecomment-1125813354

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, it’s a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better 🚀.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack

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

[GitHub] [airflow] potiuk merged pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

potiuk merged PR #23695:
URL: https://github.com/apache/airflow/pull/23695


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

[GitHub] [airflow] turbaszek commented on a diff in pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

turbaszek commented on code in PR #23695:
URL: https://github.com/apache/airflow/pull/23695#discussion_r874106908


##########
airflow/providers/google/cloud/transfers/sql_to_gcs.py:
##########
@@ -165,7 +168,9 @@ def _write_local_data_files(self, cursor):
             names in GCS, and values are file handles to local files that
             contain the data for the GCS objects.
         """
-        schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        org_schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        schema = [column for column in org_schema if column not in self.exclude_columns]
+

Review Comment:
   Consider casting `excluded_columns` to `set` in constructor. Then you can do:
   ```suggestion
           org_schema = set(schema_tuple[0] for schema_tuple in cursor.description)
           schema = org_schema - self.exclude_columns
   
   ```
   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.

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

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

[GitHub] [airflow] turbaszek commented on a diff in pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

turbaszek commented on code in PR #23695:
URL: https://github.com/apache/airflow/pull/23695#discussion_r874617487


##########
airflow/providers/google/cloud/transfers/sql_to_gcs.py:
##########
@@ -165,7 +168,9 @@ def _write_local_data_files(self, cursor):
             names in GCS, and values are file handles to local files that
             contain the data for the GCS objects.
         """
-        schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        org_schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        schema = [column for column in org_schema if column not in self.exclude_columns]
+

Review Comment:
   Sounds good to me, consider `exclude_columns = exclude_columns or set()`



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

[GitHub] [airflow] jaegwonseo commented on a diff in pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

jaegwonseo commented on code in PR #23695:
URL: https://github.com/apache/airflow/pull/23695#discussion_r875584942


##########
airflow/providers/google/cloud/transfers/sql_to_gcs.py:
##########
@@ -165,7 +168,9 @@ def _write_local_data_files(self, cursor):
             names in GCS, and values are file handles to local files that
             contain the data for the GCS objects.
         """
-        schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        org_schema = list(map(lambda schema_tuple: schema_tuple[0], cursor.description))
+        schema = [column for column in org_schema if column not in self.exclude_columns]
+

Review Comment:
   @turbaszek 
   I changed the input type, please check



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

[GitHub] [airflow] zzsza commented on pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

zzsza commented on PR #23695:
URL: https://github.com/apache/airflow/pull/23695#issuecomment-1223759682

   @jaegwonseo 
   
   Hello :)
   I was also thinking about how to handle columns to be excluded due to privacy issues when bringing data from RDS. Thank you for making a good function.
   
   I have a question while using it.
   
   If a specific column is added to exclude_columns, it seems to be deleted only from the header while leaving the value of the column as it is. I think the value of the column should also be deleted.
   
   (한국인이시니 추가적으로 한국어로 설명하면 현재 오퍼레이터에 값을 추가하면 csv의 header엔 삭제되지만, value들은 그대로 남아있더라구요. 애초에 SELECT 하실 때 특정 컬럼만 가져오시는지 궁금하네요)
   
   
   For example, the result for MySQL data is:
   
    user_id | name | phone | created_at | updated_at | is_agree
    1001 | seongyun | 010-0000-0000 | 2022-01-23 15:01:00 | 2022-08-10 03:00:00 | True
   
   When moving these data to Cloud Storage, exclude_columns = ['name', 'phone'] works.
   
   Expected results are:
   
    user_id | created_at | updated_at | is_agree
    1001 | 2022-01-23 15:01:00 | 2022-08-10 03:00:00 | True
   
   However, if it is currently running, it will be saved as
    user_id | created_at | updated_at | is_agree
    1001 | **seongyun | 010-0000-0000** | 2022-01-23 15:01:00
   
   Maybe there is something I'm misunderstanding?
   Thank you for reading


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

[GitHub] [airflow] jaegwonseo commented on pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

jaegwonseo commented on PR #23695:
URL: https://github.com/apache/airflow/pull/23695#issuecomment-1125873133

   Dozens of tables need to be transferred to bigquery while running dw, but since columns are added or deleted from time to time in service tables, the operating cost is too high to change the required columns by changing the dag every time.
   


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

[GitHub] [airflow] github-actions[bot] commented on pull request #23695: Sql to gcs with exclude columns

Posted by GitBox <gi...@apache.org>.

github-actions[bot] commented on PR #23695:
URL: https://github.com/apache/airflow/pull/23695#issuecomment-1127318518

   The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest main or amend the last commit of the PR, and push it with --force-with-lease.


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