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 support for WITH SESSION clause #23474

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

ebyhr
Copy link
Member

@ebyhr ebyhr commented Sep 18, 2024

Description

Add support for WITH SESSION clause:

WITH SESSION
    session_key = 'property',
    catalog.catalog_session_key = 'catalog_property'
SELECT 1

The syntax with both sessions and SQL functions:

WITH 
  SESSION a = 1
  FUNCTION x(a BIGINT) RETURNS BIGINT RETURN 1
SELECT x()

WITH SESSION with DML is unsupported in this PR.

Supersedes #21303

Release notes

# General
* Add support for `WITH SESSION` clause. ({issue}`23474`)

@cla-bot cla-bot bot added the cla-signed label Sep 18, 2024
@github-actions github-actions bot added the docs label Sep 18, 2024
validateSystemProperties(session, resolvedSessionSpecifications.systemProperties());

// Catalog session properties were already evaluated so we need to evaluate overrides
if (session.getTransactionId().isPresent()) {
Copy link
Member

Choose a reason for hiding this comment

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

did you consider moving validation logic to Session class where it already is? You can put it in Session.withProperties maybe?

Copy link
Member Author

Choose a reason for hiding this comment

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

It requires some changes for injecting AccessControl to Session. Added a TODO comment.

Object objectValue;

try {
objectValue = evaluatePropertyValue(specification.getValue(), type, session, plannerContext, accessControl, parameters);
Copy link
Member

Choose a reason for hiding this comment

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

Consider extracting common code from here and SetSessionTask

Copy link
Member Author

Choose a reason for hiding this comment

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

The logic is a little different from SetSessionTask. Added a TODO comment.

Copy link
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

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

Looks nice. Some comments

Copy link
Member

@martint martint left a comment

Choose a reason for hiding this comment

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

The syntax generally looks good, but there's a point we should discuss.

When inline functions are present, the full query will look like this:

WITH 
  FUNCTION x(a BIGINT) RETURNS BIGINT RETURN 1
  SESSION a = 1
SELECT * FROM t  

The session properties are supposed to apply to everything, including function definitions. It would be clearer and less surprising to users if it were structured like this, instead:

WITH 
  SESSION a = 1
  FUNCTION x(a BIGINT) RETURNS BIGINT RETURN 1
SELECT * FROM t  

@@ -4,6 +4,7 @@

```text
[ WITH FUNCTION sql_routines ]
[ WITH SESSION name = expression [, ...] ]
Copy link
Member

Choose a reason for hiding this comment

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

This is incorrect per the proposed grammar. The combined form when inline functions are present would be:

WITH 
  FUNCTION x(a BIGINT) RETURNS BIGINT RETURN 1
  SESSION a = 1
SELECT * FROM t  

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to [ WITH [ SESSION name = expression [, ...] ] [ FUNCTION sql_routines ] ]

Copy link
Member

Choose a reason for hiding this comment

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

Did you forget to push a change? I don't see it reflected in the docs above.

Copy link
Member Author

@ebyhr ebyhr Sep 20, 2024

Choose a reason for hiding this comment

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

No, it was updated in a fixup commit. Let me squash commits.

@findepi
Copy link
Member

findepi commented Sep 19, 2024

The syntax with both sessions and SQL functions:

WITH 
  SESSION a = 1
  FUNCTION x(a BIGINT) RETURNS BIGINT RETURN 1
SELECT x()

How do WITH SESSION, WITH FUNCTION, and WITH cte look like together?

@ebyhr
Copy link
Member Author

ebyhr commented Sep 19, 2024

We can repeat WITH in such cases:

 WITH
    SESSION time_zone_id = 'Europe/Warsaw'
    FUNCTION foo() RETURNS varchar RETURN current_timezone()
 WITH RECURSIVE t(n, x) AS (
     VALUES (1, foo())
     UNION ALL
     SELECT n + 1, foo() FROM t WHERE n < 4
 )
 SELECT * FROM t;

@ebyhr
Copy link
Member Author

ebyhr commented Sep 20, 2024

Addressed comments.

@@ -650,6 +656,15 @@ protected Void visitQuery(Query node, Integer indent)
}
builder.append('\n');
}

Iterator<SessionSpecification> sessionProperties = node.getSessionProperties().iterator();
Copy link
Member

Choose a reason for hiding this comment

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

This should go above functions. Are we missing some tests? Otherwise, this should fail.

Copy link
Member Author

@ebyhr ebyhr Sep 20, 2024

Choose a reason for hiding this comment

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

It was moved in a fixup commit. Squashed commits.

@@ -4,6 +4,7 @@

```text
[ WITH FUNCTION sql_routines ]
[ WITH SESSION name = expression [, ...] ]
Copy link
Member

Choose a reason for hiding this comment

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

Did you forget to push a change? I don't see it reflected in the docs above.

ebyhr and others added 2 commits September 21, 2024 06:47
Co-Authored-By: Mateusz "Serafin" Gajewski <[email protected]>
Co-Authored-By: Mateusz "Serafin" Gajewski <[email protected]>
@ebyhr ebyhr requested review from martint and removed request for raunaqmorarka September 20, 2024 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants