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

SQL: Allow long literals #31777

Merged
merged 3 commits into from
Jul 5, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -452,8 +452,14 @@ public Object visitDecimalLiteral(DecimalLiteralContext ctx) {
@Override
public Object visitIntegerLiteral(IntegerLiteralContext ctx) {
BigDecimal bigD = new BigDecimal(ctx.getText());
// TODO: this can be improved to use the smallest type available
return new Literal(source(ctx), bigD.longValueExact(), DataType.INTEGER);

long value = bigD.longValueExact();
DataType type = DataType.LONG;
// try to downsize to int if possible (since that's the most common type)
if ((int) value == value) {
type = DataType.INTEGER;
}
return new Literal(source(ctx), value, type);
}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ private static Conversion conversionToLong(DataType from) {
return Conversion.INTEGER_TO_LONG;
}
if (from == BOOLEAN) {
return Conversion.BOOL_TO_INT; // We emit an int here which is ok because of Java's casting rules
return Conversion.BOOL_TO_LONG;
}
if (from.isString()) {
return Conversion.STRING_TO_LONG;
Expand Down Expand Up @@ -407,7 +407,9 @@ public enum Conversion {

NUMERIC_TO_BOOLEAN(fromLong(value -> value != 0)),
STRING_TO_BOOLEAN(fromString(DataTypeConversion::convertToBoolean, "Boolean")),
DATE_TO_BOOLEAN(fromDate(value -> value != 0));
DATE_TO_BOOLEAN(fromDate(value -> value != 0)),

BOOL_TO_LONG(fromBool(value -> value ? 1L : 0L));
Copy link
Member Author

Choose a reason for hiding this comment

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

My preference would be to move BOOL_TO_LONG up and start using it instead of BOOL_TO_INT for date conversions (we can't right now since the enum entry is defined after).
It's consistent and avoids the int to long conversion.


private final Function<Object, Object> converter;

Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the Elastic License;
* you may not use this file except in compliance with the Elastic License.
*/
package org.elasticsearch.xpack.sql.parser;

import org.elasticsearch.test.ESTestCase;
import org.elasticsearch.xpack.sql.expression.Expression;
import org.elasticsearch.xpack.sql.expression.Literal;
import org.elasticsearch.xpack.sql.expression.function.scalar.arithmetic.Neg;
import org.elasticsearch.xpack.sql.type.DataType;

public class ExpressionTests extends ESTestCase {

private final SqlParser parser = new SqlParser();

public void testLiteralLong() throws Exception {
Expression lt = parser.createExpression(String.valueOf(Long.MAX_VALUE));
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it make sense to test for things like +123 or -123 (having the +/- sign added to the String expression)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Added a test for it (which uncovered an interesting case) - see below.

assertEquals(Literal.class, lt.getClass());
Literal l = (Literal) lt;
assertEquals(Long.MAX_VALUE, l.value());
assertEquals(DataType.LONG, l.dataType());
}

public void testLiteralLongNegative() throws Exception {
// Long.MIN_VALUE doesn't work since it is being interpreted as negate positive.long which is 1 higher than Long.MAX_VALUE
Expression lt = parser.createExpression(String.valueOf(-Long.MAX_VALUE));
assertEquals(Neg.class, lt.getClass());
Neg n = (Neg) lt;
assertTrue(n.foldable());
assertEquals(-Long.MAX_VALUE, n.fold());
assertEquals(DataType.LONG, n.dataType());
}

public void testLiteralInteger() throws Exception {
Expression lt = parser.createExpression(String.valueOf(Integer.MAX_VALUE));
assertEquals(Literal.class, lt.getClass());
Literal l = (Literal) lt;
assertEquals(Integer.MAX_VALUE, l.value());
assertEquals(DataType.INTEGER, l.dataType());
}

public void testLiteralIntegerWithShortValue() throws Exception {
Expression lt = parser.createExpression(String.valueOf(Short.MAX_VALUE));
assertEquals(Literal.class, lt.getClass());
Literal l = (Literal) lt;
assertEquals(Integer.valueOf(Short.MAX_VALUE), l.value());
assertEquals(DataType.INTEGER, l.dataType());
}

public void testLiteralIntegerWithByteValue() throws Exception {
Expression lt = parser.createExpression(String.valueOf(Byte.MAX_VALUE));
assertEquals(Literal.class, lt.getClass());
Literal l = (Literal) lt;
assertEquals(Integer.valueOf(Byte.MAX_VALUE), l.value());
assertEquals(DataType.INTEGER, l.dataType());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,8 @@ public void testConversionToLong() {
{
Conversion conversion = DataTypeConversion.conversionFor(DataType.BOOLEAN, to);
assertNull(conversion.convert(null));
assertEquals(1, conversion.convert(true));
assertEquals(0, conversion.convert(false));
assertEquals(1L, conversion.convert(true));
assertEquals(0L, conversion.convert(false));
}
Conversion conversion = DataTypeConversion.conversionFor(DataType.KEYWORD, to);
assertNull(conversion.convert(null));
Expand Down Expand Up @@ -141,12 +141,19 @@ public void testConversionToBoolean() {
assertEquals(true, conversion.convert(-10));
assertEquals(false, conversion.convert(0));
}
{
Conversion conversion = DataTypeConversion.conversionFor(DataType.LONG, DataType.BOOLEAN);
assertNull(conversion.convert(null));
assertEquals(true, conversion.convert(10L));
assertEquals(true, conversion.convert(-10L));
assertEquals(false, conversion.convert(0L));
}
{
Conversion conversion = DataTypeConversion.conversionFor(DataType.DOUBLE, DataType.BOOLEAN);
assertNull(conversion.convert(null));
assertEquals(true, conversion.convert(10.0));
assertEquals(true, conversion.convert(-10.0));
assertEquals(false, conversion.convert(0.0));
assertEquals(true, conversion.convert(10.0d));
assertEquals(true, conversion.convert(-10.0d));
assertEquals(false, conversion.convert(0.0d));
}
{
Conversion conversion = DataTypeConversion.conversionFor(DataType.KEYWORD, DataType.BOOLEAN);
Expand Down