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

SQLAlchemy/DDL: Handle server_default column schema argument well #556

Merged
merged 1 commit into from
Jun 9, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ Unreleased
SQLAlchemy 2.0 by adding the new ``insert_returning`` and ``update_returning`` flags
in the CrateDB dialect.

- SQLAlchemy DDL: Allow setting ``server_default`` on columns to enable server-generated defaults.

2023/03/30 0.31.0
=================
Expand Down
4 changes: 3 additions & 1 deletion docs/sqlalchemy.rst
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,7 @@ system <sa:orm_declarative_mapping>`:
... name_ft = sa.Column(sa.String)
... quote_ft = sa.Column(sa.String)
... even_more_details = sa.Column(sa.String, crate_columnstore=False)
... created_at = sa.Column(sa.DateTime, server_default=sa.func.now())
...
... __mapper_args__ = {
... 'exclude_properties': ['name_ft', 'quote_ft']
Expand All @@ -221,13 +222,14 @@ In this example, we:
- Use standard SQLAlchemy types for the ``id``, ``name``, and ``quote`` columns
- Use ``nullable=False`` to define a ``NOT NULL`` constraint
- Disable indexing of the ``name`` column using ``crate_index=False``
- Disable the columnstore of the ``even_more_details`` column using ``crate_columnstore=False``
- Define a computed column ``name_normalized`` (based on ``name``) that
translates into a generated column
- Use the `Object`_ extension type for the ``details`` column
- Use the `ObjectArray`_ extension type for the ``more_details`` column
- Set up the ``name_ft`` and ``quote_ft`` fulltext indexes, but exclude them from
the mapping (so SQLAlchemy doesn't try to update them as if they were columns)
- Disable the columnstore of the ``even_more_details`` column using ``crate_columnstore=False``
- Add a ``created_at`` column whose default value is set by CrateDB's ``now()`` function.

.. TIP::

Expand Down
5 changes: 4 additions & 1 deletion src/crate/client/sqlalchemy/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,10 @@ class CrateDDLCompiler(compiler.DDLCompiler):
def get_column_specification(self, column, **kwargs):
colspec = self.preparer.format_column(column) + " " + \
self.dialect.type_compiler.process(column.type)
# TODO: once supported add default here

default = self.get_column_default_string(column)
if default is not None:

Choose a reason for hiding this comment

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

Do we intentionally want to intentionally ignore empty values like [], "", {}. Why is the reason behind not using?

Suggested change
if default is not None:
if default:

Trying to understand, thanks! 😁

Copy link
Member

Choose a reason for hiding this comment

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

@sanchezfdezjavier
Not sure if I correctly understand your questions, any value beside None will evaluate to True.

'' is not None => True
[] is not None => True
{} is not None => True
False is not None => True

While using if default: it would evaluate to False for all the above examples, see also https://docs.python.org/3/library/stdtypes.html#truth.

Copy link
Member

Choose a reason for hiding this comment

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

Hi Javi,

get_column_default_string() probably returns a string, so in the case of non-empty strings, your suggestion might also work in this case.

But a) @seut is generally right, and b) explicitly testing against is not None also feels safer for me, because it does not suffer from any implicit bool-casting flaws.

With kind regards,
Andreas.

Choose a reason for hiding this comment

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

Awesome, that makes sense. Thanks for clarifying!

colspec += " DEFAULT " + default

if column.computed is not None:
colspec += " " + self.process(column.computed)
Expand Down
52 changes: 52 additions & 0 deletions src/crate/client/sqlalchemy/tests/create_table_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -259,3 +259,55 @@ class DummyTable(self.Base):

with self.assertRaises(sa.exc.CompileError):
self.Base.metadata.create_all(bind=self.engine)

def test_column_server_default_text_func(self):
class DummyTable(self.Base):
JanLikar marked this conversation as resolved.
Show resolved Hide resolved
__tablename__ = 't'
pk = sa.Column(sa.String, primary_key=True)
a = sa.Column(sa.DateTime, server_default=sa.text("now()"))

self.Base.metadata.create_all(bind=self.engine)
fake_cursor.execute.assert_called_with(
('\nCREATE TABLE t (\n\t'
'pk STRING NOT NULL, \n\t'
'a TIMESTAMP DEFAULT now(), \n\t'
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_column_server_default_string(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
__tablename__ = 't'
pk = sa.Column(sa.String, primary_key=True)
a = sa.Column(sa.String, server_default="Zaphod")

self.Base.metadata.create_all(bind=self.engine)
fake_cursor.execute.assert_called_with(
('\nCREATE TABLE t (\n\t'
'pk STRING NOT NULL, \n\t'
'a STRING DEFAULT \'Zaphod\', \n\t'
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_column_server_default_func(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
__tablename__ = 't'
pk = sa.Column(sa.String, primary_key=True)
a = sa.Column(sa.DateTime, server_default=sa.func.now())

self.Base.metadata.create_all(bind=self.engine)
fake_cursor.execute.assert_called_with(
('\nCREATE TABLE t (\n\t'
'pk STRING NOT NULL, \n\t'
'a TIMESTAMP DEFAULT now(), \n\t'
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_column_server_default_text_constant(self):
class DummyTable(self.Base):

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
__tablename__ = 't'
pk = sa.Column(sa.String, primary_key=True)
answer = sa.Column(sa.Integer, server_default=sa.text("42"))

self.Base.metadata.create_all(bind=self.engine)
fake_cursor.execute.assert_called_with(
('\nCREATE TABLE t (\n\t'
'pk STRING NOT NULL, \n\t'
'answer INT DEFAULT 42, \n\t'
'PRIMARY KEY (pk)\n)\n\n'), ())