Skip to content

Refactor boolean cast code, add tests#3016

Merged
gianm merged 1 commit intoapache:masterfrom
navis:expr-boolean-long
Dec 7, 2016
Merged

Refactor boolean cast code, add tests#3016
gianm merged 1 commit intoapache:masterfrom
navis:expr-boolean-long

Conversation

@navis
Copy link
Copy Markdown
Contributor

@navis navis commented May 25, 2016

Currently binary operators in math expr returns integer 0 or 1 for long type arguments, making following expressions regard it as a double. I think returning 0L / 1L would make it more consistent.

@nishantmonu51
Copy link
Copy Markdown
Member

travis timed out -
https://travis-ci.org/druid-io/druid/jobs/132758321

Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

👍 LGTM

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Seems like a strange definition of asBoolean (I would expect Evals.asBoolean(x) to be equivalent to x != 0). But it looks like the behavior was already like this, so no need to change it here.

@dclim
Copy link
Copy Markdown
Contributor

dclim commented Nov 1, 2016

👍 LGTM, @navis when you fix the merge conflicts we can pull this in

@dclim dclim added this to the 0.9.3 milestone Nov 1, 2016
@navis
Copy link
Copy Markdown
Contributor Author

navis commented Nov 2, 2016

@dclim most of issues described here are fixed in #2836. this can be simple refactoring over it ( or should I merge this into it?)

@dclim
Copy link
Copy Markdown
Contributor

dclim commented Nov 2, 2016

@navis there's not too many changes here, I'm fine either way

@fjy
Copy link
Copy Markdown
Contributor

fjy commented Nov 8, 2016

@navis any chance we can finish this up?

@gianm gianm assigned gianm and dclim Nov 29, 2016
@fjy
Copy link
Copy Markdown
Contributor

fjy commented Dec 6, 2016

@navis ping

@navis navis force-pushed the expr-boolean-long branch from 3f52cbd to c7ef019 Compare December 7, 2016 02:50
@navis navis force-pushed the expr-boolean-long branch from c7ef019 to f62203f Compare December 7, 2016 02:50
@navis
Copy link
Copy Markdown
Contributor Author

navis commented Dec 7, 2016

@fjy rebased on master

@gianm
Copy link
Copy Markdown
Contributor

gianm commented Dec 7, 2016

It looks like post-rebasing this is no longer fixing anything or changing behavior (the new EvalTest passes even on master) but is instead just refactoring the boolean cast code and adding a new test. Still valuable, but I'm going to change the PR title to reflect that.

@gianm gianm changed the title Make math expr return long for long type binary op Refactor boolean cast code, add tests Dec 7, 2016
Copy link
Copy Markdown
Contributor

@gianm gianm left a comment

Choose a reason for hiding this comment

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

LGTM

@gianm gianm merged commit 87c61fa into apache:master Dec 7, 2016
dgolitsyn pushed a commit to metamx/druid that referenced this pull request Feb 14, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants