Psharma/django 5.0 fix #46
Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request adds support for Django's db_default feature, which allows specifying database-level default values for model fields. The PR also upgrades the testing infrastructure to use Django 5.0.x and Python 3.11.
Key changes:
- Implements
db_default_sql()method and integrates it into column creation and field alteration logic - Updates test suite reference from Django 4.2.x to 5.0.x
- Adds test skips for db_default-related tests that involve unsupported complex expressions
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| django_singlestore/schema.py | Added db_default_sql() method and integrated db_default handling in column_sql(), add_field(), _alter_column_null_sql(), and _set_field_new_type() |
| django_singlestore/features.py | Enabled supports_db_default feature flag, added test skips for unsupported scenarios, and corrected a typo |
| README.md | Documented limitation regarding complex expressions in DEFAULT clauses |
| scripts/setup_sections/backends_setup.sql | Added a new M2M table definition for test purposes |
| .github/workflows/tests.yml | Updated Django ref to 5.0.x and Python version to 3.11 |
| .github/workflows/test_all_versions.yml | Updated Django ref to 5.0.x and Python version to 3.11 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
README.md
Outdated
| - `dumpdata` django command does not work if a table or a referenced m2m table does not have `id` column, which is the case for m2m tables created as suggested above (see `queries_paragraph_page` table definition). | ||
| - Fetching an instance of a custom through model using .objects.get() is not supported. | ||
| - Case-insensitive filter requires casting the filtered column to a `TextField` with case-insensitive collation, e.g. `utf8mb4_general_ci`. | ||
| - These complex expressions in `DEFAULT` clauses is not supported. SingleStore typically supports simple literal defaults (strings, numbers) and specific time functions like NOW() or CURRENT_TIMESTAMP() for datetime columns. |
There was a problem hiding this comment.
Grammatical error: 'These complex expressions' should be followed by 'are' not 'is'. Change 'is not supported' to 'are not supported'.
| - These complex expressions in `DEFAULT` clauses is not supported. SingleStore typically supports simple literal defaults (strings, numbers) and specific time functions like NOW() or CURRENT_TIMESTAMP() for datetime columns. | |
| - These complex expressions in `DEFAULT` clauses are not supported. SingleStore typically supports simple literal defaults (strings, numbers) and specific time functions like NOW() or CURRENT_TIMESTAMP() for datetime columns. |
psharma-1909
force-pushed
the
psharma/django_5.0_fix
branch
from
2621958 to
537ee56
Compare
psharma-1909
changed the base branch from
singlestore-django-5.0.x
to
stable/singlestore-django-5.0.x
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
django_singlestore/schema.py:145
- The
column_sqlmethod returns an empty list for params on line 145, but whendb_defaultis handled (lines 110-114), params are accumulated in theparamslist. The return statement should bereturn ' '.join(result_sql_parts), paramsto properly return the accumulated params.
return " ".join(result_sql_parts), []
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def prepare_default(self, value): | ||
| return self.quote_value(value) | ||
|
|
||
| def db_default_sql(self, field): |
There was a problem hiding this comment.
does this method have any differences compared to the same method defined in BaseDatabaseSchemaEditor? If yes, please document them; if no, we can simply use the one defined in the parent class.
There was a problem hiding this comment.
Yes, there was a small but important difference, and that’s why I didn’t use the method from BaseDatabaseSchemaEditor.
The difference in line 77.
self._column_default_sql(field) if isinstance(db_default, Value) else "(%s)"
This extra set of parentheses in the fallback (%s) caused a SQL error in our backend. Because of that, I had to override the method and adjust the SQL fragment to match the syntax expected by SingleStore.
| [], | ||
| ) | ||
|
|
||
| def _set_field_new_type(self, field, new_type): |
There was a problem hiding this comment.
same comment as db_default_sql.
There was a problem hiding this comment.
Similarly there was issue with sql generated from _alter_column_null_sql method when field is db_default .
Changing a column from NULL to NOT NULL (and vice versa) is not supported on columnstore tables. but
Workaround for changing NULL/NOT NULL:
For rowstore tables, we can use the MODIFY clause . (The change i did)
For columnstore tables, since modifying the data type or NULL constraint isn't supported directly, we would need to:
- Add a new column with the desired NULL constraint
- Copy the data from the old column
- Drop the old column
- Rename the new column
README.md
Outdated
| - `dumpdata` django command does not work if a table or a referenced m2m table does not have `id` column, which is the case for m2m tables created as suggested above (see `queries_paragraph_page` table definition). | ||
| - Fetching an instance of a custom through model using .objects.get() is not supported. | ||
| - Case-insensitive filter requires casting the filtered column to a `TextField` with case-insensitive collation, e.g. `utf8mb4_general_ci`. | ||
| - These complex expressions in `DEFAULT` clauses are not supported. SingleStore typically supports simple literal defaults (strings, numbers) and specific time functions like NOW() or CURRENT_TIMESTAMP() for datetime columns. |
There was a problem hiding this comment.
What is meant by "these"? I'd say something like "Some complex expressions (e.g. , ) in DEFAULT ..." to give some examples
We will keep the django singlestore backend for version 5.0 in branch as we can't use common skip_list and expected_failure in features.py for all version .