Skip to content

Change RowExpressionFormatter output to be closer to valid SQL #16335

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
rongrong merged 1 commit into from
Jun 29, 2021
Merged

Change RowExpressionFormatter output to be closer to valid SQL #16335

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
33 changes: 29 additions & 4 deletions ...ain/src/main/java/com/facebook/presto/sql/planner/planPrinter/RowExpressionFormatter.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.block.Block;
import com.facebook.presto.common.type.Type;
import com.facebook.presto.common.type.VarbinaryType;
import com.facebook.presto.metadata.FunctionAndTypeManager;
import com.facebook.presto.spi.ConnectorSession;
import com.facebook.presto.spi.function.FunctionMetadata;
Expand Down Expand Up @@ -79,8 +80,25 @@ else if (standardFunctionResolution.isSubscriptFunction(node.getFunctionHandle()
return formatRowExpression(session, node.getArguments().get(0)) + "[" + formatRowExpression(session, node.getArguments().get(1)) + "]";
}
else if (standardFunctionResolution.isBetweenFunction(node.getFunctionHandle())) {
List<String> formattedExpresions = formatRowExpressions(session, node.getArguments());
return String.format("%s BETWEEN (%s) AND (%s)", formattedExpresions.get(0), formattedExpresions.get(1), formattedExpresions.get(2));
List<String> formattedExpressions = formatRowExpressions(session, node.getArguments());
return String.format("%s BETWEEN (%s) AND (%s)", formattedExpressions.get(0), formattedExpressions.get(1), formattedExpressions.get(2));
}
else if (standardFunctionResolution.isLikeFunction(node.getFunctionHandle())) {
RowExpression value = node.getArguments().get(0);
CallExpression patternCallExpression = (CallExpression) node.getArguments().get(1);

// second LIKE argument is:
// CAST(pattern as LikePattern), if escape is not present
// LIKE_PATTERN(pattern, escape), if escape is present
if (standardFunctionResolution.isCastFunction(patternCallExpression.getFunctionHandle())) {
RowExpression pattern = patternCallExpression.getArguments().get(0);
return String.format("%s LIKE %s", formatRowExpression(session, value), formatRowExpression(session, pattern));
}
else {
RowExpression pattern = patternCallExpression.getArguments().get(0);
RowExpression escape = patternCallExpression.getArguments().get(1);
return String.format("%s LIKE %s ESCAPE %s", formatRowExpression(session, value), formatRowExpression(session, pattern), formatRowExpression(session, escape));
}
}
FunctionMetadata metadata = functionMetadataManager.getFunctionMetadata(node.getFunctionHandle());
return node.getDisplayName() + (metadata.getVersion().hasVersion() ? ":" + metadata.getVersion() : "") + "(" + String.join(", ", formatRowExpressions(session, node.getArguments())) + ")";
Expand Down Expand Up @@ -123,12 +141,19 @@ public String visitConstant(ConstantExpression node, ConnectorSession session)
}

