Skip to content
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

fix: [sql_database] Handle type ARRAY #547

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

vivingo
Copy link

@vivingo vivingo commented Aug 2, 2024

Tell us what you do here

  • Handle type ARRAY in source sql_database and map it to data_type "complex"

Short description

Type ARRAY (postgresql) was ignored and its data_type field was not filled.
-> is_complete_column (in dlt.common.destination.reference) is False and we have a further error during the load as the column is not recognized.

Related Issues

Additional Context

I only tried it with postgresql source and I hope this will not break other sources, but sqlalchemy's ARRAY Class documentation says (sqlalchemy.sql.sqltypes.ARRAY)

only the PostgreSQL backend has support for SQL arrays in SQLAlchemy

Also I thought more logical to map it to "complex", like json types

@rudolfix rudolfix added the ci from fork Allows to run tests from PR coming from fork label Aug 8, 2024
Copy link
Contributor

@rudolfix rudolfix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for this fix! Implementation itself is good and I expect it to work. Please see the review, tests should run automatically this time. Let's make sure that arrow backend also works...

tests/sql_database/sql_source.py Outdated Show resolved Hide resolved
@rudolfix rudolfix added ci from fork Allows to run tests from PR coming from fork and removed ci from fork Allows to run tests from PR coming from fork labels Aug 9, 2024
@vivingo vivingo force-pushed the fix/sql-database-handle-arrays branch from 0870857 to b08c113 Compare August 14, 2024 14:01
Virginie Ngo added 2 commits August 14, 2024 16:03
…ypes

as it's now supported and we want to keep the test for pyarrow backend
@vivingo vivingo force-pushed the fix/sql-database-handle-arrays branch from b08c113 to f186754 Compare August 14, 2024 14:03
@rudolfix rudolfix added ci from fork Allows to run tests from PR coming from fork and removed ci from fork Allows to run tests from PR coming from fork labels Aug 14, 2024
@rudolfix
Copy link
Contributor

@vivingo are we able to push to your fork? we are happy to look at the tests and fix them

@vivingo
Copy link
Author

vivingo commented Aug 18, 2024

@rudolfix yes, I enabled the "Allow edits by maintainers" checkbox.
Do I need to give write access on my fork repository as well, or will it be done automatically with this box checked?
Thank you !

(I don't understand why my tests succeed when I test locally, sorry for that. I just pushed a new change but I guess I might have some further errors)

@vivingo
Copy link
Author

vivingo commented Oct 14, 2024

@rudolfix any update on this ?
Do I need to give other authorizations on my fork ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci from fork Allows to run tests from PR coming from fork
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants