https://linen.dev logo
m

Mason Wheeler

05/20/2021, 7:39 PM
I just found that
JdbcBufferedConsumerFactory.toWriteConfig
has access to a
NamingConventionTransformer
instance, and
NamingConventionTransformer
has methods for generating table names, but it's not making use of them. (See lines 109-110.) Instead, it's unilaterally imposing a specific naming convention that doesn't work with all databases. Is there any rationale for this?
u

user

05/20/2021, 8:29 PM
looking now
u

user

05/20/2021, 8:29 PM
line 109 is this
u

user

05/20/2021, 8:29 PM
Copy code
final String tableName = Names.concatQuotedNames("_airbyte_raw_", namingResolver.getIdentifier(streamName));
u

user

05/20/2021, 8:30 PM
which looks like it is calling the naming transformer, no?
u

user

05/20/2021, 8:30 PM
There are methods on the name transformer specifically for creating database table names. It's not using them; it's calling
getIdentifier
and then doing its own thing with the results.
u

user

05/20/2021, 8:31 PM
And the things it's doing turn out to be incompatible with Oracle.
u

user

05/20/2021, 8:31 PM
got it.
u

user

05/20/2021, 8:31 PM
so i think the table name ones are deprecated.
u

user

05/20/2021, 8:32 PM
Well, can we un-deprecate them?
u

user

05/20/2021, 8:32 PM
what's your goal here?
u

user

05/20/2021, 8:32 PM
are they producing valid names and getidentifier isn't?
u

user

05/20/2021, 8:32 PM
looking at the implementation of those methods it looks like they do pretty much the same thing as
getIdentifier
u

user

05/20/2021, 8:32 PM
...unless you override them
u

user

05/20/2021, 8:33 PM
Look at all the stuff it's doing after calling
getIdentifier
u

user

05/20/2021, 8:33 PM
1. In Oracle, it's not valid to start a table name with an underscore. 2. Table names have a strict 30 character length limit.
u

user

05/20/2021, 8:33 PM
The current system is breaking both of those. (And possibly other things once I get past the first two.)
u

user

05/20/2021, 8:33 PM
ah.
u

user

05/20/2021, 8:34 PM
okay. so issue is that on line 109 we are prepending the table name with something that's not valid for oracle.
u

user

05/20/2021, 8:34 PM
So for the Oracle destination, I need very precise control over table name generation
u

user

05/20/2021, 8:38 PM
That's the first issue. The second issue is that temp tables end up with a ginormous name like
airbyte_1234567890_airbyte_table_name
that won't fit into 30 characters.
u

user

05/20/2021, 8:39 PM
yeah... 30 characters it not a lot of characters.
u

user

05/20/2021, 8:41 PM
is this article accurate? ^
u

user

05/20/2021, 8:41 PM
would it be sane for us to just support oracle 12.2 and above?
u

user

05/20/2021, 8:42 PM
Well that's interesting
u

user

05/20/2021, 8:42 PM
i guess whatever container you're using is maxing out at 30 characters?
u

user

05/20/2021, 8:44 PM
Hmm... looks like the Docker container I found is version 11. I'll see if there's a more recent one. It looks like that answer was updated in 2017 with the higher limit, so it could be that this is (relatively) new.
u

user

05/20/2021, 8:50 PM
it looks like davin and subodh just slipped this hack in for handling name length
u

user

05/20/2021, 8:51 PM
ah.
u

user

05/20/2021, 8:51 PM
actually that's pretty bad.
u

user

05/20/2021, 8:51 PM
weshouldn't keep doing this.
u

user

05/20/2021, 8:52 PM
yeah, I agree
u

user

05/20/2021, 8:53 PM
i'm wondering if we can do something like this https://github.com/airbytehq/airbyte/pull/3515
u

user

05/20/2021, 8:54 PM
I think the best thing to do would be just to move this logic back to the table name generation functions and un-deprecate them. Looking at the deprecation message, it basically says "we don't like this because it's very SQL-specific and not everything we connect to is a SQL database." But for stuff that is a SQL database, there are times you legitimately need SQL-specific stuff. And if you're not a SQL database, just don't use that part.
u

user

05/20/2021, 8:55 PM
does my suggestion not achieve the same thing?
u

user

05/20/2021, 8:56 PM
you mean that PR?
u

user

05/20/2021, 8:56 PM
yeah
u

user

05/20/2021, 8:56 PM
you know what i think you're right, it's fine to just use those deprecated methods for now i think that's okay.
u

user

05/20/2021, 8:56 PM
we can spend some time cleaning this up after.
u

user

05/20/2021, 8:57 PM
...kind of, but now you need a bunch of special-case logic in
getIdentifier
to check for specific prefixes and parse stuff out, etc.
u

user

05/20/2021, 8:57 PM
what's the specific prefix?
u

user

05/20/2021, 8:58 PM
isn't it just that oracle doesn't let you use special characters in names?
u

user

05/20/2021, 8:58 PM
You can use special characters; you just can't start with an underscore.
u

user

05/20/2021, 8:58 PM
ha
u

user

05/20/2021, 8:59 PM
And I'm still thinking about the length restriction, just in case we're stuck with pre 12.2 versions
u

user

05/20/2021, 8:59 PM
ok
u

user

05/20/2021, 9:01 PM
Checking with a different container name now, to see if I can get this working
u

user

05/20/2021, 9:34 PM
If I change the name of the test container, and suddenly it seems to hang (spinning for 10+ minutes at the "Instantiating tests..." phase) when I run tests, what's the most likely problem?
u

user

05/20/2021, 9:42 PM
how are you changing the name?
u

user

05/20/2021, 9:43 PM
db = new OracleContainer("image name goes here");
u

user

05/20/2021, 9:44 PM
It finally got past that and gave an error:
java.sql.SQLException: Cannot create PoolableConnectionFactory (Listener refused the connection with the following error:
ORA-12514, TNS:listener does not currently know of service requested in connect descriptor
)
u

user

05/20/2021, 9:46 PM
so it didn't hang forever, it eventually did start?
u

user

05/20/2021, 9:46 PM
yeah
u

user

05/20/2021, 9:48 PM
got it. is the image you are passing in there supposed to work?
u

user

05/20/2021, 9:48 PM
with the original image you are were able to log in just fine
u

user

05/20/2021, 9:48 PM
new image doesn't?
u

user

05/20/2021, 9:48 PM
so i maybe there's some reason that image incompatible with test containers?
u

user

05/20/2021, 9:48 PM
Yeah. I'm looking around on DockerHub for Oracle 12 images. Haven't found one yet that works.
u

user

05/20/2021, 9:49 PM
But there are a bunch so I'll keep looking...
u

user

05/20/2021, 9:49 PM
kk.
3 Views