Skip to content

Test and fix cast from bigint to varchar #17152

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.

Sign up for GitHub

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

Merged
highker merged 1 commit into from
Feb 7, 2022
Merged

Test and fix cast from bigint to varchar #17152

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
11 changes: 9 additions & 2 deletions presto-main/src/main/java/com/facebook/presto/type/BigintOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import static com.facebook.presto.common.function.OperatorType.XX_HASH_64;
import static com.facebook.presto.common.type.BigintType.BIGINT;
import static com.facebook.presto.spi.StandardErrorCode.DIVISION_BY_ZERO;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_CAST_ARGUMENT;
import static com.facebook.presto.spi.StandardErrorCode.NUMERIC_VALUE_OUT_OF_RANGE;
import static io.airlift.slice.Slices.utf8Slice;
import static java.lang.Float.floatToRawIntBits;
Expand Down Expand Up @@ -270,10 +271,16 @@ public static long castToReal(@SqlType(StandardTypes.BIGINT) long value)
@ScalarOperator(CAST)
@LiteralParameters("x")
@SqlType("varchar(x)")
public static Slice castToVarchar(@SqlType(StandardTypes.BIGINT) long value)
public static Slice castToVarchar(@LiteralParameter("x") long x, @SqlType(StandardTypes.BIGINT) long value)
{
// todo optimize me
return utf8Slice(String.valueOf(value));
String stringValue = String.valueOf(value);
// String is all-ASCII, so String.length() here returns actual code points count
if (stringValue.length() <= x) {
return utf8Slice(stringValue);
}

throw new PrestoException(INVALID_CAST_ARGUMENT, format("Value %s cannot be represented as varchar(%s)", value, x));
}

@ScalarOperator(HASH_CODE)
Expand Down
8 changes: 7 additions & 1 deletion presto-main/src/test/java/com/facebook/presto/operator/scalar/AbstractTestFunctions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import com.facebook.presto.sql.analyzer.SemanticErrorCode;
import com.google.common.collect.ImmutableList;
import io.airlift.slice.Slice;
import org.intellij.lang.annotations.Language;
import org.testng.annotations.AfterClass;
import org.testng.annotations.BeforeClass;

Expand Down Expand Up @@ -152,6 +153,11 @@ protected void assertInvalidFunction(String projection, ErrorCodeSupplier expect
functionAssertions.assertInvalidFunction(projection, expectedErrorCode);
}

protected void assertFunctionThrowsIncorrectly(@Language("SQL") String projection, Class<? extends Throwable> throwableClass, @Language("RegExp") String message)
Copy link
Copy Markdown

@highker highker Jan 27, 2022

Choose a reason for hiding this comment

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

not used?

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Jan 31, 2022

Choose a reason for hiding this comment

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

It will be used in other similar PRs, like #17232

{
functionAssertions.assertFunctionThrowsIncorrectly(projection, throwableClass, message);
}

protected void assertNumericOverflow(String projection, String message)
{
functionAssertions.assertNumericOverflow(projection, message);
Expand All @@ -162,7 +168,7 @@ protected void assertInvalidCast(String projection)
functionAssertions.assertInvalidCast(projection);
}

protected void assertInvalidCast(String projection, String message)
protected void assertInvalidCast(@Language("SQL") String projection, String message)
{
functionAssertions.assertInvalidCast(projection, message);
}
Expand Down
9 changes: 9 additions & 0 deletions presto-main/src/test/java/com/facebook/presto/operator/scalar/FunctionAssertions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@
import io.airlift.slice.Slice;
import io.airlift.slice.Slices;
import io.airlift.units.DataSize;
import org.intellij.lang.annotations.Language;
import org.joda.time.DateTime;
import org.joda.time.DateTimeZone;
import org.openjdk.jol.info.ClassLayout;
Expand Down Expand Up @@ -148,6 +149,7 @@
import static java.util.Objects.requireNonNull;
import static java.util.concurrent.Executors.newCachedThreadPool;
import static java.util.concurrent.Executors.newScheduledThreadPool;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertNull;
Expand Down Expand Up @@ -404,6 +406,13 @@ public void assertInvalidFunction(String projection, ErrorCodeSupplier expectedE
}
}

public void assertFunctionThrowsIncorrectly(@Language("SQL") String projection, Class<? extends Throwable> throwableClass, @Language("RegExp") String message)
{
assertThatThrownBy(() -> evaluateInvalid(projection))
.isInstanceOf(throwableClass)
.hasMessageMatching(message);
}

