espertechinc / esper

Esper Complex Event Processing, Streaming SQL and Event Series Analysis
GNU General Public License v2.0
839 stars 259 forks source link

Add compiler flag to have arithmetic expressions with float-types evaluate to float and not double #202

Open icholy opened 4 years ago

icholy commented 4 years ago

When using a float value in an arithmetic expression, the expression incorrectly evaluates to a double instead of float. This leads to a lot of unnecessary casts all over the place.

create schema Thing (value float);
create schema ThingPlusOne (value float);
insert into ThingPlusOne select value + 1 as value from Thing;

Exception in thread "main" com.espertech.esper.compiler.client.EPCompileException: Invalid assignment of column 'value' of type 'java.lang.Double' to event property 'value' typed as 'java.lang.Float', column and parameter types mismatch [insert into ThingPlusOne select value + 1 as value from Thing]

bernhardttom commented 4 years ago

short a=1; short b=2; short c=a*b; <-- doesn't compile ...provided is int

bernhardttom commented 4 years ago

Narrowing "int" to "short" wouldn't be allowed. I'd recommend plugin single row func.

icholy commented 4 years ago

The code I posted is contrived to demonstrate the issue. Writing single row functions for each of the arithmetic expressions in a query would be more verbose than the existing cast calls.

I understand that narrowing an int to a short isn't allowed. The issue is that short * short shouldn't be an int in the first place.

bernhardttom commented 4 years ago

Yeah well in the JVM world short * short = int There could however be a compiler flag to change the behavior.

icholy commented 4 years ago

@bernhardttom I wasn't aware that was the JVM behavior. I've updated the issue to only mention float which I believe is still incorrect.

float a = 123;
float b = 123;
System.out.println(((Object)(a + b)).getClass()); // java.lang.Float
icholy commented 4 years ago

Looks like these should be returning Float.class.

https://github.com/espertechinc/esper/blob/ab90bc7ffa8c58d419a9ea2f5f81634fc1fd8c2f/common/src/main/java/com/espertech/esper/common/internal/util/JavaClassHelper.java#L309-L314

bernhardttom commented 4 years ago

Due to backward compatibility the behavior stays as it is by default.

However a compiler flag to change the default behavior can be added.

Its possible the SQL standard prescribes it or is another reason this is the default behavior i.e. user request. I'll post when I find something.

icholy commented 4 years ago

Seems like it's implementation dependant.

75) Subclause 6.12, "": When the data type of either operand of an arithmetic operator is approximate numeric, the precision of the result is implementation-defined.

https://www.contrib.andrew.cmu.edu/~shadow/sql/sql1992.txt

I tested in postgres and it evaluates to a double

select 1::real + 1::integer -- "double precision"
icholy commented 4 years ago

Seems like this isn't a bug. Also, I can rewrite my original query using a float literal.

create schema Thing (value float);
create schema ThingPlusOne (value float);
insert into ThingPlusOne select value + 1f as value from Thing;
icholy commented 4 years ago

For the sake of completeness, here are the truth tables showing the JVM and Esper arithmetic coercion rules

JVM Arithmetic Coercion Rules

Target Byte Short Integer Long Float Double
Byte Integer Integer Integer Long Float Double
Short Integer Integer Integer Long Float Double
Integer Integer Integer Integer Long Float Double
Long Long Long Long Long Float Double
Float Float Float Float Float Float Double
Double Double Double Double Double Double Double

Esper Arithmetic Coercion Rules

Target Byte Short Integer Long Float Double
Byte Integer Integer Integer Long Double Double
Short Integer Integer Integer Long Double Double
Integer Integer Integer Integer Long Double Double
Long Long Long Long Long Double Double
Float Double Double Double Double Float Double
Double Double Double Double Double Double Double
icholy commented 4 years ago

I started implementing this and ran into this usage: https://github.com/espertechinc/esper/blob/8dd0c24c38c5ed4457229af2eb8383e332d46f37/common/src/main/java/com/espertech/esper/common/internal/epl/expression/ops/ExprInNodeImpl.java#L100

It's being called here:

https://github.com/espertechinc/esper/blob/8dd0c24c38c5ed4457229af2eb8383e332d46f37/common/src/main/java/com/espertech/esper/common/internal/epl/expression/ops/ExprInNodeImpl.java#L58-L63

I need the ExprValidationContext to access the Configuration. Is there a reason I shouldn't be using that validation context?

raguessner commented 4 years ago

Suggest to check for usages of validateWithoutContext (alt-F7 in Idea). Configuration probably needs to be passed in.