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

Fix precedence of CTEs with respect to set operations in SnowFlake #1273

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

asnare
Copy link
Contributor

@asnare asnare commented Dec 3, 2024

This PR fixes the handling of CTEs with respect to set operations; prior to this the CTE was associated with only the first SELECT in the sequence of set operations instead of the entire set.

Incidental changes:

  • Additional functional tests (that already passed) for a few of the CTEs.
  • Equivalent functional tests for TSQL to verify the behaviour was already correct.

Resolves: #1267

@asnare asnare added bug Something isn't working feat/ir everything related to abstract syntax trees sql/snowflake labels Dec 3, 2024
@asnare asnare self-assigned this Dec 3, 2024
@asnare asnare requested a review from a team as a code owner December 3, 2024 18:00
Copy link

github-actions bot commented Dec 3, 2024

Coverage tests results

472 tests  +8   435 ✅ +8   4s ⏱️ ±0s
  6 suites ±0    37 💤 ±0 
  6 files   ±0     0 ❌ ±0 

Results for commit e42b2d3. ± Comparison against base commit 502e46f.

♻️ This comment has been updated with latest results.

@asnare
Copy link
Contributor Author

asnare commented Dec 3, 2024

Looks like some of the functional tests have cosmetic differences with the Python versions, specifically extra parentheses when transpiling set operations.

I'll move the affected tests into the core-specific test set.

Copy link
Contributor

@vil1 vil1 left a comment

Choose a reason for hiding this comment

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

LGTM but some functional tests that are failing on python-sqlglot should be moved to core_engine

@vil1 vil1 requested a review from a team December 4, 2024 08:42
| a * t.d
|FROM t;""".stripMargin transpilesTo
"Common Table Expressions (CTEs)" should {
"""support expressions""" in {
Copy link
Contributor

Choose a reason for hiding this comment

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

would "have precedence" be better ?

@ericvergnaud
Copy link
Contributor

Looks like some of the functional tests have cosmetic differences with the Python versions, specifically extra parentheses when transpiling set operations.

I'll move the affected tests into the core-specific test set.

Not sure we want to drop the python tests altogether, notably n the light of the Multiplexer MVP scope ?

@sundarshankar89
Copy link
Contributor

Looks like some of the functional tests have cosmetic differences with the Python versions, specifically extra parentheses when transpiling set operations.

I'll move the affected tests into the core-specific test set.

Move those tests under core_engine folder

-- databricks sql:
WITH a AS (SELECT 1, 2, 3)

(SELECT 4, 5, 6)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the parenthesis are required here

WITH a AS (SELECT 1, 2, 3),
b AS (SELECT 4, 5, 6),
c AS (SELECT * FROM a)
(SELECT * from b)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the parenthesis are required here

-- databricks sql:
WITH a AS (SELECT 1, 2, 3)

(SELECT 4, 5, 6)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the parenthesis are required here

WITH a AS (SELECT 1, 2, 3),
b AS (SELECT 4, 5, 6),
c AS (SELECT * FROM a)
(SELECT * from b)
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the parenthesis are required here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working feat/ir everything related to abstract syntax trees sql/snowflake
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG]: Precedence for CTEs with Snowflake is incorrect with respect to set operations
4 participants