public void assertNumericOverflow(String projection, String message)
{
try {
Expand Down
38 changes: 38 additions & 0 deletions presto-main/src/test/java/com/facebook/presto/sql/TestExpressionInterpreter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@
import static com.facebook.presto.common.type.VarcharType.VARCHAR;
import static com.facebook.presto.common.type.VarcharType.createVarcharType;
import static com.facebook.presto.operator.scalar.ApplyFunction.APPLY_FUNCTION;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_CAST_ARGUMENT;
import static com.facebook.presto.spi.relation.ExpressionOptimizer.Level;
import static com.facebook.presto.spi.relation.ExpressionOptimizer.Level.OPTIMIZED;
import static com.facebook.presto.spi.relation.ExpressionOptimizer.Level.SERIALIZABLE;
Expand All @@ -102,6 +103,7 @@
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertThrows;
import static org.testng.Assert.assertTrue;
import static org.testng.Assert.fail;

public class TestExpressionInterpreter
{
Expand Down Expand Up @@ -584,6 +586,42 @@ public void testCastToString()
// TODO enabled when DECIMAL is default for literal: assertOptimizedEquals("cast(12345678901234567890.123 as VARCHAR)", "'12345678901234567890.123'");
}

@Test
public void testCastBigintToBoundedVarchar()
{
assertEvaluatedEquals("CAST(12300000000 AS varchar(11))", "'12300000000'");
assertEvaluatedEquals("CAST(12300000000 AS varchar(50))", "'12300000000'");

try {
evaluate("CAST(12300000000 AS varchar(3))", true);
fail("Expected to throw an INVALID_CAST_ARGUMENT exception");
}
catch (PrestoException e) {
try {
assertEquals(e.getErrorCode(), INVALID_CAST_ARGUMENT.toErrorCode());
assertEquals(e.getMessage(), "Value 12300000000 cannot be represented as varchar(3)");
}
catch (Throwable failure) {
failure.addSuppressed(e);
throw failure;
}
}

try {
evaluate("CAST(-12300000000 AS varchar(3))", true);
}
catch (PrestoException e) {
Copy link
Copy Markdown
Contributor

@prithvip prithvip Jan 27, 2022

Choose a reason for hiding this comment

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

nit: Could this testing logic be put into a private static "assertThrows" (or something like that) helper method so as to match the style of the rest of this class? It could be helpful in the future too to test exceptions being thrown properly, and would be less awkward than catching an exception thrown by an assertion.

Copy link
Copy Markdown
Contributor Author

@v-jizhang v-jizhang Jan 31, 2022

Choose a reason for hiding this comment

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

I will refactor it in PR #17232

try {
assertEquals(e.getErrorCode(), INVALID_CAST_ARGUMENT.toErrorCode());
assertEquals(e.getMessage(), "Value -12300000000 cannot be represented as varchar(3)");
}
catch (Throwable failure) {
failure.addSuppressed(e);
throw failure;
}
}
}

@Test
public void testCastToBoolean()
{
Expand Down
51 changes: 51 additions & 0 deletions ...src/test/java/com/facebook/presto/sql/planner/iterative/rule/TestSimplifyExpressions.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@

import com.facebook.presto.common.type.Type;
import com.facebook.presto.metadata.MetadataManager;
import com.facebook.presto.spi.PrestoException;
import com.facebook.presto.spi.relation.RowExpression;
import com.facebook.presto.spi.relation.VariableReferenceExpression;
import com.facebook.presto.sql.TestingRowExpressionTranslator;
Expand All @@ -23,6 +24,7 @@
import com.facebook.presto.sql.planner.PlanVariableAllocator;
import com.facebook.presto.sql.planner.TypeProvider;
import com.facebook.presto.sql.planner.VariablesExtractor;
import com.facebook.presto.sql.tree.Cast;
import com.facebook.presto.sql.tree.Expression;
import com.facebook.presto.sql.tree.ExpressionRewriter;
import com.facebook.presto.sql.tree.ExpressionTreeRewriter;
Expand All @@ -42,6 +44,7 @@
import static com.facebook.presto.SessionTestUtils.TEST_SESSION;
import static com.facebook.presto.common.type.BooleanType.BOOLEAN;
import static com.facebook.presto.metadata.MetadataManager.createTestMetadataManager;
import static com.facebook.presto.spi.StandardErrorCode.INVALID_CAST_ARGUMENT;
import static com.facebook.presto.sql.ExpressionUtils.binaryExpression;
import static com.facebook.presto.sql.ExpressionUtils.extractPredicates;
import static com.facebook.presto.sql.ExpressionUtils.rewriteIdentifiersToSymbolReferences;
Expand All @@ -51,6 +54,7 @@
import static java.util.stream.Collectors.toList;
import static java.util.stream.Collectors.toMap;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.fail;

public class TestSimplifyExpressions
{
Expand Down Expand Up @@ -132,6 +136,45 @@ public void testExtractCommonPredicates()
" OR (A51 AND A52) OR (A53 AND A54) OR (A55 AND A56) OR (A57 AND A58) OR (A59 AND A60)");
}

@Test
public void testCastBigintToBoundedVarchar()
{
// the varchar type length is enough to contain the number's representation
assertSimplifies("CAST(12300000000 AS varchar(11))", "'12300000000'");
// The last argument "'-12300000000'" is varchar(12). Need varchar(50) to the following test pass.
//assertSimplifies("CAST(-12300000000 AS varchar(50))", "CAST('-12300000000' AS varchar(50))", "'-12300000000'");

// cast from bigint to varchar fails, so the expression is not modified
try {
assertSimplifies("CAST(12300000000 AS varchar(3))", "CAST(12300000000 AS varchar(3))");
fail("Expected to throw an PrestoException exception");
}
catch (PrestoException e) {
try {
assertEquals(e.getErrorCode(), INVALID_CAST_ARGUMENT.toErrorCode());
assertEquals(e.getMessage(), "Value 12300000000 cannot be represented as varchar(3)");
}
catch (Throwable failure) {
failure.addSuppressed(e);
throw failure;
}
}

try {
assertSimplifies("CAST(-12300000000 AS varchar(3))", "CAST(-12300000000 AS varchar(3))");
}
catch (PrestoException e) {
Copy link
Copy Markdown
Contributor

@prithvip prithvip Jan 27, 2022

Choose a reason for hiding this comment

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

Same here as above

try {
assertEquals(e.getErrorCode(), INVALID_CAST_ARGUMENT.toErrorCode());
assertEquals(e.getMessage(), "Value -12300000000 cannot be represented as varchar(3)");
}
catch (Throwable failure) {
failure.addSuppressed(e);
throw failure;
}
}
}

private static void assertSimplifies(String expression, String expected)
{
assertSimplifies(expression, expected, null);
Expand Down Expand Up @@ -177,5 +220,13 @@ public Expression rewriteLogicalBinaryExpression(LogicalBinaryExpression node, V
.collect(toList());
return binaryExpression(node.getOperator(), predicates);
}

@Override
public Expression rewriteCast(Cast node, Void context, ExpressionTreeRewriter<Void> treeRewriter)
{
// the `expected` Cast expression comes out of the AstBuilder with the `typeOnly` flag set to false.
// always set the `typeOnly` flag to false so that it does not break the comparison.
return new Cast(node.getExpression(), node.getType(), node.isSafe(), false);
}
}
}
16 changes: 10 additions & 6 deletions presto-main/src/test/java/com/facebook/presto/type/TestBigintOperators.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
import static com.facebook.presto.common.type.DoubleType.DOUBLE;
import static com.facebook.presto.common.type.RealType.REAL;
import static com.facebook.presto.common.type.VarcharType.VARCHAR;
import static com.facebook.presto.common.type.VarcharType.createVarcharType;
import static java.lang.String.format;

public class TestBigintOperators
Expand Down Expand Up @@ -188,31 +189,34 @@ public void testCastToBigint()
@Test
public void testCastToVarchar()
{
assertFunction("cast(37 as varchar)", VARCHAR, "37");
assertFunction("cast(BIGINT '37' as varchar)", VARCHAR, "37");
assertFunction("cast(100000000017 as varchar)", VARCHAR, "100000000017");
assertFunction("cast(100000000017 as varchar(13))", createVarcharType(13), "100000000017");
assertFunction("cast(100000000017 as varchar(50))", createVarcharType(50), "100000000017");
assertInvalidCast("cast(100000000017 as varchar(2))", "Value 100000000017 cannot be represented as varchar(2)");
}

@Test
public void testCastToDouble()
{
assertFunction("cast(37 as double)", DOUBLE, 37.0);
assertFunction("cast(BIGINT '37' as double)", DOUBLE, 37.0);
assertFunction("cast(100000000017 as double)", DOUBLE, 100000000017.0);
}

@Test
public void testCastToFloat()
{
assertFunction("cast(37 as real)", REAL, 37.0f);
assertFunction("cast(BIGINT '37' as real)", REAL, 37.0f);
assertFunction("cast(-100000000017 as real)", REAL, -100000000017.0f);
assertFunction("cast(0 as real)", REAL, 0.0f);
assertFunction("cast(BIGINT '0' as real)", REAL, 0.0f);
}

@Test
public void testCastToBoolean()
{
assertFunction("cast(37 as boolean)", BOOLEAN, true);
assertFunction("cast(BIGINT '37' as boolean)", BOOLEAN, true);
assertFunction("cast(100000000017 as boolean)", BOOLEAN, true);
assertFunction("cast(0 as boolean)", BOOLEAN, false);
assertFunction("cast(BIGINT '0' as boolean)", BOOLEAN, false);
}

@Test
Expand Down