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: Allow to turn off column store #555

Merged
merged 5 commits into from
Jun 8, 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
3 changes: 3 additions & 0 deletions CHANGES.txt
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@ Changes for crate
Unreleased
==========

- SQLAlchemy DDL: Allow turning off column store using ``crate_columnstore=False``.
Thanks, @fetzerms.


2023/04/18 0.31.1
=================
Expand Down
2 changes: 2 additions & 0 deletions docs/sqlalchemy.rst
Original file line number Diff line number Diff line change
Expand Up @@ -205,6 +205,7 @@ system <sa:orm_declarative_mapping>`:
... more_details = sa.Column(types.ObjectArray)
... name_ft = sa.Column(sa.String)
... quote_ft = sa.Column(sa.String)
... even_more_details = sa.Column(sa.String, crate_columnstore=False)
...
... __mapper_args__ = {
... 'exclude_properties': ['name_ft', 'quote_ft']
Expand All @@ -220,6 +221,7 @@ 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
Expand Down
9 changes: 9 additions & 0 deletions src/crate/client/sqlalchemy/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import sqlalchemy as sa
from sqlalchemy.dialects.postgresql.base import PGCompiler
from sqlalchemy.sql import compiler
from sqlalchemy.types import String
from .types import MutableDict, _Craty, Geopoint, Geoshape
from .sa_version import SA_VERSION, SA_1_4

Expand Down Expand Up @@ -128,6 +129,14 @@ def get_column_specification(self, column, **kwargs):

colspec += " INDEX OFF"

if column.dialect_options['crate'].get('columnstore') is False:
if not isinstance(column.type, (String, )):
raise sa.exc.CompileError(
"Controlling the columnstore is only allowed for STRING columns"
)

colspec += " STORAGE WITH (columnstore = false)"
Comment on lines +132 to +138
Copy link
Member

Choose a reason for hiding this comment

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

If I got it right, columnstore can only be turned off for TEXT columns, hence I added this as a condition to the code.

I think it will be fine. Thank you for adding this constraint. Do you acknowledge this, @seut or @matriv?

Copy link
Member

Choose a reason for hiding this comment

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

With crate/crate@837db42, it is also supported on numeric and timestamp types but this is not yet released, master/nightly only (will be part of CrateDB 5.4. So yes fine like that for now.


return colspec

def visit_computed_column(self, generated):
Expand Down
27 changes: 27 additions & 0 deletions src/crate/client/sqlalchemy/tests/create_table_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -232,3 +232,30 @@ class DummyTable(self.Base):
a = sa.Column(Geopoint, crate_index=False)
with self.assertRaises(sa.exc.CompileError):
self.Base.metadata.create_all(bind=self.engine)

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

Check notice

Code scanning / CodeQL

Unused local variable

Variable DummyTable is not used.
Copy link
Member

@amotl amotl May 30, 2023

Choose a reason for hiding this comment

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

We know this admonition by CodeQL on those occasions. They can be dismissed as false positive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah... I was already wondering if there is a way to make CodeQL ignore it. I'm happy with them being dismissed tho.

Copy link
Member

@amotl amotl May 30, 2023

Choose a reason for hiding this comment

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

Yes, dismissing them is the maximum we can do here. But unfortunately, the corresponding admonition items will never be collapsed. Instead, they will be displayed "expanded" here, into eternity, even if we would resolve this conversation about it.

Unfortunately, there is also no other way to mitigate warnings on such spots, or to acknowledge them upfront, for example, by placing corresponding annotations into the code.

Copy link
Member

Choose a reason for hiding this comment

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

Hi again. CodeQL admonition items will be made collapsible, it's so sweet. Thank you, @anaarmas.

-- github/codeql-action#1411 (comment)

__tablename__ = 't'
pk = sa.Column(sa.String, primary_key=True)
a = sa.Column(sa.String, crate_columnstore=False)
b = sa.Column(sa.String, crate_columnstore=True)
c = sa.Column(sa.String)

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 STORAGE WITH (columnstore = false), \n\t'
'b STRING, \n\t'
'c STRING, \n\t'
'PRIMARY KEY (pk)\n)\n\n'), ())

def test_non_text_column_without_columnstore(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)
a = sa.Column(sa.Integer, crate_columnstore=False)

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