-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
feat: Add ByteHouse database connector #23140
base: master
Are you sure you want to change the base?
Conversation
bf75601
to
bb95361
Compare
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.
Thanks for adding support for ByteHouse! Without implementing time grain functions or datetime conversion logic I fear this spec will not be very useful. I recommend looking into how other engines are implementing the _time_grain_expressions
(as an example, I assume you should map at least P1Y
to toStartOfISOYear({col})
based on https://docs.bytehouse.cloud/en/docs/date-times#tostartofhour). Also, check out convert_dttm
, where you should at least handle toDate
and toDateTime
.
@villebro Thank you for your suggestion. I have added the conversion for |
Looks like this one got away from us a bit! @rafsan-mazumder would you be willing to give this a rebase? |
Running CI 🤞 |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #23140 +/- ##
===========================================
+ Coverage 60.48% 83.67% +23.19%
===========================================
Files 1931 519 -1412
Lines 76236 37512 -38724
Branches 8568 0 -8568
===========================================
- Hits 46114 31390 -14724
+ Misses 28017 6122 -21895
+ Partials 2105 0 -2105
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
import pytest | ||
|
||
from tests.unit_tests.db_engine_specs.utils import assert_convert_dttm | ||
from tests.unit_tests.fixtures.common import dttm |
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.
from tests.unit_tests.fixtures.common import dttm |
The ruff
linter says this isn't needed.
SUMMARY
feat: Add ByteHouse database connector https://bytehouse.cloud/
TESTING INSTRUCTIONS
Manual Testing
ADDITIONAL INFORMATION