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.

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

Jump to bottom

Show original characters for string literals instead of escaping #675

Merged
merged 2 commits into from
Jul 18, 2024
Merged

Show original characters for string literals instead of escaping #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
504d9d8
Show original characters for string literals instead of escaping
grieve54706 Jul 17, 2024
8f87958
Make TestWrenWithDuckDBTableFunction be single threaded to avoid conc…
grieve54706 Jul 18, 2024
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
64 changes: 5 additions & 59 deletions trino-parser/src/main/java/io/trino/sql/ExpressionFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
package io.trino.sql;

import com.google.common.base.CharMatcher;
import com.google.common.base.Joiner;
import com.google.common.collect.ImmutableList;
import io.trino.sql.SqlFormatter.Dialect;
Expand Down Expand Up @@ -92,7 +91,6 @@
import java.util.ArrayList;
import java.util.List;
import java.util.Locale;
import java.util.PrimitiveIterator;
import java.util.function.Function;
import java.util.stream.Collectors;

Expand Down Expand Up @@ -237,13 +235,13 @@ protected String visitBooleanLiteral(BooleanLiteral node, Void context)
@Override
protected String visitStringLiteral(StringLiteral node, Void context)
{
return formatStringLiteral(node.getValue(), dialect);
return formatStringLiteral(node.getValue());
}

@Override
protected String visitCharLiteral(CharLiteral node, Void context)
{
return "CHAR " + formatStringLiteral(node.getValue(), dialect);
return "CHAR " + formatStringLiteral(node.getValue());
}

@Override
Expand Down Expand Up @@ -327,7 +325,7 @@ protected String visitDecimalLiteral(DecimalLiteral node, Void context)
@Override
protected String visitGenericLiteral(GenericLiteral node, Void context)
{
return node.getType() + " " + formatStringLiteral(node.getValue(), dialect);
return node.getType() + " " + formatStringLiteral(node.getValue());
}

@Override
Expand Down Expand Up @@ -998,61 +996,9 @@ private String processDateDiffInBigQuery(FunctionCall node, Void context)
}
}

static String formatStringLiteral(String s, Dialect dialect)
static String formatStringLiteral(String s)
{
if (dialect == BIGQUERY) {
s = s.replace("'", "\\'");
}
else {
s = s.replace("'", "''");
}
if (CharMatcher.inRange((char) 0x20, (char) 0x7E).matchesAllOf(s)) {
return "'" + s + "'";
}

StringBuilder builder = new StringBuilder();
if (dialect == DEFAULT || dialect == DUCKDB || dialect == POSTGRES) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw there're some DuckDB-specific behaviors. Do they work well if we remove the dialect?

Copy link
Contributor Author

@grieve54706 grieve54706 Jul 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Don't worry. This condition has a test called testDecimalRewrite to cover it by #261. The test is successful after this adjustment.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds great.

builder.append("U&");
}
builder.append("'");
PrimitiveIterator.OfInt iterator = s.codePoints().iterator();
while (iterator.hasNext()) {
int codePoint = iterator.nextInt();
checkArgument(codePoint >= 0, "Invalid UTF-8 encoding in characters: %s", s);
if (isAsciiPrintable(codePoint)) {
char ch = (char) codePoint;
if (ch == '\\') {
builder.append(ch);
}
builder.append(ch);
}
else {
if (dialect == DEFAULT || dialect == DUCKDB || dialect == POSTGRES) {
if (codePoint <= 0xFFFF) {
builder.append('\\');
builder.append(format("%04X", codePoint));
}
else {
builder.append("\\+");
builder.append(format("%06X", codePoint));
}
}
else if (dialect == BIGQUERY) {
if (codePoint <= 0xFFFF) {
builder.append('\\');
builder.append('u');
builder.append(format("%04X", codePoint));
}
else {
builder.append("\\");
builder.append('U');
builder.append(format("%08X", codePoint));
}
}
}
}
builder.append("'");
return builder.toString();
return "'" + s.replace("'", "''") + "'";
}

public static String formatOrderBy(OrderBy orderBy, Dialect dialect)
Expand Down
36 changes: 18 additions & 18 deletions trino-parser/src/main/java/io/trino/sql/SqlFormatter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -892,7 +892,7 @@ protected Void visitCreateView(CreateView node, Integer indent)

node.getComment().ifPresent(comment -> builder
.append(" COMMENT ")
.append(formatStringLiteral(comment, dialect)));
.append(formatStringLiteral(comment)));

