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 #24889

Merged
merged 4 commits into from
Feb 4, 2025

Conversation

wendigo
Copy link
Contributor

@wendigo wendigo commented Feb 3, 2025

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 #23474

Round 4 :)

@cla-bot cla-bot bot added the cla-signed label Feb 3, 2025
@wendigo wendigo requested review from martint and losipiuk February 3, 2025 12:41
@github-actions github-actions bot added the docs label Feb 3, 2025
@wendigo
Copy link
Contributor Author

wendigo commented Feb 3, 2025

@martint I don't know how to reuse QueryAssertions here. If you can guide me, I can rewrite tests to use it. Beside that, all of the review comments were addressed so I'd like to move this forward.

@wendigo wendigo force-pushed the serafin/ebi/with_session branch from 7eed9a9 to b8110ef Compare February 3, 2025 13:34
@github-actions github-actions bot added the hive Hive connector label Feb 3, 2025
@wendigo
Copy link
Contributor Author

wendigo commented Feb 3, 2025

@martint disregard last comment, I've managed to figure it out. I'll push it shortly.

@wendigo wendigo force-pushed the serafin/ebi/with_session branch 3 times, most recently from 56ec4c4 to 78f05fd Compare February 3, 2025 16:22
Copy link
Contributor

@chenjian2664 chenjian2664 left a comment

Choose a reason for hiding this comment

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

What's the expectation if client submit multiple statements? For example:

WITH 
  SESSION s1=v1, s2=v2
  SELECT 1;
WITH
  SESSION s1=update_v1
  SELECT 2;

@wendigo
Copy link
Contributor Author

wendigo commented Feb 4, 2025

@chenjian2664 the WITH SESSION applies to a single query (it's query scoped):

trino> WITH SESSION time_zone_id='Europe/Warsaw' SELECT current_timezone();
    -> WITH SESSION time_zone_id='UTC' SELECT current_timezone();
     _col0
---------------
 Europe/Warsaw
(1 row)

Query 20250204_052204_00008_uu8h3, FINISHED, 1 node
Splits: 1 total, 1 done (100.00%)
0.02 [0 rows, 0B] [0 rows/s, 0B/s]

 _col0
-------
 UTC
(1 row)

Query 20250204_052204_00009_uu8h3, FINISHED, 1 node
Splits: 1 total, 1 done (100.00%)
0.02 [0 rows, 0B] [0 rows/s, 0B/s]

@wendigo wendigo force-pushed the serafin/ebi/with_session branch from cb7d2b2 to 4afe3c8 Compare February 4, 2025 05:26
@wendigo wendigo force-pushed the serafin/ebi/with_session branch from 4afe3c8 to 536ce18 Compare February 4, 2025 10:08
@wendigo wendigo merged commit 05bf08b into trinodb:master Feb 4, 2025
100 checks passed
@github-actions github-actions bot added this to the 470 milestone Feb 4, 2025
@wendigo
Copy link
Contributor Author

wendigo commented Feb 4, 2025

@sajjoseph you can find this interesting :)

@sajjoseph
Copy link
Contributor

Absolutely! This will be really useful as we can embed the session variables in SQL itself. Thanks for this feature.
It will be an exciting addition as tools like Tableau doesn't allow end users to supply session parameters. Through the above PR, now end users can. Great job!

@wendigo
Copy link
Contributor Author

wendigo commented Feb 4, 2025

@sajjoseph yes, I was thinking about these use cases :)

@martint
Copy link
Member

martint commented Feb 4, 2025

I just realized this change makes the following syntax valid, even though it should fail:

trino> WITH WITH t as (SELECT 1) SELECT 1;
 _col0
-------
     1
(1 row)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

6 participants