Column-level lexically-scoped CTE expressions #10826

GregoryTravis · 2024-08-15T19:06:14Z

Pull Request Description

This implements DB_Column.with, which uses WITH ... AS SQL clauses to remove duplicates in the generated SQL.

After a discussion with @radeusgd, we concluded that we will probably want a more complete CTE implementation, so this one is useful for now to deal with big queries (like round).

Important Notes

Still to do in this PR:

Rename with to let (or something similar)
tests
documentation
remove State hack by moving query generation into a class and using a Ref field for scoping

Results on round_float:

---	SQL length in characters (unprettified)	SQL length in lines (prettified)
Without CTEs	13193	851
With CTEs	3644	187

Compare the SQL:

without-ctes.sql.txt
with-ctes.sql.txt

Update, with name shortening:

---	SQL length in characters (unprettified)	SQL length in lines (prettified)
Without CTEs	13193	853
With CTEs	2427	176

without-cte.txt
with-cte.txt

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

The documentation has been updated, if necessary.
Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
All code follows the
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.
Unit tests have been written where possible.

This reverts commit b7be792.

…ound_float

radeusgd · 2024-08-23T16:50:35Z

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Base_Generator.enso

+
+    rename self (name : Text) (binder : Text) =
+        binders = self.binders_ref.get
+        if binders.contains_key binder then binders.get binder else


nitpick as that's probably not a bottleneck, but what if we binders.get binder if_missing=Nothing? We could avoid accessing the hashmap twice 😅

radeusgd

Looks really really great. I really appreciate the comprehensive documentation.

We should remember to run Snowflake tests before integrating.

radeusgd · 2024-08-23T16:54:56Z

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Base_Generator.enso

+    new (existing_tables : Vector Text) -> Let_Binder_Renamer =
+        Let_Binder_Renamer.Value (Hashset.from_vector existing_tables) (Ref.new Dictionary.empty) (Ref.new 0)
+
+    rename self (name : Text) (binder : Text) =


I think the consensus is all methods should have ## comment, probably with PRIVATE here.

Also I'd appreciate a short note on the logic here - I assume binder is the unique identifier and when we first encounter it, we try to find a shorter name based on name for it, and if we encounter the same identifier again we return the already assigned name?

Exactly. I added docs.
Just curious -- if the type is PRIVATE, do the methods have to be PRIVATE too?

Tbh I don't know, hence I add PRIVATE just to be sure.

I think they are not private, so if you somehow got a value of this private type, they probably would show up in CB on it. But that seems harmless as long as they do not show up in CB search 'in general' (so e.g. in the default mode where you have no value selected).

I think we just had this convention to mark methods as PRIVATE unless meant for users, but it was from time before we got private types so it's a question of how we want to amend that convention for that case.

Also, I'm not sure how this plays with @jdunkerley's code linter/formatter. I assume it may 'warn' about these non-PRIVATE functions, but again I'm not sure.

radeusgd · 2024-08-26T08:03:46Z

distribution/lib/Standard/Database/0.0.0-dev/src/DB_Column.enso

@@ -212,6 +212,11 @@ type DB_Column
    ## PRIVATE
       Column-level manual CTE factoring.

+       Arguments:
+       - name: A prefix to use for the generated let binder.
+       - callback: A functiont that receives the let reference and returns a


Suggested change

- callback: A functiont that receives the let reference and returns a

- callback: A function that receives the let reference and returns a

radeusgd · 2024-08-26T08:18:20Z

distribution/lib/Standard/Database/0.0.0-dev/src/Internal/Base_Generator.enso

@@ -707,15 +717,23 @@ default_fetch_types_query dialect expression context where_filter_always_false_l

 ## PRIVATE
   Helper class for shortening the binder names generated for WITH clauses.
+
+   Each time `rename` is called on an identifier it hasn't seen before, it
+   generates a new, short identifier. The short identifier consits of the `name`


Suggested change

generates a new, short identifier. The short identifier consits of the `name`

generates a new, short identifier. The short identifier consists of the `name`

radeusgd

Looks great, thanks for the comprehensive docs

jdunkerley · 2024-08-27T08:15:43Z

distribution/lib/Standard/Table/0.0.0-dev/src/Column.enso

+            _ : Float   -> x.is_finite
+            _           -> False


Suggested change

_ : Float -> x.is_finite

_ -> False

_ : Float -> x.is_finite

_ : Number -> True

_ : Decimal -> True

_ -> False

Actually changing to:

_ : Float -> x.is_finite _ -> True

Because we're already using expect_numeric, and only Float can be infinite.

GregoryTravis · 2024-08-27T18:52:24Z

test/Table_Tests/src/Common_Table_Operations/Column_Operations_Spec.enso

