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 #2268: fix expression parsing bug #2288

Merged
merged 4 commits into from
Apr 15, 2015
Merged

fix #2268: fix expression parsing bug #2288

merged 4 commits into from
Apr 15, 2015

Conversation

dgnorton
Copy link
Contributor

@benbjohnson

Before the fix...

bad

After the fix...

good

@benbjohnson
Copy link
Contributor

Can you explain the isroot variable? I don't understand why it's needed.

@dgnorton
Copy link
Contributor Author

@benbjohnson sometimes it needs to replace the entire expression tree and other times in descents into the tree and modifies a sub node. I hate this change...I'm sure there's a cleaner way that I'm just not seeing. This was what I wanted to pair on yesterday. I have an idea that'll I'll try. If that doesn't clean it up then maybe we can pair for 15 - 20 min later today.

Parsing:

WHERE time > now() - 2d AND time < now() + 10d

generated an expression tree that evaluated as:

... AND (time < now()) + 10d

instead of:

... AND time < (now() + 10d)
@benbjohnson
Copy link
Contributor

@dgnorton sure, sounds good.

@toddboom
Copy link
Contributor

👍 if you can make ben happy. 💃

@benbjohnson
Copy link
Contributor

works for me

}
} else {
expr = &BinaryExpr{LHS: expr, RHS: rhs, Op: op}

Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: looks like a superfluous blank line here.

@otoolep
Copy link
Contributor

otoolep commented Apr 15, 2015

Some super-picky nits, test case looks good. +1 on green build.

dgnorton added a commit that referenced this pull request Apr 15, 2015
fix #2268: fix expression parsing bug
@dgnorton dgnorton merged commit 2b21f2d into master Apr 15, 2015
@dgnorton dgnorton deleted the fix-2268 branch April 15, 2015 17:57
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.

4 participants