node.getSecurity().ifPresent(security -> builder
.append(" SECURITY ")
Expand Down Expand Up @@ -957,7 +957,7 @@ protected Void visitCreateMaterializedView(CreateMaterializedView node, Integer
builder.append(formatName(node.getName(), dialect));
node.getComment().ifPresent(comment -> builder
.append("\nCOMMENT ")
.append(formatStringLiteral(comment, dialect)));
.append(formatStringLiteral(comment)));
builder.append(formatPropertiesMultiLine(node.getProperties()));
builder.append(" AS\n");

Expand Down Expand Up @@ -1051,11 +1051,11 @@ protected Void visitShowCatalogs(ShowCatalogs node, Integer indent)

node.getLikePattern().ifPresent(value -> builder
.append(" LIKE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

node.getEscape().ifPresent(value -> builder
.append(" ESCAPE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

return null;
}
Expand All @@ -1071,11 +1071,11 @@ protected Void visitShowSchemas(ShowSchemas node, Integer indent)

node.getLikePattern().ifPresent(value -> builder
.append(" LIKE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

node.getEscape().ifPresent(value -> builder
.append(" ESCAPE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

return null;
}
Expand All @@ -1091,11 +1091,11 @@ protected Void visitShowTables(ShowTables node, Integer indent)

node.getLikePattern().ifPresent(value -> builder
.append(" LIKE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

node.getEscape().ifPresent(value -> builder
.append(" ESCAPE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

return null;
}
Expand Down Expand Up @@ -1126,11 +1126,11 @@ protected Void visitShowColumns(ShowColumns node, Integer indent)

node.getLikePattern().ifPresent(value -> builder
.append(" LIKE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

node.getEscape().ifPresent(value -> builder
.append(" ESCAPE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

return null;
}
Expand All @@ -1151,11 +1151,11 @@ protected Void visitShowFunctions(ShowFunctions node, Integer indent)

node.getLikePattern().ifPresent(value -> builder
.append(" LIKE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

node.getEscape().ifPresent(value -> builder
.append(" ESCAPE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

return null;
}
Expand All @@ -1167,11 +1167,11 @@ protected Void visitShowSession(ShowSession node, Integer indent)

node.getLikePattern().ifPresent(value -> builder
.append(" LIKE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

node.getEscape().ifPresent(value -> builder
.append(" ESCAPE ")
.append(formatStringLiteral(value, dialect)));
.append(formatStringLiteral(value)));

return null;
}
Expand Down Expand Up @@ -1259,7 +1259,7 @@ protected Void visitCreateTableAsSelect(CreateTableAsSelect node, Integer indent

node.getComment().ifPresent(comment -> builder
.append("\nCOMMENT ")
.append(formatStringLiteral(comment, dialect)));
.append(formatStringLiteral(comment)));
builder.append(formatPropertiesMultiLine(node.getProperties()));

builder.append(" AS ");
Expand Down Expand Up @@ -1310,7 +1310,7 @@ protected Void visitCreateTable(CreateTable node, Integer indent)

node.getComment().ifPresent(comment -> builder
.append("\nCOMMENT ")
.append(formatStringLiteral(comment, dialect)));
.append(formatStringLiteral(comment)));

builder.append(formatPropertiesMultiLine(node.getProperties()));

Expand Down Expand Up @@ -1351,7 +1351,7 @@ private String formatColumnDefinition(ColumnDefinition column)
}
column.getComment().ifPresent(comment -> builder
.append(" COMMENT ")
.append(formatStringLiteral(comment, dialect)));
.append(formatStringLiteral(comment)));
builder.append(formatPropertiesSingleLine(column.getProperties()));
return builder.toString();
}
Expand Down Expand Up @@ -1442,7 +1442,7 @@ private String joinProperties(List<Property> properties)
protected Void visitComment(Comment node, Integer context)
{
String comment = node.getComment()
.map(str -> formatStringLiteral(str, dialect))
.map(ExpressionFormatter::formatStringLiteral)
.orElse("NULL");

switch (node.getType()) {
Expand Down
11 changes: 7 additions & 4 deletions trino-parser/src/test/java/io/trino/sql/parser/TestStatementBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -331,11 +331,14 @@ public void testStatementBuilder()
public void testStringFormatter()
{
assertSqlFormatter("U&'hello\\6d4B\\8Bd5\\+10FFFFworld\\7F16\\7801'",
"U&'hello\\6D4B\\8BD5\\+10FFFFworld\\7F16\\7801'");
"'hello测试\uDBFF\uDFFFworld编码'");
assertSqlFormatter("'hello world'", "'hello world'");
assertSqlFormatter("U&'!+10FFFF!6d4B!8Bd5ABC!6d4B!8Bd5' UESCAPE '!'", "U&'\\+10FFFF\\6D4B\\8BD5ABC\\6D4B\\8BD5'");
assertSqlFormatter("U&'\\+10FFFF\\6D4B\\8BD5\\0041\\0042\\0043\\6D4B\\8BD5'", "U&'\\+10FFFF\\6D4B\\8BD5ABC\\6D4B\\8BD5'");
assertSqlFormatter("U&'\\\\abc\\6D4B'''", "U&'\\\\abc\\6D4B'''");
assertSqlFormatter("U&'!+10FFFF!6d4B!8Bd5ABC!6d4B!8Bd5' UESCAPE '!'", "'\uDBFF\uDFFF测试ABC测试'");
assertSqlFormatter("U&'\\+10FFFF\\6D4B\\8BD5\\0041\\0042\\0043\\6D4B\\8BD5'", "'\uDBFF\uDFFF测试ABC测试'");
assertSqlFormatter("U&'\\\\abc\\6D4B'''", "'\\abc测'''");
assertSqlFormatter("'攻殻機動隊'", "'攻殻機動隊'");
assertSqlFormatter("'😂'", "'😂'");
assertSqlFormatter("'시험'", "'시험'");
Comment on lines +336 to +341
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

}

@Test
Expand Down
1 change: 1 addition & 0 deletions wren-tests/src/test/java/io/wren/testing/duckdb/TestWrenWithDuckDBTableFunction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;

@Test(singleThreaded = true)
Copy link
Contributor

Choose a reason for hiding this comment

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

Note: Reloading DuckDB would be triggered by other test cases (set init-sql). Run tests sequentially to avoid testing failure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another test class, TestDuckDBResource, which uses setDuckDBInitSQL, is already set to singleThreaded. They don't affect each other. We don't need to make any adjustments now. If we encounter any issues in the future, we can then modify the test suite as necessary.

public class TestWrenWithDuckDBTableFunction
extends RequireWrenServer
{
Expand Down
Loading