Return destination GCS URIs from ADLSToGCSOperator

[Abhishekmishra2808]

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.

[SameerMesiah97]

Looks good overall. But this would benefit from some more polish.

[SameerMesiah97]

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?

[SameerMesiah97]

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.

[shahar1]

nit for the nit: I assumed you meant PosixPath from Pathlib :)
The odd contributor won't be odd if #10388 gets resolved (not that I think it will happen anytime soon)

[SameerMesiah97]

nit for the nit: I assumed you meant PosixPath from Pathlib :) The odd contributor won't be odd if #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.

[shahar1]

Please resolve comment made by @SameerMesiah97 (great review!),
and I think that it will be good to go.
I recall that we should run the system tests before merging this PR to ensure that they don't break, please let us know if you're able to do it - otherwise Google team or someone else will have to do it.

[shahar1]

nit for the nit: I assumed you meant PosixPath from Pathlib :)
The odd contributor won't be odd if #10388 gets resolved (not that I think it will happen anytime soon)

[Abhishekmishra2808]

@shahar1 @SameerMesiah97 Could you please check now, and tell if something is missing on my end.

[shahar1]

Looks good!
We need to run the system tests to ensure that nothing breaks (+attach screenshots).

@yuseok89 - if you could help with it, it would be great.

CC: @VladaZakharova @MaksYermak

[yuseok89]

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