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

Conversation

fetzerms
Copy link
Contributor

@fetzerms fetzerms commented May 15, 2023

Summary of the changes / Why this is an improvement

This allows to disable column store for a text column using:

my_data: str = Column(String, crate_index=False, crate_columnstore=False)

Which will yield:

 "my_data" TEXT INDEX OFF STORAGE WITH ( columnstore = false )

Checklist

  • CLA is signed

@amotl
Copy link
Member

amotl commented May 22, 2023

Dear Matthias,

thanks a stack for submitting this improvement to the DDL compiler. In order to make the patch complete, we will need to add software tests and documentation. Would you be up to add them, or do you want us to take over?

With kind regards,
Andreas.

@amotl amotl changed the title Allow to turn off column store SQLAlchemy/DDL: Allow to turn off column store May 22, 2023
@fetzerms
Copy link
Contributor Author

@amotl Thank you very much. I will add some tests and documentation - just not sure when, as I'm quite occupied at work lately. But as the changes are very minor, I think I will soon push some changes.

@fetzerms
Copy link
Contributor Author

@amotl I have added the tests and some (little) docs. If I got it right, columnstore can only be turned off for TEXT columns, hence I added this as a condition to the code. In case this needs some rework, please let me know.

Copy link
Member

@amotl amotl left a comment

Choose a reason for hiding this comment

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

Dear Matthias,

your patch looks good to me. Thank you very much for adding corresponding software tests.

With kind regards,
Andreas.

Comment on lines +132 to +138
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)"
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.

@amotl amotl requested review from hammerhead and matriv May 30, 2023 14:29
@@ -232,3 +232,30 @@
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)

'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.
@amotl amotl merged commit 7b86801 into crate:master Jun 8, 2023
@amotl
Copy link
Member

amotl commented Jun 8, 2023

Dear Matthias,

after adding a changelog item to your patch, I've just merged it. Thank you again.

With kind regards,
Andreas.

@fetzerms fetzerms deleted the fetzerms-sa_column_store branch June 10, 2023 19:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants