Fix BigQuery tests for column names with special characters #16613
Conversation
ssheikin
force-pushed
the
ssheikin/29/oss/bigquery_column_names
branch
from
e044586 to
a20896d
Compare
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java
Outdated
Show resolved
Hide resolved
| @@ -13,6 +13,7 @@ | |||
| */ | |||
There was a problem hiding this comment.
I am a bit lost. Are we testing bigquery here? @ebyhr can you please let me know what is the point of test based on incorrect column names?
As I understand we should only update filterColumnNameTestData
There was a problem hiding this comment.
Testing unsupported column names helps us find SQL injection vector.
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/BaseBigQueryConnectorTest.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| protected Optional<String> filterColumnNameTestData(String columnName) | ||
| { | ||
| if (NOT_ALLOWED_ANYMORE_COLUMN_NAMES.contains(columnName)) { |
There was a problem hiding this comment.
Creating and inserting into tables with these columns works and selecting the table isn't problem, right? This change hides the fact and we can't understand what happened after the operations. It would be better to deny such columns in all operations, or verify the result with different way, e.g. use query based API instead of storage API.
| @@ -13,6 +13,7 @@ | |||
| */ | |||
There was a problem hiding this comment.
Testing unsupported column names helps us find SQL injection vector.
ssheikin
force-pushed
the
ssheikin/29/oss/bigquery_column_names
branch
4 times, most recently
from
58c7ac8 to
5d17c67
Compare
|
@ebyhr hope I've addressed your comments. |
|
|
||
| // these column names started to fail because of BigQuery internal change ~ between 18:00 16 of March 2023 GMT and 00:00 17 of March 2023 GMT | ||
| // it's still possible to CREATE TABLE with such column names, and even INSERT some data, but it is not possible to SELECT from it. | ||
| // TODO is it possible to improve trino to be able to SELECT from such columns again? |
There was a problem hiding this comment.
That is a good question. But maybe we should prevent creating columns with names containing the "prohibited" characters to avoid this gotcha?
There was a problem hiding this comment.
Let's handle in follow-up #16664. We would like to fix CI failure first.
plugin/trino-bigquery/src/test/java/io/trino/plugin/bigquery/TestBigQueryAvroConnectorTest.java
Outdated
Show resolved
Hide resolved
Some column names started to fail because of BigQuery internal change ~ between 18:00 16 of March 2023 GMT and 00:00 17 of March 2023 GMT, first noticed when 90d6f07 merged to master.
ebyhr
force-pushed
the
ssheikin/29/oss/bigquery_column_names
branch
from
5d17c67 to
dd467dd
Compare
|
@ebyhr Thank you for improving and merging |
Some column names started to fail because of BigQuery internal change
~ between
18:00 16 of March 2023 GMT and
00:00 17 of March 2023 GMT,
first noticed when 90d6f07 merged to
master.
Description
Fixes #16603
Release notes
(x) This is not user-visible or docs only and no release notes are required.