Postgres "ALTER TABLE" enhancement, and timestamp bug fix #1338

WittierDinosaur · 2021-08-31T19:30:36Z

The initial purpose of this PR was to enhance the functionality of ALTER TABLE is Postgres, to match https://www.postgresql.org/docs/13/sql-altertable.html

However, the PR also includes a bugfix, for the timestamp with time zone issue, as this was getting in the way of my testing. It also slightly expands the definition of Expression D in ANSI, to allow the following kind of expression:
timestamp 'epoch' + foo_timestamp * interval '1 second'

Fixes #1289, Fixes #1286, Fixes #1168, Fixes #1155

Are there any other side effects of this change that we should be aware of?

Expands ANSI definitions of Expression_D, and timestamp with time zone (this is ANSI standard though). All current ANSI tests pass.

Pull Request checklist

I have included a lot of test cases for alter table - though more may be required.
The only extra functionality needed is sequences, for which I haven't yet raised a ticket

…me zone

barrywhart

The code looks good. I'm seeing some linter failures. Can you address these? I think it'll be ready to merge at that point, assuming the build passes.

./src/sqlfluff/dialects/dialect_ansi.py:519:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:361:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:440:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:440:1: D415 First line should end with a period, question mark, or exclamation point
./src/sqlfluff/dialects/dialect_postgres.py:625:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:683:1: D200 One-line docstring should fit on one line with quotes
./src/sqlfluff/dialects/dialect_postgres.py:683:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:683:1: D415 First line should end with a period, question mark, or exclamation point
./src/sqlfluff/dialects/dialect_postgres.py:796:1: D200 One-line docstring should fit on one line with quotes
./src/sqlfluff/dialects/dialect_postgres.py:796:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:796:1: D415 First line should end with a period, question mark, or exclamation point
./src/sqlfluff/dialects/dialect_postgres.py:821:1: D200 One-line docstring should fit on one line with quotes
./src/sqlfluff/dialects/dialect_postgres.py:821:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:821:1: D415 First line should end with a period, question mark, or exclamation point
./src/sqlfluff/dialects/dialect_postgres.py:849:1: D200 One-line docstring should fit on one line with quotes
./src/sqlfluff/dialects/dialect_postgres.py:849:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:849:1: D415 First line should end with a period, question mark, or exclamation point
./src/sqlfluff/dialects/dialect_postgres.py:866:1: D200 One-line docstring should fit on one line with quotes
./src/sqlfluff/dialects/dialect_postgres.py:866:1: D212 Multi-line docstring summary should start at the first line
./src/sqlfluff/dialects/dialect_postgres.py:866:1: D415 First line should end with a period, question mark, or exclamation point

…nition option in Expression_D

codecov · 2021-08-31T23:18:44Z

Codecov Report

Merging #1338 (dbfe612) into main (c7620f4) will increase coverage by 0.03%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1338      +/-   ##
==========================================
+ Coverage   97.07%   97.10%   +0.03%     
==========================================
  Files         124      124              
  Lines        8541     8573      +32     
==========================================
+ Hits         8291     8325      +34     
+ Misses        250      248       -2

