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

Added a fix for BQ connector to correctly treat single quote in query criteria #7869

Merged
merged 1 commit into from
May 17, 2021
Merged

Added a fix for BQ connector to correctly treat single quote in query criteria #7869

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 2 additions & 1 deletion plugin/trino-bigquery/src/main/java/io/trino/plugin/bigquery/BigQueryType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,8 @@ private static ZonedDateTime toZonedDateTime(long millisUtc, ZoneId zoneId)
static String stringToStringConverter(Object value)
{
Slice slice = (Slice) value;
return quote(slice.toStringUtf8());
// TODO (https://github.com/trinodb/trino/issues/7900) Add support for all String and Bytes literals
return quote(slice.toStringUtf8().replace("'", "\\'"));
Copy link
Member

Choose a reason for hiding this comment

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

It'd be nice to handle all possible characters that need escaping.

If possible can you also add support for the ones listed at https://cloud.google.com/bigquery/docs/reference/standard-sql/lexical#string_and_bytes_literals along with test-cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, let me take a look and figure out what does it actually take to add escaping for all possible characters.
If it is too much effort, I'll probably take it up as a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hashhar Adding support for other literals doesn't look straightforward. Is it fine if we go ahead with this one and I'll do some more research and raise a separate issue for other characters?

Copy link
Member

Choose a reason for hiding this comment

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

Please create an issue with your findings and add a TODO comment here so that we don't forget.

cc: @ebyhr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I am on it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created an issue for adding support for all characters #7900.
Added a TODO comment.

}

static String numericToStringConverter(Object value)
Expand Down
15 changes: 15 additions & 0 deletions ...ino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryIntegrationSmokeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,21 @@ public void testSelectFromYearlyPartitionedTable()
assertEquals((long) actualValues.getOnlyValue(), 1L);
}

@Test(description = "regression test for https://github.com/trinodb/trino/issues/7784")
public void testSelectWithSingleQuoteInWhereClause()
{
String tableName = "test.select_with_single_quote";

onBigQuery("DROP TABLE IF EXISTS " + tableName);
onBigQuery("CREATE TABLE " + tableName + "(col INT64, val STRING)");
onBigQuery("INSERT INTO " + tableName + " VALUES (1,'escape\\'single quote')");

MaterializedResult actualValues = computeActual("SELECT val FROM " + tableName + " WHERE val = 'escape''single quote'");

assertEquals(actualValues.getRowCount(), 1);
assertEquals(actualValues.getOnlyValue(), "escape'single quote");
}

@Test(description = "regression test for https://github.com/trinodb/trino/issues/5618")
public void testPredicatePushdownPrunnedColumns()
{
Expand Down
4 changes: 4 additions & 0 deletions plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,10 @@ public void testStringToStringConverter()
assertThat(BigQueryType.stringToStringConverter(
utf8Slice("test")))
.isEqualTo("'test'");

assertThat(BigQueryType.stringToStringConverter(
utf8Slice("test's test")))
.isEqualTo("'test\\'s test'");
}

@Test
Expand Down