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

Simplify the call valuer to remove the influxql.Expr references #18

Merged
merged 1 commit into from
Mar 30, 2018

Conversation

jsternberg
Copy link
Contributor

The call valuer implementations usually just care about the interface
values. They rarely if ever cared about if it was a variable reference
or a literal. This modifies the interface to reflect that and moves the
logic for how to convert an individual AST node to an interface in the
influxql package instead so the logic is the same everywhere.

@jsternberg
Copy link
Contributor Author

@Tomcat-Engineering this is what I ended up coming up with. If you think something else would be more simple to make it easier to implement functions, just tell me here so we can ensure the interface matches with the expectations of the calling developer.

@jsternberg jsternberg force-pushed the js-simplify-call-valuer branch 2 times, most recently from 45c3a5b to 09283a9 Compare March 28, 2018 13:58
@Tomcat-Engineering
Copy link
Contributor

Is the lazy evaluation of arguments (passing a function which gives you the arg values, rather than just passing in the arg values) really worth the added complexity? Are there common situations which this is going to speed up? I guess the user might get an error message faster if they supply the wrong number of args or something, but that case doesn't seem worth optimising to this extent.

@Tomcat-Engineering
Copy link
Contributor

It is possible that we might want functions to know where their arg values came from, in particular whether they were literals or not. The example which springs to mind is unsigned vs integer literals. The parser parses "1" as an IntegerLiteral, not an UnsignedLiteral. The latter is only used for values which don't fit into a signed int64. Under the proposed system, I think the math function won't be able to distinguish between a literal "1" argument and an Integer value from the database?

Imagine a hypothetical add(x, y) function. If we add two unsigned values, the user might expect an unsigned value to be returned. But if the user calls add(x::unsigned, 1) the math function will just see add(unsigned, integer) which will have to return a signed int.

I guess the binary operator code may already have addressed this issue?

@jsternberg
Copy link
Contributor Author

Here's a benchmark between the two. It seems the indirection isn't worth it from a performance standpoint like I thought it would be. I figured that it would reduce memory usage in the case of allocating 1 or 2 arguments, but it didn't.

benchmark           old ns/op     new ns/op     delta
BenchmarkEval-8     524           525           +0.19%

benchmark           old allocs     new allocs     delta
BenchmarkEval-8     7              7              +0.00%

benchmark           old bytes     new bytes     delta
BenchmarkEval-8     104           88            -15.38%

So I'll switch it to just passing in a slice of interfaces.

I would like to see a way to know the source of a value, but I don't see a way without making it more complicated. We'll likely have to get that in validation. There's some validation code in the type checker preventing binary operations between integer and unsigned if it is an unsigned literal, but I'm not really sure there's a compelling reason to have even done that. If a person is going to write that type of query, they'll just get overflow anyway so I'm not too concerned with preventing misuse of functions. The type valuer is much more complete than it was in 1.5 with this refactor so it will correctly pick up on those kinds of weird things.

The call valuer implementations usually just care about the interface
values. They rarely if ever cared about if it was a variable reference
or a literal. This modifies the interface to reflect that and moves the
logic for how to convert an individual AST node to an interface in the
influxql package instead so the logic is the same everywhere.
@jsternberg jsternberg merged commit 145e067 into master Mar 30, 2018
@jsternberg jsternberg deleted the js-simplify-call-valuer branch March 30, 2018 23:54
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.

None yet

3 participants