You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "yuseok89 (via GitHub)" <gi...@apache.org> on 2026/09/17 06:01:57 UTC
[PR] Fix prefix handling in Amazon transfer operators [airflow]
yuseok89 opened a new pull request, #73269:
URL: https://github.com/apache/airflow/pull/73269
<!-- SPDX-License-Identifier: Apache-2.0
https://www.apache.org/licenses/LICENSE-2.0 -->
<!--
Thank you for contributing!
Please provide above a brief description of the changes made in this pull request.
Write a good git commit message following this guide: https://chris.beams.io/posts/git-commit/
Please make sure that your code changes are covered with tests.
And in case of new features or big changes remember to adjust the documentation.
For user-facing UI changes, please attach before/after screenshots (or a short
screen recording) so reviewers can assess the visual impact.
Feel free to ping (in general) for the review if you do not see reaction for a few days
(72 Hours is the minimum reaction time you can expect from volunteers) - we sometimes miss notifications.
In case of an existing issue, reference it using one of the following:
* closes: #ISSUE
* related: #ISSUE
-->
This PR aligns filename prefix handling with the documented behavior of the Amazon FTP and SFTP transfer operators.
Previously, string prefixes were matched anywhere in a filename. This could select unintended files. Renaming also replaced every occurrence of the prefix, including occurrences later in the filename.
The updated behavior:
- Selects only filenames that start with the configured source prefix.
- Replaces only the leading source prefix when a destination prefix is configured.
- Applies consistently to FTP-to-S3, SFTP-to-S3, S3-to-FTP, and S3-to-SFTP transfers.
- Adds regression tests covering both file selection and prefix replacement.
Users relying on the undocumented substring-matching behavior may observe different file selection after this change.
---
##### Was generative AI tooling used to co-author this PR?
<!--
If generative AI tooling has been used in the process of authoring this PR, please
change below checkbox to `[X]` followed by the name of the tool, uncomment the "Generated-by".
-->
- [x] Yes (please specify the tool below)
- gpt-5.6
<!--
Generated-by: [Tool Name] following [the guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#gen-ai-assisted-contributions)
-->
---
* Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/main/contributing-docs/05_pull_requests.rst#pull-request-guidelines)** for more information. Note: commit author/co-author name and email in commits become permanently public when merged.
* For fundamental code changes, an Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvement+Proposals)) is needed.
* When adding dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
* For significant user-facing changes create newsfragment: `{pr_number}.significant.rst`, in [airflow-core/newsfragments](https://github.com/apache/airflow/tree/main/airflow-core/newsfragments). You can add this file in a follow-up commit after the PR is created so you know the PR number.
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck merged PR #73269:
URL: https://github.com/apache/airflow/pull/73269
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "seanghaeli (via GitHub)" <gi...@apache.org>.
seanghaeli commented on PR #73269:
URL: https://github.com/apache/airflow/pull/73269#issuecomment-5766117852
Looks good, pending merge conflicts
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "yuseok89 (via GitHub)" <gi...@apache.org>.
yuseok89 commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4079285165
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/s3_to_ftp.py:
##########
@@ -109,16 +109,25 @@ def execute(self, context: Context):
self.log.info("Getting files in s3://%s/%s", self.s3_bucket, self.s3_key)
all_keys = s3_hook.list_keys(bucket_name=self.s3_bucket, prefix=self.s3_key) or []
filenames = [k[len(self.s3_key) :] for k in all_keys]
- if self.s3_filenames == "*":
+ s3_prefix: str = self.s3_filenames
+ if s3_prefix == "*":
files = filenames
else:
- s3_prefix: str = self.s3_filenames
- files = [f for f in filenames if s3_prefix in f]
+ files = [f for f in filenames if f.startswith(s3_prefix)]
Review Comment:
Thanks, good catch. I went with the file name on the S3 side too, for both matching and renaming, so a nested key keeps its directory.
I was a bit wary that this makes the prefix mean something other than an S3 key prefix. Since these operators bridge S3 to a filesystem, and `s3_key` already carries the directory while `*_filenames` carries the file name, I thought the file name reading fits better here. I noted it in the docstring and the changelog. Happy to revisit if you see it differently. Either way, all four operators now behave the same on layouts with subdirectories.
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "ramitkataria (via GitHub)" <gi...@apache.org>.
ramitkataria commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4051497144
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/ftp_to_s3.py:
##########
@@ -135,17 +135,17 @@ def execute(self, context: Context):
path=self.ftp_path,
)
- if self.ftp_filenames == "*":
+ ftp_prefix: str = self.ftp_filenames
+ if ftp_prefix == "*":
files = list_dir
else:
- ftp_filename: str = self.ftp_filenames
- files = [f for f in list_dir if ftp_filename in f]
+ files = [f for f in list_dir if f.startswith(ftp_prefix)]
Review Comment:
I agreed. Let's not do a deprecation cycle. I would go with the warning log + the basename handling and a changelog note
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "seanghaeli (via GitHub)" <gi...@apache.org>.
seanghaeli commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4050952718
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/ftp_to_s3.py:
##########
@@ -135,17 +135,17 @@ def execute(self, context: Context):
path=self.ftp_path,
)
- if self.ftp_filenames == "*":
+ ftp_prefix: str = self.ftp_filenames
+ if ftp_prefix == "*":
files = list_dir
else:
- ftp_filename: str = self.ftp_filenames
- files = [f for f in list_dir if ftp_filename in f]
+ files = [f for f in list_dir if f.startswith(ftp_prefix)]
Review Comment:
I am against a deprecation cycle for this change, as the docs are already clear in what the function *should* be doing. I'm in favor of logging a warning for users when there exists a non-prefix match that would have happened under the substring behavior prior to this PR.
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "yuseok89 (via GitHub)" <gi...@apache.org>.
yuseok89 commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4045114697
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/ftp_to_s3.py:
##########
@@ -135,17 +135,17 @@ def execute(self, context: Context):
path=self.ftp_path,
)
- if self.ftp_filenames == "*":
+ ftp_prefix: str = self.ftp_filenames
+ if ftp_prefix == "*":
files = list_dir
else:
- ftp_filename: str = self.ftp_filenames
- files = [f for f in list_dir if ftp_filename in f]
+ files = [f for f in list_dir if f.startswith(ftp_prefix)]
Review Comment:
Good points. I agree that the FTP operator should use the basename for matching and replacement while retaining the original path for retrieval. I also agree that changing the matching behavior immediately could surprise existing users and should be noted in the Amazon provider changelog.
The `startswith` change was intended to resolve the mismatch between the documented prefix semantics and the current substring matching.
Would it make sense to preserve substring matching for now, emit a deprecation warning for non-prefix matches, clarify the behavior for nested S3 keys, and switch to prefix-only matching after the deprecation period?
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "yuseok89 (via GitHub)" <gi...@apache.org>.
yuseok89 commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4053255758
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/ftp_to_s3.py:
##########
@@ -135,17 +135,17 @@ def execute(self, context: Context):
path=self.ftp_path,
)
- if self.ftp_filenames == "*":
+ ftp_prefix: str = self.ftp_filenames
+ if ftp_prefix == "*":
files = list_dir
else:
- ftp_filename: str = self.ftp_filenames
- files = [f for f in list_dir if ftp_filename in f]
+ files = [f for f in list_dir if f.startswith(ftp_prefix)]
Review Comment:
Good point, the deprecation window would keep the rename corruption bug alive the whole time. Not worth it. Going with the warning log and basename fix.
- FTP matches/replaces on the basename, not the full listed path
- Warn when the old substring rule would've matched but prefix doesn't
- Changelog note for the behavior change
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "ramitkataria (via GitHub)" <gi...@apache.org>.
ramitkataria commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4077695771
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/s3_to_ftp.py:
##########
@@ -109,16 +109,25 @@ def execute(self, context: Context):
self.log.info("Getting files in s3://%s/%s", self.s3_bucket, self.s3_key)
all_keys = s3_hook.list_keys(bucket_name=self.s3_bucket, prefix=self.s3_key) or []
filenames = [k[len(self.s3_key) :] for k in all_keys]
- if self.s3_filenames == "*":
+ s3_prefix: str = self.s3_filenames
+ if s3_prefix == "*":
files = filenames
else:
- s3_prefix: str = self.s3_filenames
- files = [f for f in filenames if s3_prefix in f]
+ files = [f for f in filenames if f.startswith(s3_prefix)]
Review Comment:
One thing that was left open from the earlier thread: `list_keys(prefix=s3_key)` also returns nested keys, so a relative name like `2024/pre_one.txt` no longer starts with the prefix and ends up in `dropped`. The `"*"` branch in this same operator still transfers those nested keys, and `FTPToS3Operator` would select them via the basename match, so the four operators still disagree on layouts with subdirectories.
Could we either match on `posixpath.basename(f)` here and in `S3ToSFTPOperator` (and do the rename on the basename too, so `replace(..., 1)` cannot hit a directory segment), or state in the changelog that the S3-side prefix applies to the full path relative to `s3_key`? I am fine with either, I mostly want the behavior to be intentional and documented
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/s3_to_ftp.py:
##########
@@ -109,16 +109,25 @@ def execute(self, context: Context):
self.log.info("Getting files in s3://%s/%s", self.s3_bucket, self.s3_key)
all_keys = s3_hook.list_keys(bucket_name=self.s3_bucket, prefix=self.s3_key) or []
filenames = [k[len(self.s3_key) :] for k in all_keys]
- if self.s3_filenames == "*":
+ s3_prefix: str = self.s3_filenames
+ if s3_prefix == "*":
files = filenames
else:
- s3_prefix: str = self.s3_filenames
- files = [f for f in filenames if s3_prefix in f]
+ files = [f for f in filenames if f.startswith(s3_prefix)]
+ dropped = [f for f in filenames if s3_prefix in f and not f.startswith(s3_prefix)]
+ if dropped:
+ self.log.warning(
Review Comment:
Minor: this logs the full `dropped` list at WARNING on every run, and for a bucket or directory where the non-prefix siblings are a permanent, legitimate part of the layout it will keep doing so forever. With nested S3 keys, the list can get long.
Could we log the count plus a bounded sample, say the first 10 entries? Same in the other three operators
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #73269:
URL: https://github.com/apache/airflow/pull/73269#issuecomment-5720720101
@ramitkataria @seanghaeli
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "vincbeck (via GitHub)" <gi...@apache.org>.
vincbeck commented on PR #73269:
URL: https://github.com/apache/airflow/pull/73269#issuecomment-5795628193
Note for the next release manager, this a bug fix. It is a breaking change but considered as bug fix because the doc and code was not matching
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "ramitkataria (via GitHub)" <gi...@apache.org>.
ramitkataria commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4044788981
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/ftp_to_s3.py:
##########
@@ -135,17 +135,17 @@ def execute(self, context: Context):
path=self.ftp_path,
)
- if self.ftp_filenames == "*":
+ ftp_prefix: str = self.ftp_filenames
+ if ftp_prefix == "*":
files = list_dir
else:
- ftp_filename: str = self.ftp_filenames
- files = [f for f in list_dir if ftp_filename in f]
+ files = [f for f in list_dir if f.startswith(ftp_prefix)]
Review Comment:
I am not sure how people use the string form of `*_filenames` in practice, but this could affect a few groups without any error being raised: anyone filtering on a suffix like `".csv"`, S3 users with nested keys under `s3_key`, and FTP servers whose `nlst` returns path-qualified entries, since `FTPToS3Operator` currently passes the listed name straight to `retrieve_file`. In all of those cases the task would now succeed having transferred zero files.
What do you think about:
- Match and replace on the basename in the FTP operator so `startswith` works regardless of `nlst` output shape
- If you think the suffix use case is common, we could discuss whether selection should stay lenient while only the rename is fixed, since the rename corruption is the clearer bug
Depending on the approach, I think we should add a warning in the changelog so users are aware.
--
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] Fix prefix handling in Amazon transfer operators [airflow]
Posted by "yuseok89 (via GitHub)" <gi...@apache.org>.
yuseok89 commented on code in PR #73269:
URL: https://github.com/apache/airflow/pull/73269#discussion_r4079286927
##########
providers/amazon/src/airflow/providers/amazon/aws/transfers/s3_to_ftp.py:
##########
@@ -109,16 +109,25 @@ def execute(self, context: Context):
self.log.info("Getting files in s3://%s/%s", self.s3_bucket, self.s3_key)
all_keys = s3_hook.list_keys(bucket_name=self.s3_bucket, prefix=self.s3_key) or []
filenames = [k[len(self.s3_key) :] for k in all_keys]
- if self.s3_filenames == "*":
+ s3_prefix: str = self.s3_filenames
+ if s3_prefix == "*":
files = filenames
else:
- s3_prefix: str = self.s3_filenames
- files = [f for f in filenames if s3_prefix in f]
+ files = [f for f in filenames if f.startswith(s3_prefix)]
+ dropped = [f for f in filenames if s3_prefix in f and not f.startswith(s3_prefix)]
+ if dropped:
+ self.log.warning(
Review Comment:
Makes sense. The warning now keeps the full count and names only the first 10 entries, with the rest summarized as "and N more". Same in all four operators.
--
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