-
Notifications
You must be signed in to change notification settings - Fork 4.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨ New: Snowflake Cortex Destination 🚀 #36807
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
airbyte-integrations/connectors/destination-snowflake-cortex/metadata.yaml
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! A few changes requested inline, but lmk if you want to push back on any of them, and/or defer.
The one that is probably hardest to implement (and also hardest to change later) is the layout of the password
input in the config spec. Happy to help on that if needed.
airbyte-integrations/connectors/destination-snowflake-cortex/metadata.yaml
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
...-integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/config.py
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
...integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/indexer.py
Outdated
Show resolved
Hide resolved
class SnowflakeCortexIndexingModel(BaseModel): | ||
account: str = Field( | ||
..., | ||
title="Account", | ||
airbyte_secret=True, | ||
description="Enter the account name you want to use to access the database.", | ||
examples=["xxx.us-east-2.aws"], | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I compared these with the Java-based snowflake destination to see if these parameters match the similar destination. If we can make the auth/config flow consistent between the two, this could help end users who might end up using both for different use cases.
It looks like all of our specified inputs match except account, which the Java Snowflake destination calls host
("Host").
Also, it looks like we are not accepting schema
("Default Schema") and we should probably add that so users can control which schema is written to.
The last difference I noted was that the way we are configuring password is slightly different, just because the Java destination supports more auth options (OAuth and Key Pair private key). Even if we just have a single auth option, it probably makes sense to try to mirror the same structure, which is to place "password" under a "credentials" object. The challenge here is that we can't necessarily copy code from the Snowflake source - since it is in Java, but maybe another Python connector that supports multiple auth flows could be a model here.
Here is the spec.json
for destination-snowflake
: https://github.com/airbytehq/airbyte/blob/72b083cbd259b3d922e522bee70e81d60f88f481/airbyte-integrations/connectors/destination-snowflake/src/main/resources/spec.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed! The parameters should ideally match the snowflake destination. Made the following changes:
- replaced "account" with "host" for UI.
- reordered params to match the order in snowflake destination
- added "default_schema" field
- added a new credentials section with password field.
Implementing the Oauth option seems to much at the moment, but above changes should help us do so in future.
airbyte-integrations/connectors/destination-snowflake-cortex/integration_tests/spec.json
Show resolved
Hide resolved
airbyte-integrations/connectors/destination-snowflake-cortex/integration_tests/spec.json
Show resolved
Hide resolved
@aaronsteers PR is ready for another look.
|
airbyte-integrations/connectors/destination-snowflake-cortex/metadata.yaml
Outdated
Show resolved
Hide resolved
"database": "MYDATABASE", | ||
"default_schema": "MYSCHEMA", | ||
"username": "MYUSERNAME", | ||
"password": "xxxxxxx" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should password
be nested under credentials
now?
Weird that this didn't throw an error.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch.. this file is for reference purpose, doesn't get used :) i will update it.
...-integrations/connectors/destination-snowflake-cortex/destination_snowflake_cortex/config.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🚀
I think you can merge when ready.
…estination_snowflake_cortex/config.py Co-authored-by: Aaron ("AJ") Steers <aj@airbyte.io>
…etadata.yaml Co-authored-by: Aaron ("AJ") Steers <aj@airbyte.io>
Related to: airbytehq/PyAirbyte#120
Destination is working as expected. All unit tests and integration tests are passing. Majority of the code is generated from the
vector-db SDK
, or taken from Pinecone destination. The most interesting bit (write logic), specific to Cortex is inSnowflakeCortexIndexer
class.To be done as fast follow