-
Notifications
You must be signed in to change notification settings - Fork 4.8k
source-postgres: field type mapping #65983
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
source-postgres: field type mapping #65983
Conversation
π Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Helpful Resources
PR Slash CommandsAirbyte Maintainers (that's you!) can execute the following slash commands on your PR:
|
|
|
|
|
|
|
|
40cf841
to
de8cc06
Compare
} | ||
} | ||
|
||
class InfJsonEncoder<T>(private val base: JsonEncoder<T>) : JsonEncoder<InfWrapper<T>> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we're using the type of JsonEncoder as a hint for protobuf encoding, this may create a difficulty because the inner type is erased.
Maybe reified
can help here.
I'm noting this as a difficulty to myself.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
inline
and reified
are new to me. Good idea.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just played around with this a bit. Maybe we can pair on it in person later this week.
JDBCType.CHAR, | ||
JDBCType.VARCHAR, | ||
// JDBCType.TINYTEXT, | ||
// JDBCType.TEXT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
all these don't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, forgot to clean this up. Commit incoming.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait, I have this removed locally and the sha on the tip commit matches.... something weird is going on
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, the commented ones are all in red here.... duh. Yes, these JDBC types are not present in column metadata for Postgres.
object PostgresSource { | ||
@JvmStatic | ||
fun main(args: Array<String>) { | ||
// TODO: Remove before merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh π€¦
Co-authored-by: Rodi Reich Zilberman <[email protected]>
Add field type mapping and associated tests based on the testing framework to the postgres source on the bulk CDK.
Can this PR be safely reverted and rolled back?