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

Add postgreSql interval column result #4152

Merged

Conversation

griffio
Copy link
Contributor

@griffio griffio commented May 10, 2023

closes #4144 Look into approach for supporting INTERVAL literal column results

SELECT INTERVAL '1 day';

Override Literals, test returns PGInterval type.

TODOs

Literals for CURRENT_DATE, CURRENT_TIME, CURRENT_DATE_TIME should also be considered.

Support SELECT NOW() + INTERVAL '1 day';

@griffio griffio marked this pull request as draft May 10, 2023 16:25
Copy link
Collaborator

@AlecKazakova AlecKazakova left a comment

Choose a reason for hiding this comment

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

very nice

if (PsiTreeUtil.getParentOfType(this, SqlSetterExpression::class.java) != null) return
if (PsiTreeUtil.getParentOfType(this, SqlCreateTableStmt::class.java) != null) return

annotationHolder.createErrorAnnotation(this, "Cannot use time literal in expression")
Copy link
Collaborator

Choose a reason for hiding this comment

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

test case for this

Copy link
Collaborator

Choose a reason for hiding this comment

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

you can do error tests in the fixtures

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean specifically on this error annotation. They're super easy to set up a test for, it's like this where a test fixture has a comment specifically for a reported error

Copy link
Contributor Author

@griffio griffio May 13, 2023

Choose a reason for hiding this comment

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

Thanks - specifically I was interested in the inherited LiteralValueMixin that has the restriction "Cannot use time literal in expression"?

As both PostgreSql + ansiFixtures are run by the tests - I can't override it in the PostgreSql dialect to support the syntax
updatedAt > CURRENT_TIMESTAMP - INTERVAL '30' days - The timestamp-literal will always run and currently fails if I override the restriction in LiteralValueMixin.

I looked at the Fixture setup and I don't think it will override the ansi test cases even if I redeclare the test fixture timestamp-literals in postgreSql fixtures.

Hopefully - that makes sense and is for another PR once I understand it :)

Bind arg test is easiest to debug
Add Integration test for PGInterval type
Override literal_value to support interval_expression
“INTERVAL ‘1 day” is the literal expression that will be used by the TypeResolver

This creates a new PostgresLiteralValue that inherits the base LiteralValueMixin

LiteralValueMixin may have to be customised to support returning CURRENT_DATE_TIME etc.
The base behaviour only allows assignments of timestamps.
The SqlLiteralExpr will be “INTERVAL ‘1 day’”

Add CURRENT_  types to return actual types -ansi  inherited was TEXT

Calls the inherited “ansi” behaviour for NULL etc

There is probably a better way to achieve this in future PRs as there is more to do
@griffio griffio force-pushed the add-postgreSql-Interval-column-result branch from 0a6ba07 to a855d64 Compare May 12, 2023 09:47
@griffio griffio marked this pull request as ready for review May 12, 2023 10:27
Used for debug
Copy link
Collaborator

@hfhbd hfhbd left a comment

Choose a reason for hiding this comment

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

Thanks, sorry for the missing answer in the issue, but this is exactly what I mean.

@AlecKazakova AlecKazakova merged commit 63f6de8 into cashapp:master May 25, 2023
@griffio griffio deleted the add-postgreSql-Interval-column-result branch May 25, 2023 19:14
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.

PostgreSQL: Support Interval expression
3 participants