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

JanLikar · 2023-05-26T22:14:04Z

Summary of the changes / Why this is an improvement

CrateDB's SQLAlchemy dialect now handles the server_default when generating table DDL.

Checklist

CLA is signed

amotl

Dear Jan,

thanks a stack for submitting this patch. I like that the test methods enumerate different kinds of values / value types the server_default argument is able to obtain.

Other than a suggestion about reStructuredText syntax, and maybe a slight naming change on the test methods, I think it will be good to go.

With kind regards,
Andreas.

docs/sqlalchemy.rst

src/crate/client/sqlalchemy/tests/create_table_test.py

+             'PRIMARY KEY (pk)\n)\n\n'), ())
+
+    def test_column_server_default_string(self):
+        class DummyTable(self.Base):


src/crate/client/sqlalchemy/tests/create_table_test.py

+             'PRIMARY KEY (pk)\n)\n\n'), ())
+
+    def test_column_server_default_func(self):
+        class DummyTable(self.Base):


src/crate/client/sqlalchemy/tests/create_table_test.py

+             'PRIMARY KEY (pk)\n)\n\n'), ())
+
+    def test_column_server_default_constant(self):
+        class DummyTable(self.Base):


JanLikar · 2023-05-31T10:57:24Z

Other than a suggestion about reStructuredText syntax, and maybe a slight naming change on the test methods, I think it will be good to go.

I applied the fixes and squashed the commits. To be honest, I've been using Markdown so much lately I didn't even realize this is rst, thanks for spotting the issue.

seut

Look good! I've just added a type related suggestion related to a test case.

seut · 2023-06-05T13:16:53Z

src/crate/client/sqlalchemy/tests/create_table_test.py

+        fake_cursor.execute.assert_called_with(
+            ('\nCREATE TABLE t (\n\t'
+             'pk STRING NOT NULL, \n\t'
+             'a TIMESTAMP DEFAULT \'Zaphod\', \n\t'


This is a non-working example as CrateDB will complain here that the Zaphod string literal cannot be converted to timestamp. Can we use a different example here? E.g. a TEXT DEFAULT 'CrateDB rocks' or something like that.

Nice catch. Fixed.

Hi again. After invoking CI, it looks like the test case test_column_server_default_string needs corresponding adjustments to compensate this fix.

Hi, sorry about that, I would have run the tests before pushing, but I was traveling without access to a dev environment.

I'm home now and the tests pass for me, so I think we're good to go.

Dear Jan,

no worries about that hiccup, and welcome back. Now that I've just merged GH-555, it is my turn to apologize for the inconvenience that you may need to rebase your patch now, and most probably also resolve some conflicts around the same spots where @fetzerms made his changes. Please take your time.

If you could also add a changelog item like 7b86801, the patch will be perfect and complete. Thank you so much!

With kind regards,
Andreas.

P.S.: Don't worry about failing GHA tests. Right now, they are only failing because coverage stats can't be uploaded to Codecov on behalf of your account's permissions, because it would need access to this repository's tokens. While it is a public repository which usually does not need any authentication tokens, we established them the other day after Codecov tripped us very often. It is a known issue that it sometimes blocks access from GitHub, and it happens more often on larger test matrix scenarios, where many requests are being submitted from GitHub to Codecov, like this one. Now you know the whole story ;].

Andreas, thank you for your kind words! I rebased and pushed my changes, luckily the conflicts were relatively mild.

sanchezfdezjavier · 2023-06-07T19:48:46Z

src/crate/client/sqlalchemy/compiler.py

-        # TODO: once supported add default here
+
+        default = self.get_column_default_string(column)
+        if default is not None:


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! 😁

@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.

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.

Awesome, that makes sense. Thanks for clarifying!

CrateDB's SQLAlchemy dialect now handles the `server_default` when generating table DDL. Fix #454.

src/crate/client/sqlalchemy/tests/create_table_test.py

+             'PRIMARY KEY (pk)\n)\n\n'), ())
+
+    def test_column_server_default_string(self):
+        class DummyTable(self.Base):


src/crate/client/sqlalchemy/tests/create_table_test.py

+             'PRIMARY KEY (pk)\n)\n\n'), ())
+
+    def test_column_server_default_func(self):
+        class DummyTable(self.Base):


amotl · 2023-06-09T15:40:59Z

Merged. Thank you again, and have a good weekend.

amotl approved these changes May 30, 2023

View reviewed changes

docs/sqlalchemy.rst Outdated Show resolved Hide resolved

src/crate/client/sqlalchemy/tests/create_table_test.py Outdated Show resolved Hide resolved

src/crate/client/sqlalchemy/tests/create_table_test.py Outdated Show resolved Hide resolved

amotl requested a review from seut May 30, 2023 14:37

amotl changed the title ~~Implement server_default for SA columns~~ SQLAlchemy/DDL: Handle server_default column schema argument well May 30, 2023

github-advanced-security bot found potential problems May 30, 2023

View reviewed changes

JanLikar force-pushed the column-server-default-454 branch from 9757321 to 6a32045 Compare May 31, 2023 10:54

seut reviewed Jun 5, 2023

View reviewed changes

JanLikar force-pushed the column-server-default-454 branch from 6a32045 to 1d9cf51 Compare June 5, 2023 14:07

JanLikar requested a review from seut June 5, 2023 14:08

seut approved these changes Jun 6, 2023

View reviewed changes

sanchezfdezjavier reviewed Jun 7, 2023

View reviewed changes

JanLikar force-pushed the column-server-default-454 branch 2 times, most recently from 6e098dd to 1b63dfa Compare June 8, 2023 23:21

Implement server_default for SA columns

e84aa11

CrateDB's SQLAlchemy dialect now handles the `server_default` when generating table DDL. Fix #454.

JanLikar force-pushed the column-server-default-454 branch from 1b63dfa to e84aa11 Compare June 8, 2023 23:22

github-advanced-security bot found potential problems Jun 9, 2023

View reviewed changes

amotl merged commit 4c62ecf into crate:master Jun 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

JanLikar commented May 26, 2023 •

edited by amotl

Loading

amotl left a comment •

edited

Loading

JanLikar commented May 31, 2023 •

edited by amotl

Loading

seut left a comment

seut Jun 5, 2023

JanLikar Jun 5, 2023

amotl Jun 8, 2023

JanLikar Jun 8, 2023

amotl Jun 8, 2023 •

edited

Loading

JanLikar Jun 8, 2023

sanchezfdezjavier Jun 7, 2023

seut Jun 8, 2023

amotl Jun 8, 2023

sanchezfdezjavier Jun 9, 2023

amotl commented Jun 9, 2023 •

edited

Loading

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

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

Conversation

JanLikar commented May 26, 2023 • edited by amotl Loading

Summary of the changes / Why this is an improvement

Checklist

amotl left a comment • edited Loading

Choose a reason for hiding this comment

JanLikar commented May 31, 2023 • edited by amotl Loading

seut left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amotl Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amotl commented Jun 9, 2023 • edited Loading

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

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

JanLikar commented May 26, 2023 •

edited by amotl

Loading

amotl left a comment •

edited

Loading

JanLikar commented May 31, 2023 •

edited by amotl

Loading

amotl Jun 8, 2023 •

edited

Loading

amotl commented Jun 9, 2023 •

edited

Loading