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

Implement floor, ceil, and round functions #9632

Merged
merged 1 commit into from Apr 4, 2018

Conversation

Ahmah2009
Copy link
Contributor

@Ahmah2009 Ahmah2009 commented Mar 26, 2018

implement floor, ceil, round functions (listed in #5930) requested in #3691.

the functions have been implemented based on /query/math.go as math functions. #9499
so all the (floor, ceiling, truncate) can be used anywhere such as in a field or an expression.

Example usage:
select load from cpu

name: cpu
time load
1434055562000000000 42
1434055562000000000 15.4
1434055562000000000 78


select floor(load) from cpu

name: cpu
time floor
1434055562000000000 42
1434055562000000000 15
1434055562000000000 78


`select ceil(load) from cpu

name: cpu
time ceiling
1434055562000000000 42
1434055562000000000 16
1434055562000000000 78


select round(load) from cpu

name: cpu
time truncate
1434055562000000000 42
1434055562000000000 15
1434055562000000000 78


Required for all non-trivial PRs
  • Sign CLA (if not already signed)
Required only if applicable
  • Provide example syntax
  • Update man page when modifying a command

@ghost ghost added the proposed label Mar 26, 2018
query/math.go Outdated
@@ -9,7 +9,7 @@ import (

func isMathFunction(call *influxql.Call) bool {
switch call.Name {
case "sin", "cos", "tan":
case "sin", "cos", "tan", "floor", "ceiling", "truncate":
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you name these ceil(), floor(), and round()? I'm not sure why truncate is needed. I think you can get that behavior by using a cast such as value::integer. If you can, please implement round instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have pushed a new commit with rename ceiling to ceil and remove truncate and implemented round .

query/math.go Outdated
@@ -49,6 +49,12 @@ func (v *MathValuer) Call(name string, args []influxql.Expr) (interface{}, bool)
return v.callTrigFunction(math.Cos, args[0])
case "tan":
return v.callTrigFunction(math.Tan, args[0])
case "floor":
return v.callTrigFunction(math.Floor, args[0])
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you'll need to implement a new function for this. The argument type for these three functions should probably be restricted to only floats. It doesn't make sense to call any of these with an integer. I think these functions should also return integers rather than floats.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

callRoundFunction will called now.

@jsternberg
Copy link
Contributor

I'm going to take a look at your updates in a bit, but I just wanted to say that we want these functions to return floats if the input is a float contrary to what I said previously.

@Ahmah2009
Copy link
Contributor Author

@jsternberg
It is actually return float if the input is float. if the input is integer it will return it directly.

@jsternberg
Copy link
Contributor

Hi @Ahmah2009, sorry for not responding for a few days. There was another related PR that would affect what I said on this PR, so I decided to wait.

Can you update this PR to use this updated interface? The rationale and what was done can be found in this PR: influxdata/influxql#18.

For me to accept the PR, these three functions will need to follow a few rules.

  1. The function will return the same type as its input.
  2. It needs to formally support float, integer, and unsigned types. These are respectively float64, int64, and uint64.
  3. For integer and unsigned, these functions should just be identity functions.

You will need to modify the MathTypeMapper and MathValuer.

Thanks. If you need any help or don't have time to update the PR, please let us know so we know to either do the implementation ourselves or encourage a different contribution. We appreciate the time you take to improve InfluxDB and I will do my best to respond promptly to this PR.

query/math.go Outdated
@@ -22,7 +22,7 @@ func (MathTypeMapper) MapType(measurement *influxql.Measurement, field string) i

func (MathTypeMapper) CallType(name string, args []influxql.DataType) (influxql.DataType, error) {
switch name {
case "sin", "cos", "tan":
case "sin", "cos", "tan", "floor", "ceil", "round":
Copy link
Contributor

Choose a reason for hiding this comment

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

You're going to need to modify this part to return the correct type. So something like this:

switch name {
case "sin", "cos", "tan":
    return influxql.Float, nil
case "floor", "ceil", "round":
    switch args[0] {
    case influxql.Float, influxql.Integer, influxql.Unsigned, influxql.Unknown:
        return args[0], nil
    default:
        return nil, fmt.Errorf("invalid argument type for first argument in %s(): %s", name, args[0])
}

The same or a similar error should be returned for the trig functions, but I'll take care of that so we can ignore that for now. The important part is you need to whitelist OK types. Unknown should be considered an OK type since it might be correct, but just not present in the database.

@@ -46,6 +46,12 @@ func (v MathValuer) Call(name string, args []interface{}) (interface{}, bool) {
return v.callTrigFunction(math.Cos, arg0)
case "tan":
return v.callTrigFunction(math.Tan, arg0)
case "floor":
Copy link
Contributor

Choose a reason for hiding this comment

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

For these functions, it should be easier to just do this:

switch arg0 := arg0.(type) {
case float64:
    return math.Floor(arg0), true
case int64, uint64:
    return arg0, true
default:
    return nil, true
}

I wouldn't worry about doing type checking here. While it's possible for us to call the function incorrectly in testing, the type checker should take care of things during actual execution.

I don't think you need a separate function and indirection for this since it's rather simple.

query/math.go Outdated
@@ -21,9 +22,17 @@ func (MathTypeMapper) MapType(measurement *influxql.Measurement, field string) i
}

func (MathTypeMapper) CallType(name string, args []influxql.DataType) (influxql.DataType, error) {

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need the extra newline here.

query/math.go Outdated
@@ -71,16 +80,16 @@ func (MathValuer) callTrigFunction(fn func(x float64) float64, arg0 interface{})
}

func (MathValuer) callRoundFunction(fn func(x float64) float64, arg0 interface{}) (interface{}, bool) {
var value float64

Copy link
Contributor

Choose a reason for hiding this comment

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

I see you updated this with my advice, but I think that just manually inlining this would be better since the code itself is relatively small. That way we would be able to remove this entire function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had new commit with the inlining the callRoundFunction as your advice, But I would like to ask it is better inline such like function or write it in separate functions?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case it's better to inline it. The previous system had more complicated logic where you had to convert and handle different inputs types. Since it's now as simple as a single type cast, inlining it just means there's less indirection. If we find it too repetitive, we can always refactor it into a function later.

Copy link
Contributor

@jsternberg jsternberg left a comment

Choose a reason for hiding this comment

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

I just need you to squash all of the commits into a single commit (so no merge commits) and we're good.

@jsternberg jsternberg changed the title implement floor, ceiling, truncate functions Implement floor, ceiling, and round functions Apr 4, 2018
@jsternberg jsternberg changed the title Implement floor, ceiling, and round functions Implement floor, ceil, and round functions Apr 4, 2018
@Ahmah2009
Copy link
Contributor Author

I have squashed the commits into one commit.

@Ahmah2009
Copy link
Contributor Author

@dswarbrick

math.Round() was implemented in go 1.10:

https://golang.org/doc/go1.10#math

InfluxDB requires Go 1.9.2 which not have math.Round:

https://github.com/influxdata/influxdb/blob/master/CONTRIBUTING.md#installing-go

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.

None yet

3 participants