Type type = node.getType();
if (node.getType().getJavaType() == Block.class) {
if (type.getJavaType() == Block.class) {
Block block = (Block) value;
// TODO: format block
return format("[Block: position count: %s; size: %s bytes]", block.getPositionCount(), block.getRetainedSizeInBytes());
}
return type.getDisplayName().toUpperCase() + " " + value.toString();

String valueString = "'" + value.toString().replace("'", "''") + "'";

if (VarbinaryType.isVarbinaryType(type)) {
Copy link
Copy Markdown
Contributor

@rongrong rongrong Jun 28, 2021

Choose a reason for hiding this comment

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

You can get the type name generically with type.getTypeSignature().getBase()

Copy link
Copy Markdown
Contributor Author

@lightseba lightseba Jun 28, 2021

Choose a reason for hiding this comment

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

done

return "X" + valueString;
Copy link
Copy Markdown
Contributor

@rongrong rongrong Jun 28, 2021

Choose a reason for hiding this comment

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

Why is varbinary treated differently rather than using varbinary'xxx'?

Copy link
Copy Markdown
Contributor Author

@lightseba lightseba Jun 28, 2021

Choose a reason for hiding this comment

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

It seems like using TypeConstructor on varbinary takes the UTF-8 encoded version of the string as the binary literal, instead of reading it like a hex string. For example, when I run SELECT varbinary'abc123', x'abc123', varbinary'😎' on a local Presto server, I get

presto:sf1> SELECT varbinary'abc123', x'abc123', varbinary'😎';
       _col0       |  _col1   |    _col2    
-------------------+----------+-------------
 61 62 63 31 32 33 | ab c1 23 | f0 9f 98 8e

I'm not sure if this is intentional behavior or not, but since varbinary literals are output as hex strings, I needed this workaround.

Copy link
Copy Markdown
Contributor

@rongrong rongrong Jun 28, 2021

Choose a reason for hiding this comment

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

A better way of doing this might be to change the ConstantExpression.toString() rather than putting this complex logic here.

Copy link
Copy Markdown
Contributor Author

@lightseba lightseba Jun 28, 2021

Choose a reason for hiding this comment

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

I moved it into a function LiteralInterpreter.evaluateToString(), since turning the constant to a string requires the relevant ConnectorSession and LiteralInterpreter.evaluate() contains similar logic. Thoughts?

Copy link
Copy Markdown
Contributor

@rongrong rongrong Jun 28, 2021

Choose a reason for hiding this comment

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

Sorry I didn't pay close attention. If it's not possible to move this to ConstantExpression, let's keep the logic in RowExpressionFormatter. Thanks!

}

return type.getTypeSignature().getBase().toUpperCase() + valueString;
}
}
}
134 changes: 83 additions & 51 deletions presto-main/src/test/java/com/facebook/presto/sql/planner/TestRowExpressionFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import com.facebook.presto.spi.relation.SpecialFormExpression;
import com.facebook.presto.spi.relation.VariableReferenceExpression;
import com.facebook.presto.sql.planner.planPrinter.RowExpressionFormatter;
import com.facebook.presto.sql.relational.FunctionResolution;
import com.google.common.collect.ImmutableList;
import com.google.common.io.BaseEncoding;
import io.airlift.slice.Slice;
Expand Down Expand Up @@ -74,15 +75,19 @@
import static com.facebook.presto.type.ColorType.COLOR;
import static com.facebook.presto.type.IntervalDayTimeType.INTERVAL_DAY_TIME;
import static com.facebook.presto.type.IntervalYearMonthType.INTERVAL_YEAR_MONTH;
import static com.facebook.presto.type.LikePatternType.LIKE_PATTERN;
import static io.airlift.slice.Slices.utf8Slice;
import static java.lang.Float.floatToIntBits;
import static org.testng.Assert.assertEquals;