+            group_builder.teardown <|
+                connection_data.teardown
+
+            if connection_data.connection.dialect.supports_nested_with_clause then


@radeusgd I'm using the dialect for this. It could also be a test flag, but then we would have this flag in both the test config and the dialect, which seems redundant.

👍 that seems very reasonable

GregoryTravis · 2024-08-28T15:59:57Z

Snowflake CI run successful.

GregoryTravis added 30 commits August 7, 2024 14:23

expression-level with

33b2ff7

generate binder

0a406d9

.standalone wip

5f4406c

Merge branch 'develop' into wip/gmt/10306-round-with

7dff97a

missing pattern field

267581b

removed standalone, try scoping with state hack

96dd259

use Text key

fe5d66c

Merge branch 'develop' into wip/gmt/10306-round-with

fd55ff7

storage hack

266de31

Merge branch 'develop' into wip/gmt/10306-round-with

0d60089

round, iif

ede8c28

move State hack to base_generator, pg tests pass

75ffca6

remove bindee body parens for sqlite

a1611b1

Merge branch 'develop' into wip/gmt/10306-round-with

fb4def4

short table name generator

b7be792

Revert "short table name generator"

899b223

This reverts commit b7be792.

with_ctes setting

e042b85

dialect flag

35ed369

with names

1672cd8

is_finite

4166db7

with for short_circuit_special_floating_point

1bf42e8

Merge branch 'develop' into wip/gmt/10306-round-with

1ed8b3e

cleanup

9e035e7

do not use in iif and 10.from-outside-1-2-3, but use on two iifs in r…

d6bc340

…ound_float

remove two unneeded withs

56fcc7b

cleanup

291484c

docs

18c0ff9

cleanup

a8fc1d0

sql_expression docs

ce5c8ad

wip

5b2a413

radeusgd reviewed Aug 23, 2024

View reviewed changes

GregoryTravis added 8 commits August 23, 2024 14:50

Merge branch 'develop' into wip/gmt/10306-round-with

69efbbd

private

3500f74

doc traverse

8017546

switch args, require name

4c033cf

wip

0c3300f

fix indentation

680e6ba

wip

b8007ea

docs

e9e28bf

radeusgd reviewed Aug 26, 2024

View reviewed changes

radeusgd approved these changes Aug 26, 2024

View reviewed changes

GregoryTravis added 2 commits August 26, 2024 10:30

Merge branch 'develop' into wip/gmt/10306-round-with

8cf7210

snowflake test failures

45e45cd

jdunkerley reviewed Aug 27, 2024

View reviewed changes

jdunkerley approved these changes Aug 27, 2024

View reviewed changes

GregoryTravis added 3 commits August 27, 2024 11:06

Merge branch 'develop' into wip/gmt/10306-round-with

d5ff38e

review

647aedf

is_finite True

8460c82

GregoryTravis added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Ready to merge This PR is eligible for automatic merge labels Aug 27, 2024

fix snowflake tests

b8c9ca4

GregoryTravis commented Aug 27, 2024

View reviewed changes

Merge branch 'develop' into wip/gmt/10306-round-with

7b3a823

GregoryTravis added the CI: Ready to merge This PR is eligible for automatic merge label Aug 28, 2024

mergify bot merged commit 8260a95 into develop Aug 28, 2024
36 checks passed

mergify bot deleted the wip/gmt/10306-round-with branch August 28, 2024 18:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Column-level lexically-scoped CTE expressions #10826

Column-level lexically-scoped CTE expressions #10826

GregoryTravis commented Aug 15, 2024 •

edited

Loading

radeusgd Aug 23, 2024

GregoryTravis Aug 23, 2024

radeusgd left a comment

radeusgd Aug 23, 2024

GregoryTravis Aug 23, 2024

radeusgd Aug 26, 2024

radeusgd Aug 26, 2024

GregoryTravis Aug 27, 2024

radeusgd Aug 26, 2024

GregoryTravis Aug 27, 2024

radeusgd left a comment

jdunkerley Aug 27, 2024

GregoryTravis Aug 27, 2024

GregoryTravis Aug 27, 2024

radeusgd Aug 27, 2024

GregoryTravis commented Aug 28, 2024

	- callback: A functiont that receives the let reference and returns a
	- callback: A function that receives the let reference and returns a

	generates a new, short identifier. The short identifier consits of the `name`
	generates a new, short identifier. The short identifier consists of the `name`

Column-level lexically-scoped CTE expressions #10826

Column-level lexically-scoped CTE expressions #10826

Conversation

GregoryTravis commented Aug 15, 2024 • edited Loading

Pull Request Description

Important Notes

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeusgd 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

GregoryTravis commented Aug 28, 2024

GregoryTravis commented Aug 15, 2024 •

edited

Loading