Flag	Coverage Δ
dbt018-py38	`61.35% <100.00%> (+0.16%)`	⬆️
dbt019-py38	`61.35% <100.00%> (+0.16%)`	⬆️
dbt020-py38	`61.32% <100.00%> (+0.16%)`	⬆️
py36	`95.85% <100.00%> (+0.01%)`	⬆️
py37	`95.88% <100.00%> (+0.01%)`	⬆️
py38	`95.92% <100.00%> (+0.03%)`	⬆️
py39	`95.89% <100.00%> (+0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/sqlfluff/dialects/dialect_ansi.py	`100.00% <100.00%> (ø)`
src/sqlfluff/dialects/dialect_postgres.py	`100.00% <100.00%> (ø)`
src/sqlfluff/core/linter/runner.py	`94.68% <0.00%> (+2.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c7620f4...dbfe612. Read the comment docs.

WittierDinosaur · 2021-09-01T08:42:46Z

Linter failures fixed, build passing

tunetheweb · 2021-09-01T12:26:10Z

@WittierDinosaur can you checkout what's going on with the formatting changes to benchmarks/bench_002/bench_002_pearson_fix.sql - was this intentional?

WittierDinosaur · 2021-09-01T12:47:57Z

@tunetheweb No, I'm not sure why that is happening. But I don't think it's been introduced by my code. The output is weird, but it passes the tox -e bench test. I think this issue was introduced somewhere else. I just ran tox before I committed, so the file was changed. I'm getting the same behaviour if I run tox -e bench when I checkout main. Can you verify it's happening on the main branch for you?

tunetheweb · 2021-09-01T13:07:19Z

No I'm not getting it. Do you have a .sqlfluff file in your repo with different settings to the default?

tunetheweb

Looks great. Though as discussed can you try and figure out the benchmark change? Maybe a rogue .sqlfluff file?

test/fixtures/parser/ansi/select_true_and_not_false.yml

test/fixtures/parser/postgres/postgres_select.sql

test/fixtures/parser/postgres/postgres_alter_table.sql

WittierDinosaur · 2021-09-01T15:46:56Z

@tunetheweb I've just cloned sqlfluff's main fresh, pip installed tox, and the bench output is doing the same thing. I have no idea how to explain it. Surely nothing is cached outside of the project folder? Maybe an OS thing? I don't know.

tunetheweb · 2021-09-01T16:16:03Z

@tunetheweb I've just cloned sqlfluff's main fresh, pip installed tox, and the bench output is doing the same thing. I have no idea how to explain it. Surely nothing is cached outside of the project folder? Maybe an OS thing? I don't know.

Weird! For now do you just wanna revert the changes in your branch and try your best not to check them in?

WittierDinosaur · 2021-09-01T16:21:34Z

Can do, worried about them masking an issue though. Are you able to checkout my branch and see if tox -e bench does this for my branch on your machine?

WittierDinosaur · 2021-09-01T16:34:42Z

Update - I got my flatmate to test on his machine. His steps were, git clone, pip install tox, tox -e bench, and the same thing happened on his machine

tunetheweb · 2021-09-01T16:41:25Z

Actually you're right it DOES do this for me. Sorry for misleading you.

I've just never run bench when using tox. I normally just do the below (from the CONTRIBUTING.md file):

tox -e generate-fixture-yml,cov-init,py36,cov-report,linting

So never noticed this before. So we might as well commit it to avoid the issue in future for anyone else that runs this!

WittierDinosaur · 2021-09-01T16:44:10Z

Thought I was going mad 😄 Okay, add in your edits, and check that not keyword then, commits inbound shortly. Also, we should raise this as a bug, i'm sure that's not the expected format

src/sqlfluff/dialects/dialect_ansi.py

tunetheweb

LGTM! Thanks for this - and for working through the feedback.

@barrywhart you any further comments or happy to merge?

barrywhart

Looks good to me! My only concern was the code linter stuff, and that's been addressed. Thanks!

Olaktal · 2021-09-13T17:41:10Z

Hello guys,
Sounds like it didn't solve my problem :(
Any idea ?

WittierDinosaur and others added 3 commits August 31, 2021 20:03

Enhanced postgres support for alter table, bugfixed timestamp with ti…

1b196d1

…me zone

Moved timestamp with time zone logic to ansi, added test cases

b6943e2

Merge branch 'main' into postgres-alter-table-enhancement

4fbdc70

barrywhart requested changes Aug 31, 2021

View reviewed changes

WittierDinosaur added 5 commits August 31, 2021 23:01

Postgres select changed

0700792

Linting changes

df09ce1

Linting changes

7a63732

Bugfix, removed QualifiedNUmericLiteralSegment from new function defi…

901e911

…nition option in Expression_D

Blacked file

1c6f690

Merge branch 'main' into postgres-alter-table-enhancement

da5dc00

tunetheweb requested changes Sep 1, 2021

View reviewed changes

test/fixtures/parser/ansi/select_true_and_not_false.yml Outdated Show resolved Hide resolved

test/fixtures/parser/postgres/postgres_select.sql Outdated Show resolved Hide resolved

test/fixtures/parser/postgres/postgres_alter_table.sql Outdated Show resolved Hide resolved

WittierDinosaur added 2 commits September 1, 2021 17:45

Style changes

9c32eb1

Deleted files added for debugging

06308a0

tunetheweb reviewed Sep 1, 2021

View reviewed changes

src/sqlfluff/dialects/dialect_ansi.py Show resolved Hide resolved

Added NOT as a DataTypeIdentifier antipattern

dbfe612

tunetheweb approved these changes Sep 1, 2021

View reviewed changes

barrywhart approved these changes Sep 1, 2021

View reviewed changes

tunetheweb merged commit ab11418 into sqlfluff:main Sep 1, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Postgres "ALTER TABLE" enhancement, and timestamp bug fix #1338

Postgres "ALTER TABLE" enhancement, and timestamp bug fix #1338

WittierDinosaur commented Aug 31, 2021 •

edited

Loading

barrywhart left a comment

codecov bot commented Aug 31, 2021 •

edited

Loading

WittierDinosaur commented Sep 1, 2021

tunetheweb commented Sep 1, 2021

WittierDinosaur commented Sep 1, 2021

tunetheweb commented Sep 1, 2021

tunetheweb left a comment

WittierDinosaur commented Sep 1, 2021 •

edited

Loading

tunetheweb commented Sep 1, 2021

WittierDinosaur commented Sep 1, 2021

WittierDinosaur commented Sep 1, 2021

tunetheweb commented Sep 1, 2021 •

edited

Loading

WittierDinosaur commented Sep 1, 2021

tunetheweb left a comment

barrywhart left a comment

Olaktal commented Sep 13, 2021

Postgres "ALTER TABLE" enhancement, and timestamp bug fix #1338

Postgres "ALTER TABLE" enhancement, and timestamp bug fix #1338

Conversation

WittierDinosaur commented Aug 31, 2021 • edited Loading

Are there any other side effects of this change that we should be aware of?

Pull Request checklist

barrywhart left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 31, 2021 • edited Loading

Codecov Report

WittierDinosaur commented Sep 1, 2021

tunetheweb commented Sep 1, 2021

WittierDinosaur commented Sep 1, 2021

tunetheweb commented Sep 1, 2021

tunetheweb left a comment

Choose a reason for hiding this comment

WittierDinosaur commented Sep 1, 2021 • edited Loading

tunetheweb commented Sep 1, 2021

WittierDinosaur commented Sep 1, 2021

WittierDinosaur commented Sep 1, 2021

tunetheweb commented Sep 1, 2021 • edited Loading

WittierDinosaur commented Sep 1, 2021

tunetheweb left a comment

Choose a reason for hiding this comment

barrywhart left a comment

Choose a reason for hiding this comment

Olaktal commented Sep 13, 2021

WittierDinosaur commented Aug 31, 2021 •

edited

Loading

codecov bot commented Aug 31, 2021 •

edited

Loading

WittierDinosaur commented Sep 1, 2021 •

edited

Loading

tunetheweb commented Sep 1, 2021 •

edited

Loading