public class TestRowExpressionFormatter
{
private static final FunctionAndTypeManager FUNCTION_AND_TYPE_MANAGER = createTestFunctionAndTypeManager();
private static final FunctionResolution FUNCTION_RESOLUTION = new FunctionResolution(FUNCTION_AND_TYPE_MANAGER);
private static final RowExpressionFormatter FORMATTER = new RowExpressionFormatter(FUNCTION_AND_TYPE_MANAGER);
private static final VariableReferenceExpression C_BIGINT = new VariableReferenceExpression("c_bigint", BIGINT);
private static final VariableReferenceExpression C_BIGINT_ARRAY = new VariableReferenceExpression("c_bigint_array", new ArrayType(BIGINT));
private static final VariableReferenceExpression C_VARCHAR = new VariableReferenceExpression("c_varchar", VARCHAR);

@Test
public void testConstants()
Expand All @@ -93,66 +98,66 @@ public void testConstants()

// boolean
constantExpression = constant(true, BOOLEAN);
assertEquals(format(constantExpression), "BOOLEAN true");
assertEquals(format(constantExpression), "BOOLEAN'true'");

// double
constantExpression = constant(1.1, DOUBLE);
assertEquals(format(constantExpression), "DOUBLE 1.1");
assertEquals(format(constantExpression), "DOUBLE'1.1'");
constantExpression = constant(Double.NaN, DOUBLE);
assertEquals(format(constantExpression), "DOUBLE NaN");
assertEquals(format(constantExpression), "DOUBLE'NaN'");
constantExpression = constant(Double.POSITIVE_INFINITY, DOUBLE);
assertEquals(format(constantExpression), "DOUBLE Infinity");
assertEquals(format(constantExpression), "DOUBLE'Infinity'");

// real
constantExpression = constant((long) floatToIntBits(1.1f), REAL);
assertEquals(format(constantExpression), "REAL 1.1");
assertEquals(format(constantExpression), "REAL'1.1'");
constantExpression = constant((long) floatToIntBits(Float.NaN), REAL);
assertEquals(format(constantExpression), "REAL NaN");
assertEquals(format(constantExpression), "REAL'NaN'");
constantExpression = constant((long) floatToIntBits(Float.POSITIVE_INFINITY), REAL);
assertEquals(format(constantExpression), "REAL Infinity");
assertEquals(format(constantExpression), "REAL'Infinity'");

// string
constantExpression = constant(Slices.utf8Slice("abcde"), VARCHAR);
assertEquals(format(constantExpression), "VARCHAR abcde");
constantExpression = constant(Slices.utf8Slice("fgh"), createCharType(3));
assertEquals(format(constantExpression), "CHAR(3) fgh");
constantExpression = constant(utf8Slice("abcde"), VARCHAR);
assertEquals(format(constantExpression), "VARCHAR'abcde'");
constantExpression = constant(utf8Slice("fgh"), createCharType(3));
assertEquals(format(constantExpression), "CHAR'fgh'");

// integer
constantExpression = constant(1L, TINYINT);
assertEquals(format(constantExpression), "TINYINT 1");
assertEquals(format(constantExpression), "TINYINT'1'");
constantExpression = constant(1L, SMALLINT);
assertEquals(format(constantExpression), "SMALLINT 1");
assertEquals(format(constantExpression), "SMALLINT'1'");
constantExpression = constant(1L, INTEGER);
assertEquals(format(constantExpression), "INTEGER 1");
assertEquals(format(constantExpression), "INTEGER'1'");
constantExpression = constant(1L, BIGINT);
assertEquals(format(constantExpression), "BIGINT 1");
assertEquals(format(constantExpression), "BIGINT'1'");

// varbinary
Slice value = Slices.wrappedBuffer(BaseEncoding.base16().decode("123456"));
Slice value = Slices.wrappedBuffer(BaseEncoding.base16().decode("123456AB"));
constantExpression = constant(value, VARBINARY);
assertEquals(format(constantExpression), "VARBINARY 12 34 56");
assertEquals(format(constantExpression), "X'12 34 56 ab'");

// color
constantExpression = constant(256L, COLOR);
assertEquals(format(constantExpression), "COLOR 256");
assertEquals(format(constantExpression), "COLOR'256'");

// long and short decimals
constantExpression = constant(decimal("1.2345678910"), DecimalType.createDecimalType(11, 10));
assertEquals(format(constantExpression), "DECIMAL(11,10) 1.2345678910");
assertEquals(format(constantExpression), "DECIMAL'1.2345678910'");
constantExpression = constant(decimal("1.281734081274028174012432412423134"), DecimalType.createDecimalType(34, 33));
assertEquals(format(constantExpression), "DECIMAL(34,33) 1.281734081274028174012432412423134");
assertEquals(format(constantExpression), "DECIMAL'1.281734081274028174012432412423134'");

// time
constantExpression = constant(662727600000L, TIMESTAMP);
assertEquals(format(constantExpression), "TIMESTAMP 1991-01-01 00:00:00.000");
assertEquals(format(constantExpression), "TIMESTAMP'1991-01-01 00:00:00.000'");
constantExpression = constant(7670L, DATE);
assertEquals(format(constantExpression), "DATE 1991-01-01");
assertEquals(format(constantExpression), "DATE'1991-01-01'");

// interval
constantExpression = constant(24L, INTERVAL_DAY_TIME);
assertEquals(format(constantExpression), "INTERVAL DAY TO SECOND 0 00:00:00.024");
assertEquals(format(constantExpression), "INTERVAL DAY TO SECOND'0 00:00:00.024'");
constantExpression = constant(25L, INTERVAL_YEAR_MONTH);
assertEquals(format(constantExpression), "INTERVAL YEAR TO MONTH 2-1");
assertEquals(format(constantExpression), "INTERVAL YEAR TO MONTH'2-1'");

// block
constantExpression = constant(new LongArrayBlockBuilder(null, 4).writeLong(1L).writeLong(2).build(), new ArrayType(BIGINT));
Expand All @@ -166,31 +171,31 @@ public void testCalls()

// arithmetic
callExpression = createCallExpression(ADD);
assertEquals(format(callExpression), "(c_bigint) + (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) + (BIGINT'5')");
callExpression = createCallExpression(SUBTRACT);
assertEquals(format(callExpression), "(c_bigint) - (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) - (BIGINT'5')");
callExpression = createCallExpression(MULTIPLY);
assertEquals(format(callExpression), "(c_bigint) * (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) * (BIGINT'5')");
callExpression = createCallExpression(DIVIDE);
assertEquals(format(callExpression), "(c_bigint) / (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) / (BIGINT'5')");
callExpression = createCallExpression(MODULUS);
assertEquals(format(callExpression), "(c_bigint) % (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) % (BIGINT'5')");

// comparison
callExpression = createCallExpression(GREATER_THAN);
assertEquals(format(callExpression), "(c_bigint) > (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) > (BIGINT'5')");
callExpression = createCallExpression(LESS_THAN);
assertEquals(format(callExpression), "(c_bigint) < (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) < (BIGINT'5')");
callExpression = createCallExpression(GREATER_THAN_OR_EQUAL);
assertEquals(format(callExpression), "(c_bigint) >= (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) >= (BIGINT'5')");
callExpression = createCallExpression(LESS_THAN_OR_EQUAL);
assertEquals(format(callExpression), "(c_bigint) <= (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) <= (BIGINT'5')");
callExpression = createCallExpression(EQUAL);
assertEquals(format(callExpression), "(c_bigint) = (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) = (BIGINT'5')");
callExpression = createCallExpression(NOT_EQUAL);
assertEquals(format(callExpression), "(c_bigint) <> (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) <> (BIGINT'5')");
callExpression = createCallExpression(IS_DISTINCT_FROM);
assertEquals(format(callExpression), "(c_bigint) IS DISTINCT FROM (BIGINT 5)");
assertEquals(format(callExpression), "(c_bigint) IS DISTINCT FROM (BIGINT'5')");

// negation
RowExpression expression = createCallExpression(ADD);
Expand All @@ -199,7 +204,7 @@ public void testCalls()
FUNCTION_AND_TYPE_MANAGER.resolveOperator(NEGATION, fromTypes(expression.getType())),
expression.getType(),
expression);
assertEquals(format(callExpression), "-((c_bigint) + (BIGINT 5))");
assertEquals(format(callExpression), "-((c_bigint) + (BIGINT'5'))");

