Fix bug when querying system.jdbc.tables #4965
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
findepi
reviewed
Aug 25, 2020
| @@ -36,7 +38,7 @@ private FilterUtil() {} | |||
| } | |||
|
|
|||
| Object value = domain.getSingleValue(); | |||
| return Optional.of(((Slice) value).toStringUtf8()); | |||
| return Optional.of(((Slice) value).toStringUtf8().toLowerCase(ENGLISH)); | |||
There was a problem hiding this comment.
This is not correct. This method is not dedicated to table/column names, so should not lower case.
Even if it was dedicated, lowercasing would be misleading. (a non-lowercase expected value never matches, when lowercased will)
There was a problem hiding this comment.
You are faster than GHA.
Fixed - moved lowercasing to a different place.
skrzypo987
force-pushed
the
bugfix-upper-case-list-tables-filter
branch
from
e527598
to
72db624
Compare
kokosing
reviewed
Aug 25, 2020
presto-tests/src/test/java/io/prestosql/tests/TestMetadataManager.java
Outdated
Show resolved
Hide resolved
kokosing
reviewed
Aug 25, 2020
| @@ -81,8 +82,8 @@ public RecordCursor cursor(ConnectorTransactionHandle transactionHandle, Connect | |||
| { | |||
| Session session = ((FullConnectorSession) connectorSession).getSession(); | |||
| Optional<String> catalogFilter = tryGetSingleVarcharValue(constraint, 0); | |||
| Optional<String> schemaFilter = tryGetSingleVarcharValue(constraint, 1); | |||
| Optional<String> tableFilter = tryGetSingleVarcharValue(constraint, 2); | |||
| Optional<String> schemaFilter = tryGetSingleVarcharValue(constraint, 1).map(filter -> filter.toLowerCase(ENGLISH)); | |||
There was a problem hiding this comment.
Can we instead fix the check in the other place to allow non lower cased values? This seems invalid to me to manipulate values given in sql literals.
skrzypo987
force-pushed
the
bugfix-upper-case-list-tables-filter
branch
from
72db624
to
a9f6f79
Compare
findepi
reviewed
Aug 26, 2020
| Builder table = InMemoryRecordSet.builder(METADATA); | ||
| if (!isLowerCase(schemaFilter) || !isLowerCase(tableFilter)) { | ||
| // Uppercase predicate will never match a lowercase name | ||
| return table.build().cursor(); |
There was a problem hiding this comment.
This is something to be removed in #17
If we place the logic closer to where it's needed (eg when QualifiedTablePrefix is constructed), then
- it would be more obvious why the check matters
- it would be easier to track it down and remove when doing above issue
kokosing
approved these changes
Aug 26, 2020
|
Please check automation failures. |
kokosing
reviewed
Aug 26, 2020
| { | ||
| assertThat(queryRunner.execute("SELECT * FROM system.jdbc.tables WHERE TABLE_SCHEM = 'upper_case_schema' AND TABLE_NAME = 'upper_case_table'")) | ||
| .hasSize(1); | ||
| assertThat(queryRunner.execute("SELECT * FROM system.jdbc.tables WHERE TABLE_SCHEM = 'UPPER_CASE_SCHEMA'")) |
skrzypo987
force-pushed
the
bugfix-upper-case-list-tables-filter
branch
from
a9f6f79
to
2c1737f
Compare
Making where constraints upper-case caused exception
skrzypo987
force-pushed
the
bugfix-upper-case-list-tables-filter
branch
from
2c1737f
to
d921455
Compare
findepi
approved these changes
Sep 2, 2020
presto-main/src/main/java/io/prestosql/connector/system/jdbc/TableJdbcTable.java
Outdated
Show resolved
Hide resolved
presto-main/src/main/java/io/prestosql/connector/system/jdbc/TableJdbcTable.java
Outdated
Show resolved
Hide resolved
presto-tests/src/test/java/io/prestosql/tests/TestMetadataManager.java
Outdated
Show resolved
Hide resolved
…ger.java Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
…ger.java Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
…ableJdbcTable.java Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
…ableJdbcTable.java Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
…ger.java Co-authored-by: Piotr Findeisen <piotr.findeisen@gmail.com>
|
Merged as: c336e9a |
|
Thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Making where constraints upper-case caused exception