https://linen.dev logo
Join Slack
Powered by
I just found that `JdbcBufferedConsumerFactory.toW...
# contributing-to-airbyte
m
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
looking now
u
line 109 is this
u
Copy code
final String tableName = Names.concatQuotedNames("_airbyte_raw_", namingResolver.getIdentifier(streamName));
u
which looks like it is calling the naming transformer, no?
u
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
And the things it's doing turn out to be incompatible with Oracle.
u
got it.
u
so i think the table name ones are deprecated.
u
Well, can we un-deprecate them?
u
what's your goal here?
u
are they producing valid names and getidentifier isn't?
u
looking at the implementation of those methods it looks like they do pretty much the same thing as 
getIdentifier
u
...unless you override them
u
Look at all the stuff it's doing after calling 
getIdentifier
u
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
The current system is breaking both of those. (And possibly other things once I get past the first two.)
u
ah.
u
okay. so issue is that on line 109 we are prepending the table name with something that's not valid for oracle.
u
So for the Oracle destination, I need very precise control over table name generation
u
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
yeah... 30 characters it not a lot of characters.
u
u
is this article accurate? ^
u
would it be sane for us to just support oracle 12.2 and above?
u
Well that's interesting
u
i guess whatever container you're using is maxing out at 30 characters?
u
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
u
it looks like davin and subodh just slipped this hack in for handling name length
u
ah.
u
actually that's pretty bad.
u
weshouldn't keep doing this.
u
yeah, I agree
u
i'm wondering if we can do something like this https://github.com/airbytehq/airbyte/pull/3515
u
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
does my suggestion not achieve the same thing?
u
you mean that PR?
u
yeah
u
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
we can spend some time cleaning this up after.
u
...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
what's the specific prefix?
u
isn't it just that oracle doesn't let you use special characters in names?
u
You can use special characters; you just can't start with an underscore.
u
ha
u
And I'm still thinking about the length restriction, just in case we're stuck with pre 12.2 versions
u
ok
u
Checking with a different container name now, to see if I can get this working
u
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
how are you changing the name?
u
db = new OracleContainer("image name goes here");
u
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
so it didn't hang forever, it eventually did start?
u
yeah
u
got it. is the image you are passing in there supposed to work?
u
with the original image you are were able to log in just fine
u
new image doesn't?
u
so i maybe there's some reason that image incompatible with test containers?
u
Yeah. I'm looking around on DockerHub for Oracle 12 images. Haven't found one yet that works.
u
But there are a bunch so I'll keep looking...
u
kk.
3 Views
Open in Slack
PreviousNext
Powered by