Support Mixed Case Pinot Tables#7630
Conversation
|
TODO: adding tests. |
elonazoulay
changed the title
elonazoulay
force-pushed
the
mixed_case
branch
3 times, most recently
from
7bd7758 to
393735f
Compare
elonazoulay
changed the title
elonazoulay
force-pushed
the
mixed_case
branch
7 times, most recently
from
3e65426 to
dd1789b
Compare
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
Some initial comments.
But I have concerns about the implementation so I didn't review the entire PR. The current implementation ties the remote name cache to the metadata cache and doesn't provide a way to disable the case-insensitive matching which will lead to failures when a Pinot catalog contains colliding table names without any solution other than renaming the offending tables (which may not always be possible).
I'd recommend something similar to how it's implemented in trino-base-jdbc - look at BaseJdbcClient#toRemoteTableName usages.
There's a separate cache that maps Trino table names (i.e. always lowercase) to the actual name in the remote system. This cache is used or not depending on a config so that the feature can be disabled.
There was a problem hiding this comment.
Can we have a test detect this when this gets fixed on Pinot side? e.g. asserting that incorrect results are returned for filter on a varbinary columns with an actually empty array and a null value?
There was a problem hiding this comment.
nit: It might be helpful to use a name like toRemoteTableName and toRemoteSchemaName since many other connectors use those method names and it'll make it easier for readers to figure out what this is trying to do.
I don't have a strong opinion on this though.
There was a problem hiding this comment.
Won't this silently swallow colliding names?
There was a problem hiding this comment.
Yes, I will push an update shortly that returns colliding table names. It is based off of #9098.
elonazoulay
force-pushed
the
mixed_case
branch
from
dd1789b to
75575c6
Compare
elonazoulay
force-pushed
the
mixed_case
branch
from
75575c6 to
79dc0b5
Compare
|
Rebased off of #9781 . |
elonazoulay
force-pushed
the
mixed_case
branch
5 times, most recently
from
4edf88a to
6180aa9
Compare
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
Looked at last commit since others were merged in #9781.
Is it possible to make this controlled via a toggle? That way once quoted-identifiers are supported it'll be much easier to migrate to that instead of having to hunt the code for places which would need changing.
Also, some named abstraction for Pinot name would be helpful to indicate which places in code expect Trino names vs Pinot names. This became very confusing in Jdbc based connectors in the past.
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/PinotMetadata.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Declare as Multimap even if impl is ListMultimap.
plugin/trino-pinot/src/main/java/io/trino/plugin/pinot/client/PinotClient.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/AbstractPinotIntegrationSmokeTest.java
Outdated
Show resolved
Hide resolved
plugin/trino-pinot/src/test/java/io/trino/plugin/pinot/MockPinotClient.java
Outdated
Show resolved
Hide resolved
hashhar
left a comment
hashhar
left a comment
There was a problem hiding this comment.
Thanks. I'll merge once CI is done.
2 participants
Extracted from the pinot insert pr.
Fixes #6789