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

Several fixes and additions #153

Open
wants to merge 20 commits into
base: main
Choose a base branch
from
Open

Conversation

gallejesus
Copy link

We have created this PR to fix some issues we have found when workig with ClickHouse target.

We can discuss them more in depth but main changes include the addition of tests covering more data types, explicit override of sql data type conversion and manage of nulls and keeping original name from source.

Let me know what you think of those :)

Thank you!

@gallejesus
Copy link
Author

Hello! Have you been abel to take a look at the changes or want a bit more details on them? :)

@BTheunissen
Copy link
Contributor

Hi @gallejesus, apologies I've been on leave for the past few weeks and I miss some of these notifications. Can review this now.

Copy link
Contributor

@BTheunissen BTheunissen left a comment

Choose a reason for hiding this comment

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

The changes look great, thanks for adding the comprehensive type tests. Once the date type is mapped to Date32 this should be good to merge.

return nullabilizer(jsonschema_type, t.cast(sqlalchemy.types.TypeEngine, sqlalchemy.types.DATETIME()))
if datelike_type in "time":
return nullabilizer(jsonschema_type, t.cast(sqlalchemy.types.TypeEngine, sqlalchemy.types.TIME()))
if datelike_type == "date":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you change this to use the newly added Date32 Clickhouse SQLAlchemy type I added, as the Clickhouse Date type doesn't support date values before epoch I switched to using that type to be able to ingest dates in the past.

Copy link
Author

Choose a reason for hiding this comment

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

Hi, great! Let me take a look at it and change it :)

Copy link
Author

Choose a reason for hiding this comment

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

Hi @BTheunissen , I had time now to check it and I think the last commit fixes it, using the Date32 added in clickhouse-sqlalchemy v0.3.2. Can you check it? Thanks!

@@ -107,13 +153,6 @@ def to_sql_type(self, jsonschema_type: dict) -> sqlalchemy.types.TypeEngine:
sqlalchemy.types.TypeEngine,
Copy link
Author

Choose a reason for hiding this comment

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

I have kept the Date32 here, as I believe is the place where the datatype to ch is created, did you mean this? (I have removed the nulls over the date and time types as the nulls are managed in the nullabilizer function)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants