-
Notifications
You must be signed in to change notification settings - Fork 105
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
Remove default value checks #852
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #852 +/- ##
==========================================
- Coverage 87.72% 87.71% -0.02%
==========================================
Files 129 129
Lines 11475 11476 +1
Branches 1545 1545
==========================================
- Hits 10067 10066 -1
- Misses 1020 1022 +2
Partials 388 388
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Deploying datachain-documentation with
|
Latest commit: |
a5248d0
|
Status: | ✅ Deploy successful! |
Preview URL: | https://5a2ee850.datachain-documentation.pages.dev |
Branch Preview URL: | https://ilongin-11161-clickhouse-nul.datachain-documentation.pages.dev |
@@ -276,8 +276,9 @@ def has_table(self, name: str) -> bool: | |||
) | |||
return bool(next(self.execute(query))[0]) | |||
|
|||
def create_table(self, table: "Table", if_not_exists: bool = True) -> None: | |||
def create_table(self, table: "Table", if_not_exists: bool = True) -> "Table": |
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.
Changing this as it makes code better in Studio part where table columns are modified to allow nullable
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.
👍
Can we introduce something like |
The thing here is actually this is not null vs not null constraint, but making sure we don't rely on Clickhouse defaults when we insert empty value (
The simplest solution is to make all columns nullable in CH, which is what this (and Studio companion) PR is up to, as having those default values is not correct and expected from user perspective. The only drawback is performance implication as it could slow down queries by double, according to what people are saying. Your suggestion goes one step further -> adding a new feature to the system for setting NULL constraints in user model fields. This is easy to do in SQLite, but in Clickhouse we need to add special constraints or maybe this setting although it seems like it's global and we need it for specific columns Regarding the joins, as they were also mentioned multiple times in our calls, we have special global setting join_use_nulls set to |
Companion issue of https://github.com/iterative/studio/pull/11236
With making CH accept nullable columns, we no longer need special checks for default values in tests, as they will be
None
always.