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

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

Open
icholy opened this issue Jun 18, 2020 · 12 comments

Comments

@icholy
Copy link
Contributor

icholy commented Jun 18, 2020

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]

@icholy icholy changed the title Arithmetic expressions evaluate to incorrect type when using float or short Arithmetic expressions evaluate to incorrect type when using float, short, or byte Jun 18, 2020
@bernhardttom
Copy link
Contributor

bernhardttom commented Jun 18, 2020

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

@bernhardttom
Copy link
Contributor

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

@icholy
Copy link
Contributor Author

icholy commented Jun 19, 2020

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
Copy link
Contributor

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

@icholy icholy changed the title Arithmetic expressions evaluate to incorrect type when using float, short, or byte Arithmetic expressions evaluate to incorrect type when using float Jun 19, 2020
@icholy
Copy link
Contributor Author

icholy commented Jun 19, 2020

@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
Copy link
Contributor Author

icholy commented Jun 19, 2020

Looks like these should be returning Float.class.

if ((boxedOne == Float.class) && (!isFloatingPointClass(typeTwo))) {
return Double.class;
}
if ((boxedTwo == Float.class) && (!isFloatingPointClass(typeOne))) {
return Double.class;
}

@bernhardttom bernhardttom changed the title Arithmetic expressions evaluate to incorrect type when using float Add compiler flag to have arithmetic expressions with float-types evaluate to float and not double Jun 19, 2020
@bernhardttom
Copy link
Contributor

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
Copy link
Contributor Author

icholy commented Jun 19, 2020

Seems like it's implementation dependant.

  1. 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
Copy link
Contributor Author

icholy commented Jun 19, 2020

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 icholy closed this as completed Jun 19, 2020
@icholy
Copy link
Contributor Author

icholy commented Jun 19, 2020

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
Copy link
Contributor Author

icholy commented Jul 28, 2020

I started implementing this and ran into this usage:

coercionType = JavaClassHelper.getCommonCoercionType(comparedTypes.toArray(new EPType[comparedTypes.size()]));

It's being called here:

public ExprNode validate(ExprValidationContext validationContext) throws ExprValidationException {
validateWithoutContext();
return null;
}
public void validateWithoutContext() throws ExprValidationException {

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

@raguessner
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants