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

Transpile snow flake select statement #1203

Open
wants to merge 17 commits into
base: drop-PlanParser-dependency-on-antlr4-
Choose a base branch
from

Conversation

ericvergnaud
Copy link
Contributor

@ericvergnaud ericvergnaud commented Nov 13, 2024

borrows technical idea from #1208, but applies it to a new field, thus avoiding any backwards compatibility issue.

Adds a test for checking transpilation of SnowFlake select statement, and fixes issues

Requires #1201

Progresses #869

Supersedes #1193

@ericvergnaud ericvergnaud marked this pull request as draft November 13, 2024 17:41
@ericvergnaud ericvergnaud changed the base branch from main to drop-PlanParser-dependency-on-antlr4- November 13, 2024 17:45

/**
* Returns true if this expression and all its children have been resolved to a specific schema and false if it still
* contains any unresolved placeholders. Implementations of LogicalPlan can override this (e.g.[[UnresolvedRelation]]
* should return `false`).
*/
lazy val resolved: Boolean = expressions.forall(_.resolved) && childrenResolved
var comments: Array[CommentNode] = Array()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

to be discussed, mutating a node at parse time is no big deal, alternately we could insert comment nodes before first child (or after depending on context), but that also requires mutating the tree

@ericvergnaud ericvergnaud marked this pull request as ready for review November 13, 2024 19:02
@ericvergnaud ericvergnaud marked this pull request as draft November 14, 2024 07:45
@nfx nfx requested a review from jimidle November 14, 2024 14:43
Copy link
Contributor

@jimidle jimidle left a comment

Choose a reason for hiding this comment

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

I think we have started running before we know the correct direction. As per previous comments, we need to design this as it is potentially a big change.

Copy link

Coverage tests results

448 tests  ±0   414 ✅ ±0   4s ⏱️ ±0s
  6 suites ±0    34 💤 ±0 
  6 files   ±0     0 ❌ ±0 

Results for commit 53de7dc. ± Comparison against base commit 444edfe.

@ericvergnaud
Copy link
Contributor Author

ericvergnaud commented Nov 15, 2024

I think we have started running before we know the correct direction. As per previous comments, we need to design this as it is potentially a big change.

Indeed but feasability can only be proven by feasing...

Now we know what can be done, and we have evidence of which problems are introduced

@ericvergnaud ericvergnaud marked this pull request as ready for review November 15, 2024 14:04
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.

2 participants