Skip to content
This repository has been archived by the owner on Jan 28, 2021. It is now read-only.

Zachmu/bug fixes to contribute #784

Merged
merged 16 commits into from
Jul 10, 2019
Merged

Zachmu/bug fixes to contribute #784

merged 16 commits into from
Jul 10, 2019

Conversation

zachmu
Copy link
Contributor

@zachmu zachmu commented Jul 3, 2019 •
edited
Loading

Several bug fixes and improvements:

  1. Float comparisons were broken because they were being truncated to integers. Fixed and added many tests with float values.
  2. Negative float values were getting optimized away to 0 by the parser, fixed and added tests.
  3. Added integration tests of IS NULL, IS NOT NULL
  4. Implemented IS TRUE, IS FALSE, and NOT versions of each
  5. Fixed some typos in the source
  6. Allow conversion of string > bool to match MySQL semantics
  7. Fixed bug where some non-column filter expressions were getting effectively optimized away by being force-converted to a boolean

@zachmu zachmu force-pushed the zachmu/bug-fixes-to-contribute branch from 5f46739 to bdf8233 Compare July 3, 2019 19:46
@zachmu
Copy link
Contributor Author

zachmu commented Jul 3, 2019

Sorry for the relatively messy diff (includes several recent changes from your master), not sure what caused it exactly. Open to suggestions for how to make it cleaner.

@mjesuele mjesuele force-pushed the zachmu/bug-fixes-to-contribute branch from bdf8233 to 59a3829 Compare July 3, 2019 21:42
@zachmu
Copy link
Contributor Author

zachmu commented Jul 3, 2019

Cleaned this up with the help of my colleague, ready to be reviewed.

@zachmu zachmu force-pushed the zachmu/bug-fixes-to-contribute branch 2 times, most recently from 515c313 to d40a464 Compare July 3, 2019 22:20
erizocosmico
erizocosmico approved these changes Jul 4, 2019
View reviewed changes
Copy link
Contributor

@erizocosmico erizocosmico left a comment

Choose a reason for hiding this comment

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

Just a minor nitpick. Other than that LGTM. Good catches! and thank you so much for the contribution.

sql/expression/istrue.go Outdated Show resolved Hide resolved
juanjux
juanjux suggested changes Jul 4, 2019
View reviewed changes
sql/expression/istrue.go Outdated Show resolved Hide resolved
sql/expression/istrue_test.go Show resolved Hide resolved
@juanjux
Copy link
Contributor

juanjux commented Jul 4, 2019

Only a couple easy to fix things, thanks for the PR!

@zachmu zachmu force-pushed the zachmu/bug-fixes-to-contribute branch from 6f02dac to 89d4baa Compare July 9, 2019 16:49
zachmu added 15 commits July 9, 2019 11:21
@zachmu
Fixed float comparisons, added lots of tests for same.
8f2dafc 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Formatted new tests more succinctly.
1590eb3 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Removed redundant tests, added an order by test
c6d9cae 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Fixed remaining broken tests
725d21a 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Implemented is true / is false expressions
ecd0be7 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Added new table with nil values
00b8a2f 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Lots more tests of IS NULL, IS TRUE, etc.
c5c6fcf 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Uncommented test cases
ce330c4 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Fix broken test for not expression
e522139 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Fixed bug in comparisons for negative float literals
072ad2b 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
After merging from master, replace vitess with our forked version
35c0ecc 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Reverting local changes to go.mod
6f9a12a 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Fixed an error in receiver type for String() method of IsTrue
fb26ee9 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Fixed a typo
f86f7e2 
Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Fixed bug in is false evaluation for nil values. Added tests for bool…
ded9391 
… evaluation of string fields. This required allowing conversion of strings to bool (always false, as MySQL). This in turn exposed a bug in optimization, where non-column expressions were inappropriately getting coerced into boolean values. Removed the boolean casting, which is slightly less performant but correct in all cases.

Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu zachmu force-pushed the zachmu/bug-fixes-to-contribute branch from 89d4baa to ded9391 Compare July 9, 2019 18:28
@zachmu
Updated IsTrue to use the new Expression interface (TransformUp -> Wi…
99dc027 
…thChildren)

Signed-off-by: Zach Musgrave <zach@liquidata.co>
@zachmu
Copy link
Contributor Author

zachmu commented Jul 9, 2019

Addressed PR feedback and updated to include the recent interface change to Expression.

Adding tests for boolean columns for IsTrue evaluation as Juanjo suggested required being able to evaluate strings as booleans. Once implemented, this exposed a bug in the filter optimizer, where non-column expressions were incorrectly being coerced to booleans. Really, this is the same bug that was causing UnaryMinus expressions (for negative floats) to not work correctly. My fix is more general than my original PR: don't coerce to boolean during optimization. This is slightly less performant but more correct.

Please take another look and merge if it looks good.

@juanjux
Copy link
Contributor

juanjux commented Jul 10, 2019

Thanks for the fixes!

@ajnavarro ajnavarro merged commit 9b083fa into src-d:master Jul 10, 2019
@Hydrocharged Hydrocharged deleted the zachmu/bug-fixes-to-contribute branch September 16, 2019 19:08
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@erizocosmico erizocosmico erizocosmico approved these changes

@ajnavarro ajnavarro ajnavarro approved these changes

@juanjux juanjux juanjux approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@zachmu @juanjux @ajnavarro @erizocosmico