Add DuckDB support to the Amazon provider - #73301
Conversation
Hey @ashb, this was already mentioned in the DISCUSS thread (which many people have plus one'd). Many many services can connect with DuckDB, we cannot have the vendor neutral Hook support all of them. Both from a neutrality standpoint (CC @eladkal) and also the shear number of lines of code, it would be a mess. It is quite reasonable for provider packages that are interested in having a specific DuckDB operator to be free to add one. |
|
@o-nikolas A discussion thread is just that, a discussion, and not a detailed design discussion. Doubly so as in that discuss list you said nothing about "AwsDuckDBOperator". The main thing I don't like about this is, as a user, why do I need to go "okay, I've developed this against local files and it works, now I need to swap it to a different operator to deploy to prod." It really feels like it should be "I just change the connection". Why can't it work this way? It's a much nicer user experience.
I never said the hook code has to live in the DuckDB provider, this is all about the user experience - i.e. operators.. Having an dedicated operator for DuckDB on S3 is an anti-pattern that sets a bad precedent. |
301e6bf to
8e60163
Compare
|
Hey @ashb, thanks for staying engaged on this one!
I think both are a minor changes to a dag and I'm not sure that one could argue one is much worse than the other realistically. Also it's a false premise, any operator based off the vendor neutral operator works with all the local development. If you knew you wanted to work with S3, you could begin your local testing with the But any who, I assume you will still disagree. So I pushed the changes to update the |
Hey @potiuk I already made the changes @ashb was asking for and moved the S3 support into the DuckDB provider? Following the same connection driven hook selection as we use for for BaseSQLOperator._hook. If you also agree with Ash, and I've already implemented those changes, is there something else you'd like to discuss here? |
potiuk
left a comment
There was a problem hiding this comment.
Ah.. Looked at the original commit. That looks good now
|
Myself and a few others have been playing with this new connection driven approach and it works. But it's been a little confusing for folks and explaining it to them has been tricky. The questions that keep coming up are all variations of "so what do I actually have to create and why?":
None of that is impossible to figure out, but it has proven confusing so far. So in the name of user experience (the original objection) what do folks think about keeping the connection-driven SQL selection as it is now and also adding the thin AwsDuckDBOperator (that packages up this connection work for you, no empty connections needed to be created). It forces nobody to swap operators, so Ash's original concern is met, but it gives people an easier route if they just want basic functionality quickly? Both of them life side by side just fine. |
|
Can this (be made to) work: with a connection defined as "duckdb_conn" {
"extensions": ["aws"], // Or "s3" as you like
"database": "s3://bucket/db.duckdb",
"aws_conn_id": "some_aws_conn", // Only needed if not aws_default or not ambient worker creds.
}yielding: DuckDBExecuteQueryOperator(
conn_id="duckdb_conn" ,
sql="SELECT ...",
) |
ashb
left a comment
There was a problem hiding this comment.
We should probably cross link back to providers/amazon/docs/operators/duckdb.rst from in the duckdb provider docs too.
I'm still not sure about the idea of the duckdb_aws, connection type, but having no Aws specific query operator is a massive improvment, thank you!
8e60163 to
748f3e6
Compare
|
Hey @ash,
Sure, done. As well as the other comments above, either code changes made or a reason provided why not.
Unfortunately, not really, the I agree with you about the connection (read my comment above here) which is why I simply wanted a single/small operator to be the mechanism to point duck to AWS-land for users that just need a convenient way to have AWS creds wired up for them. GCS can add their own operator (instead of their own connection), etc, etc. The connection approach works, it has it's pros and cons, and I'm fine to keep it but I think it's ultimately more tedious IMHO. |
|
All the feedback is addressed. Hoping to merge this one in the next day or two with this connection approach instead of the operator @ashb, let me know if you have any other requests. |
|
This PR has been blocked a while. All the comments are addressed and the new approach suggested by @ash was used. I think it's a good compromise from the original approach. Going to give it one more day before merging if you want to take a last minute look! Otherwise we can always iterate further afterwards |
|
Quickest fix: git fetch upstream main && git rebase upstream/main
rm uv.lock && uv lock
git add uv.lock && git rebase --continue
git push --force-with-leaseAutomated nudge — ignore if you're not ready to rebase. This comment is updated in place on future |
DuckDB reads from S3 directly, but its httpfs extension has its own HTTP client that doesn't implement the ECS container credential provider, so an ambient task role is invisible to it and S3 access fails with an opaque HTTP 403. This brokers credentials from the Airflow AWS connection into the DuckDB session instead, so a Dag author only supplies SQL. The AWS-specific part lives here rather than in the duckdb provider, matching the split used for OpenSearch: the underlying technology stays vendor-neutral and the managed-service glue sits with the vendor. DuckDB stays an optional extra so it's never a hard dependency here, and no duckdb connection type is registered.
Having an AwsDuckDBOperator meant a Dag developed against local files had to change code to deploy against S3, which is the wrong seam. DuckDBExecuteQueryOperator now resolves its hook from the connection, so the AWS hook is reached by pointing at a duckdb_aws connection and the Dag itself doesn't change. This is how the provider already handles redshift and athena, where the generic SQL operator works off the connection type. AwsDuckDBOperator is gone. The AWS code stays here in the Amazon provider, so the duckdb provider still knows nothing about AWS. Two things fell out of it: - The operator now guards what it resolves. Selecting a hook by connection type means a wrong type would otherwise hand back a foreign hook, so anything that isn't a DuckDBHook is rejected with a message naming the connection. - DuckDBHook took duckdb_conn_id's default from a parameter default, which binds that class's value, so a subclass declaring its own default_conn_name silently got duckdb_default. It's resolved at runtime now.
748f3e6 to
8972ea2
Compare
Okay removing the request for change and merging this one now, see the justification above 🙂 There was also an approval of the new connection approach from @potiuk |
All the comments are addressed and the new approach (the request change) by @ash was used. Ash reviewed the requested change and was overall happier with it and left a few more questions that were answered.
A week (with reminders throughout) was given to hear back again. Also when initially reaching out to Ash over Slack it was made clear the original feedback wasn't a veto (but either way, the requested change was made).
Backport failed to create: v3-3-test. View the failure log Run detailsNote: As of Merging PRs targeted for Airflow 3.X In matter of doubt please ask in #release-management Slack channel.
You can attempt to backport this manually by running: cherry_picker cc19675 v3-3-testThis should apply the commit to the v3-3-test branch and leave the commit in conflict state marking After you have resolved the conflicts, you can continue the backport process by running: cherry_picker --continueIf you don't have cherry-picker installed, see the installation guide. |
DuckDB reads from S3 directly, but its httpfs extension has its own HTTP client that doesn't implement the ECS container credential provider, so an ambient task role is invisible to it and S3 access fails with an opaque HTTP 403. This brokers credentials from the Airflow AWS connection into the DuckDB session instead, so a Dag author only supplies SQL.
The AWS-specific part lives here rather than in the duckdb provider, matching the split used for OpenSearch: the underlying technology stays vendor-neutral and the managed-service glue sits with the vendor. DuckDB stays an optional extra so it's never a hard dependency here, and no duckdb connection type is registered.
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.