-
Notifications
You must be signed in to change notification settings - Fork 84
Feature/clickhouse connector #1090
Feature/clickhouse connector #1090
Conversation
release: v0.10.1
✅ Deploy Preview for frontend-sb canceled.
|
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.
Hey @MahmoudElhalwany, thank you very much for your contribution! This looks great overall and everything seems to be working (I tested on a local clickhouse set up). The icon is good too.
That said, I had a few comments and questions on the changes. Please find them below.
Co-authored-by: Samyak Sarnayak <samyak201@gmail.com>
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!
It would be helpful if you could show the error you got when trying the native client. I'll try it from my side too.
Okay, but there is a potential bug too |
Looks like there is an open PR for this: xzkostyan/clickhouse-sqlalchemy#184 Is this something that is necessary for your usecase? Did you find any bugs specific to the native interface/connector? |
@Samyak2 |
Oh I see. By initialization do you mean adding the data source? Or is this after adding the KPI?
No, the native connector works for me too. Although, I had to change the port to 9000 instead of 8123 for HTTP. |
so should I change the https to native or u gonna change it ? |
I'll do it |
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 good! Thank you for contributing this connector. I'm sure the community will benefit greatly from this.
We'll update clickhouse-sqlalchemy
to the next version which will likely include the fixes for Int128
when it's released.
thanks -- this is my first real contribution to open source project -- so I must make it an anniversary 😄 |
#1085