You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "Abhishekmishra2808 (via GitHub)" <gi...@apache.org> on 2026/02/04 18:39:39 UTC
[PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Abhishekmishra2808 opened a new pull request, #61463:
URL: https://github.com/apache/airflow/pull/61463
Related: #11323
- **Return value**: `execute()` now returns a `list[str]` of destination GCS URIs (`gs://bucket/object`) for uploaded files.
- The return type remains `list[str]`; only the returned values are updated to reflect the destination in GCS.
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on code in PR #61463:
URL: https://github.com/apache/airflow/pull/61463#discussion_r2772860680
##########
providers/google/src/airflow/providers/google/cloud/transfers/adls_to_gcs.py:
##########
Review Comment:
nit for the nit: I assumed you meant `PosixPath` from `Pathlib` :)
The odd contributor won't be odd if [#10388](https://redirect.github.com/apache/airflow/issues/10388) gets resolved (not that I think it will happen anytime soon)
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "SameerMesiah97 (via GitHub)" <gi...@apache.org>.
SameerMesiah97 commented on code in PR #61463:
URL: https://github.com/apache/airflow/pull/61463#discussion_r2773825166
##########
providers/google/src/airflow/providers/google/cloud/transfers/adls_to_gcs.py:
##########
Review Comment:
> nit for the nit: I assumed you meant `PosixPath` from `Pathlib` :) The odd contributor won't be odd if [#10388](https://redirect.github.com/apache/airflow/issues/10388) gets resolved (not that I think it will happen anytime soon)
Yes. My point is that path construction should be OS agnostic. If the function you mentioned is the acceptable one, then that is the one the author should use. Thank you for calling that out.
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "Abhishekmishra2808 (via GitHub)" <gi...@apache.org>.
Abhishekmishra2808 commented on PR #61463:
URL: https://github.com/apache/airflow/pull/61463#issuecomment-4009799384
@shahar1 I fixed the backward compatibility issue, please review 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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "SameerMesiah97 (via GitHub)" <gi...@apache.org>.
SameerMesiah97 commented on code in PR #61463:
URL: https://github.com/apache/airflow/pull/61463#discussion_r2770496182
##########
providers/google/src/airflow/providers/google/cloud/transfers/adls_to_gcs.py:
##########
Review Comment:
I would suggest using `posixpath.join(dest_gcs_prefix, obj)` as the behavior of `os.path.join` is OS dependent. On windows, it can result in the `\\` separator being used during construction, resulting in an invalid path. Now, it won't cause issues in 99% of cases but for the odd contributor running this code locally on their windows machine, `os.path.join` can definitely cause issues. This is more of a nit though.
##########
providers/google/src/airflow/providers/google/cloud/transfers/adls_to_gcs.py:
##########
Review Comment:
It looks like `dest_gcs_bucket, dest_gcs_prefix = _parse_gcs_url(self.dest_gcs)` will return the same value regardless of the iteration as `self.dest_gcs` is constant. Any reason why it can't be moved outside the loop?
##########
providers/google/tests/unit/google/cloud/transfers/test_adls_to_gcs.py:
##########
@@ -99,8 +98,18 @@ def test_execute(self, gcs_mock_hook, adls_one_mock_hook, adls_two_mock_hook):
impersonation_chain=IMPERSONATION_CHAIN,
)
- # we expect MOCK_FILES to be uploaded
- assert sorted(MOCK_FILES) == sorted(uploaded_files)
+ # Verify that the return value is a list of destination GCS URIs
+ assert isinstance(uploaded_files, list)
+ assert len(uploaded_files) == len(MOCK_FILES)
+
+ # Each returned value should be a GCS URI starting with gs://
+ for uri in uploaded_files:
+ assert uri.startswith("gs://")
Review Comment:
This is redundant as the assertion on the next line already checks for uris starting with `gs://`. I would advise you to remove it.
##########
providers/google/tests/unit/google/cloud/transfers/test_adls_to_gcs.py:
##########
@@ -138,5 +146,15 @@ def test_execute_with_gzip(self, gcs_mock_hook, adls_one_mock_hook, adls_two_moc
any_order=True,
)
- # we expect MOCK_FILES to be uploaded
- assert sorted(MOCK_FILES) == sorted(uploaded_files)
+ # Verify that the return value is a list of destination GCS URIs
+ assert isinstance(uploaded_files, list)
+ assert len(uploaded_files) == len(MOCK_FILES)
+
+ # Each returned value should be a GCS URI starting with gs://
+ for uri in uploaded_files:
+ assert uri.startswith("gs://")
Review Comment:
Redundant assertion here as well. See the above comment.
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #61463:
URL: https://github.com/apache/airflow/pull/61463#issuecomment-3993470715
@Abhishekmishra2808 I wanted to merge, but realized that we're missing the flag `return_gcs_uris` to ensure backward compatibility with current returned value, as well as aa warning that the default behavior will change in the future (please see other PRs under the same GitHub issue for reference).
Could you please handle it?
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 commented on PR #61463:
URL: https://github.com/apache/airflow/pull/61463#issuecomment-4011251145
Great job Abhishek!
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "shahar1 (via GitHub)" <gi...@apache.org>.
shahar1 merged PR #61463:
URL: https://github.com/apache/airflow/pull/61463
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "yuseok89 (via GitHub)" <gi...@apache.org>.
yuseok89 commented on PR #61463:
URL: https://github.com/apache/airflow/pull/61463#issuecomment-3891166235
@shahar1
While running the system tests, it looks like this should be using ADLS `Gen2` rather than the older Gen1 path.
If I’m mistaken or missing context here, please correct me.
Also, it seems this change may already be reflected in `main`, so my guess is that resolving the conflict (merge `main`) and re-running the system tests could be the next step.
--
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
Re: [PR] Return destination GCS URIs from ADLSToGCSOperator [airflow]
Posted by "Abhishekmishra2808 (via GitHub)" <gi...@apache.org>.
Abhishekmishra2808 commented on PR #61463:
URL: https://github.com/apache/airflow/pull/61463#issuecomment-3861283556
@shahar1 @SameerMesiah97 Could you please check now, and tell if something is missing on my end.
--
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