// subscript
ArrayType arrayType = (ArrayType) C_BIGINT_ARRAY.getType();
Expand All @@ -209,15 +214,15 @@ public void testCalls()
elementType,
ImmutableList.of(C_BIGINT_ARRAY, constant(0L, INTEGER)));
callExpression = subscriptExpression;
assertEquals(format(callExpression), "c_bigint_array[INTEGER 0]");
assertEquals(format(callExpression), "c_bigint_array[INTEGER'0']");

// cast
callExpression = call(
CAST.name(),
FUNCTION_AND_TYPE_MANAGER.lookupCast(CastType.CAST, TINYINT.getTypeSignature(), BIGINT.getTypeSignature()),
BIGINT,
constant(1L, TINYINT));
assertEquals(format(callExpression), "CAST(TINYINT 1 AS bigint)");
CAST.name(),
FUNCTION_AND_TYPE_MANAGER.lookupCast(CastType.CAST, TINYINT.getTypeSignature(), BIGINT.getTypeSignature()),
BIGINT,
constant(1L, TINYINT));
assertEquals(format(callExpression), "CAST(TINYINT'1' AS bigint)");

// between
callExpression = call(
Expand All @@ -227,15 +232,42 @@ public void testCalls()
subscriptExpression,
constant(1L, BIGINT),
constant(5L, BIGINT));
assertEquals(format(callExpression), "c_bigint_array[INTEGER 0] BETWEEN (BIGINT 1) AND (BIGINT 5)");
assertEquals(format(callExpression), "c_bigint_array[INTEGER'0'] BETWEEN (BIGINT'1') AND (BIGINT'5')");

// other
callExpression = call(
HASH_CODE.name(),
FUNCTION_AND_TYPE_MANAGER.resolveOperator(HASH_CODE, fromTypes(BIGINT)),
BIGINT,
constant(1L, BIGINT));
assertEquals(format(callExpression), "HASH_CODE(BIGINT 1)");
assertEquals(format(callExpression), "HASH_CODE(BIGINT'1')");

