Skip to content

fix(sql): exception thrown when same DECLARE variable used in 2-params function #5806

Merged
bluestreak01 merged 5 commits intomasterfrom
raph_deep_clone_decls
Jul 4, 2025
Merged

fix(sql): exception thrown when same DECLARE variable used in 2-params function #5806
bluestreak01 merged 5 commits intomasterfrom
raph_deep_clone_decls

Conversation

@RaphDal
Copy link
Copy Markdown
Contributor

@RaphDal RaphDal commented Jul 2, 2025

This PR fixes #5804 by deepCloning ExpressionNodes that are used to replace variables.

When SqlParser replaces declare variables with constant expressions, it was re-using the original ExpressionNode instances instead of cloning them. Because PostOrderTreeTraversalAlgo relies on reference comparison to track traversal state, sharing the same node instance between the lhs and rhs of an expression can trick the algorithm into thinking it has already processed lhs.

The problematic check:

// PostOrderTreeTraversalAlgo
if (peek.lhs != null && lastVisited != peek.lhs) { // lastVisited may actually be peek.rhs,
    node = peek.lhs;
}

Example of a failing query:

DECLARE
  @ts := '2025-07-02T13:00:00.000000Z',
  @int := interval(@ts, @ts)
SELECT @int;

nwoolmer
nwoolmer previously approved these changes Jul 2, 2025
View reviewed changes
Copy link
Copy Markdown
Contributor

@nwoolmer nwoolmer left a comment

Choose a reason for hiding this comment

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

nit: title could be more descriptive of the problem, for example:

"fix(sql): exception thrown when duplicate DECLARE variables appear in function argument lists"

Explaining more what the problem was, rather than how you fixed it!

bluestreak01
bluestreak01 requested changes Jul 2, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@bluestreak01 bluestreak01 left a comment

Choose a reason for hiding this comment

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

thank you for picking up and diagnosing this issue so quickly!

While the fix works in this particular instance, I would encourage you to consider a different perspective:

  • post order traversal algo is essentially defective, it doesn't handle lhs and rhs being the same object references
  • instead of fixing the algo defect we are conforming a particular "user" of this algo to fudge different object instance even in cases where it isn't necessary
  • there are many other "users" of this algo that are unaware of the algo limitation can either experience this issue at runtime or sometime in the future making someone else debug the same issue

You should be able to vibe-fix this algo with AI (Claude) in 5 min and not create clone of args now or ever in the future.

What do you think?

@nwoolmer
Copy link
Copy Markdown
Contributor

nwoolmer commented Jul 3, 2025

thank you for picking up and diagnosing this issue so quickly!

While the fix works in this particular instance, I would encourage you to consider a different perspective:

  • post order traversal algo is essentially defective, it doesn't handle lhs and rhs being the same object references
  • instead of fixing the algo defect we are conforming a particular "user" of this algo to fudge different object instance even in cases where it isn't necessary
  • there are many other "users" of this algo that are unaware of the algo limitation can either experience this issue at runtime or sometime in the future making someone else debug the same issue

You should be able to vibe-fix this algo with AI (Claude) in 5 min and not create clone of args now or ever in the future.

What do you think?

The author proposed both solutions at first. I thought that fixing this case in isolation was a safe bet for 9.0.0, which could be followed up with the wider reaching algo fix afterwards.

@RaphDal RaphDal changed the title fix(sql): deepClone expression nodes from declares fix(sql): exception thrown when same DECLARE variable used in 2-params function Jul 3, 2025
@RaphDal
Copy link
Copy Markdown
Contributor Author

RaphDal commented Jul 3, 2025 •
edited
Loading

nit: title could be more descriptive of the problem

Got it, I will do so.

thank you for picking up and diagnosing this issue so quickly!

While the fix works in this particular instance, I would encourage you to consider a different perspective:

  • post order traversal algo is essentially defective, it doesn't handle lhs and rhs being the same object references
  • instead of fixing the algo defect we are conforming a particular "user" of this algo to fudge different object instance even in cases where it isn't necessary
  • there are many other "users" of this algo that are unaware of the algo limitation can either experience this issue at runtime or sometime in the future making someone else debug the same issue

You should be able to vibe-fix this algo with AI (Claude) in 5 min and not create clone of args now or ever in the future.

What do you think?

That was my initial approach (see related patch) but after discussing with Nick, we agreed that this patch was too invasive for such a specific case.

After discussions, we will fix the PostOrderTreeTraversalAlgo implementation.

@glasstiger
Copy link
Copy Markdown
Contributor

glasstiger commented Jul 3, 2025

[PR Coverage check]

😍 pass : 5 / 5 (100.00%)

file detail

path covered line new line coverage
🔵 io/questdb/griffin/PostOrderTreeTraversalAlgo.java 5 5 100.00%

@bluestreak01 bluestreak01 merged commit 0409f8c into master Jul 4, 2025
37 checks passed
@bluestreak01 bluestreak01 deleted the raph_deep_clone_decls branch July 4, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bluestreak01 bluestreak01 bluestreak01 approved these changes

@nwoolmer nwoolmer nwoolmer left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reusing same declared parameter in interval() causes server error

4 participants

@RaphDal @nwoolmer @glasstiger @bluestreak01