// like
callExpression = call(
"LIKE",
FUNCTION_RESOLUTION.likeVarcharFunction(),
BOOLEAN,
C_VARCHAR,
call(
CAST.name(),
FUNCTION_AND_TYPE_MANAGER.lookupCast(CastType.CAST, VARCHAR.getTypeSignature(), LIKE_PATTERN.getTypeSignature()),
LIKE_PATTERN,
constant(utf8Slice("prefix%"), VARCHAR)));
assertEquals(format(callExpression), "c_varchar LIKE VARCHAR'prefix%'");

// like escape
callExpression = call(
"LIKE",
FUNCTION_RESOLUTION.likeVarcharFunction(),
BOOLEAN,
C_VARCHAR,
call(
"LIKE_PATTERN",
FUNCTION_RESOLUTION.likePatternFunction(),
LIKE_PATTERN,
constant(utf8Slice("%escaped$_"), VARCHAR),
constant(utf8Slice("$"), VARCHAR)));
assertEquals(format(callExpression), "c_varchar LIKE VARCHAR'%escaped$_' ESCAPE VARCHAR'$'");
}

@Test
Expand All @@ -245,13 +277,13 @@ public void testSpecialForm()

// or and and
specialFormExpression = new SpecialFormExpression(OR, BOOLEAN, createCallExpression(NOT_EQUAL), createCallExpression(IS_DISTINCT_FROM));
assertEquals(format(specialFormExpression), "((c_bigint) <> (BIGINT 5)) OR ((c_bigint) IS DISTINCT FROM (BIGINT 5))");
assertEquals(format(specialFormExpression), "((c_bigint) <> (BIGINT'5')) OR ((c_bigint) IS DISTINCT FROM (BIGINT'5'))");
specialFormExpression = new SpecialFormExpression(AND, BOOLEAN, createCallExpression(EQUAL), createCallExpression(GREATER_THAN));
assertEquals(format(specialFormExpression), "((c_bigint) = (BIGINT 5)) AND ((c_bigint) > (BIGINT 5))");
assertEquals(format(specialFormExpression), "((c_bigint) = (BIGINT'5')) AND ((c_bigint) > (BIGINT'5'))");

// other
specialFormExpression = new SpecialFormExpression(IS_NULL, BOOLEAN, createCallExpression(ADD));
assertEquals(format(specialFormExpression), "IS_NULL((c_bigint) + (BIGINT 5))");
assertEquals(format(specialFormExpression), "IS_NULL((c_bigint) + (BIGINT'5'))");
}

@Test
Expand All @@ -266,7 +298,7 @@ public void testComplex()
BIGINT,
C_BIGINT,
expression);
assertEquals(format(complexExpression), "(c_bigint) - ((c_bigint) + (BIGINT 5))");
assertEquals(format(complexExpression), "(c_bigint) - ((c_bigint) + (BIGINT'5'))");

RowExpression expression1 = createCallExpression(ADD);
RowExpression expression2 = call(
Expand All @@ -277,7 +309,7 @@ public void testComplex()
C_BIGINT);
RowExpression expression3 = createCallExpression(GREATER_THAN);
complexExpression = new SpecialFormExpression(OR, BOOLEAN, expression2, expression3);
assertEquals(format(complexExpression), "(((c_bigint) + (BIGINT 5)) * (c_bigint)) OR ((c_bigint) > (BIGINT 5))");
assertEquals(format(complexExpression), "(((c_bigint) + (BIGINT'5')) * (c_bigint)) OR ((c_bigint) > (BIGINT'5'))");

ArrayType arrayType = (ArrayType) C_BIGINT_ARRAY.getType();
Type elementType = arrayType.getElementType();
Expand All @@ -296,7 +328,7 @@ public void testComplex()
BIGINT,
expression2,
constant(5L, BIGINT));
assertEquals(format(expression3), "(-(c_bigint_array[INTEGER 5])) + (BIGINT 5)");
assertEquals(format(expression3), "(-(c_bigint_array[INTEGER'5'])) + (BIGINT'5')");
}

protected static Object decimal(String decimalString)
Expand Down
2 changes: 2 additions & 0 deletions presto-spi/src/main/java/com/facebook/presto/spi/function/StandardFunctionResolution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ public interface StandardFunctionResolution

FunctionHandle likeCharFunction(Type valueType);

boolean isLikeFunction(FunctionHandle functionHandle);

FunctionHandle arrayConstructor(List<? extends Type> argumentTypes);

FunctionHandle arithmeticFunction(OperatorType operator, Type leftType, Type rightType);
Expand Down