Channels
j
Jared Rhizor (Airbyte)
11/11/2020, 3:04 AM
@charles I'm running into some problems with google adwords -> postgres
u
user
11/11/2020, 3:04 AM Copy code
2020-11-11 03:00:31 ERROR (/tmp/workspace/32/0) LineGobbler(voidCall):69 - Caused by: org.postgresql.util.PSQLException: ERROR: relation "click_performance_report_1605063619055" does not existu
user
11/11/2020, 3:04 AMhowever,
CLICK_PERFORMANCE_REPORT_1605063619055u
user
11/11/2020, 3:04 AMall of the performance reports are all caps
u
user
11/11/2020, 3:05 AMcampaigns/ad_groups/ads/accounts are all snake case
u
user
11/11/2020, 3:05 AMthis is without normalization
u
user
11/11/2020, 3:05 AMmaybe this could be related to the problems @s is seeing with capitalization problems?
u
user
11/11/2020, 3:07 AMI think this might be a casing issue
u
user
11/11/2020, 3:07 AMcapitalization
u
user
11/11/2020, 3:07 AMdo you think this is in the same area you're already looking into?
u
user
11/11/2020, 3:07 AMdamn
u
user
11/11/2020, 3:08 AMor want me to look into it
u
user
11/11/2020, 3:08 AMi think it’s related.
u
user
11/11/2020, 3:08 AMbut different.
u
user
11/11/2020, 3:08 AMsources need to be responsible for passing valid stream and field names.
u
user
11/11/2020, 3:09 AMbut not all valid stream and field names are going to be valid in a destination
u
user
11/11/2020, 3:09 AMand a destination should be responsible for making sure that all valid stream and field names can be handled
u
user
11/11/2020, 3:09 AMwhich it sounds like they don’t right now.
u
user
11/11/2020, 3:10 AMit seems like the postgres destination needs to be adjusted to handle this capitalization case.
u
user
11/11/2020, 3:10 AMunless we want to take a stance that stream names and fields names are always all caps 🤷
u
user
11/11/2020, 3:10 AMi think tha’ts probably a bad idea
u
user
11/11/2020, 3:11 AMif everything for postgres is properly quoted, it should support upper case names
u
user
11/11/2020, 3:14 AMmaybe i’m not understanding the problem.
u
user
11/11/2020, 3:14 AMI guess it's a question of if we want to normalize names or support case sensitive names
u
user
11/11/2020, 3:14 AMwhy is it capitalized sometimes and not others in this postgres case?
u
user
11/11/2020, 3:15 AMI'm not sure, it might just be that the source streams happen to be that way
u
user
11/11/2020, 3:15 AMi’m learning towards not supporting case sensitive names.
u
user
11/11/2020, 3:15 AMthe source is outputting capitalized streams in my case:
https://user-images.githubusercontent.com/6246757/98760749-c5716700-2388-11eb-9b1d-d58cfde9575f.png▾
u
user
11/11/2020, 3:15 AMthese are dumb issues to have.
u
user
11/11/2020, 3:15 AMwoof. that’s so whacky.
u
user
11/11/2020, 3:16 AMI think this means we aren't using jooq escaping properly in the destination for names
u
user
11/11/2020, 3:19 AMi vote case insensitive.
u
user
11/11/2020, 3:19 AMmeaning we auto lower case everything?
u
user
11/11/2020, 3:19 AMor that we reject uppercase?
u
user
11/11/2020, 3:19 AM• sources need to be able to only return and need to be able to acceppt either all caps or all lower case. don’t care which one.
• destinations need to be able to convert from all lower case or all upper case to whatever case they need.
u
user
11/11/2020, 3:20 AMtu so destination needs to be able to accept
u
user
11/11/2020, 3:20 AMwhat do we want uppercase or lower case?
u
user
11/11/2020, 3:21 AMlower case?
u
user
11/11/2020, 3:21 AMyes
u
user
11/11/2020, 3:21 AMso then sources can only emit lower case stream names and field names
u
user
11/11/2020, 3:21 AMwhy either all caps or upper?
u
user
11/11/2020, 3:21 AMI think it has to happen all in the destination though
u
user
11/11/2020, 3:21 AMplus whatever restrictions we put on them around whitespace and whatever.
u
user
11/11/2020, 3:21 AMdestinations assume all inputs are all lower case and adjust them to whatever they need them to be.
u
user
11/11/2020, 3:22 AMI think it's safer if the destination is fully responsible for interpreting the stream name
u
user
11/11/2020, 3:22 AM^
u
user
11/11/2020, 3:22 AMI think it’s simpler if we just say no matter what a source outputs, the destination will be responsible for handling it correctly
u
user
11/11/2020, 3:22 AMalthough I'm worried that the schema selection won't match then
u
user
11/11/2020, 3:23 AMyeah. i’m worried what the airbyte catalog is going to look like if we don’t enforce sanity in the sources.
u
user
11/11/2020, 3:23 AMi do agree destinations should be cautious and assume the worse. no disagreement there
u
user
11/11/2020, 3:23 AMbut i think we also need to enforce what the source is allowed to emit
u
user
11/11/2020, 3:23 AMand we should be strict so that we don’t have to handle stupid edge case in our configuration model.
u
user
11/11/2020, 3:23 AMyeah, my most recent statement was just w.r.t casing, not special characters
u
user
11/11/2020, 3:24 AMi want to enforce casing in airbyte catalog too is what i’m saying
u
user
11/11/2020, 3:24 AMi don’t want source to emit any non lowercase characters.
u
user
11/11/2020, 3:24 AMis that cool with you?
u
user
11/11/2020, 3:25 AMso that means at the python entrypoint.py level we'd want to standardize naming?
u
user
11/11/2020, 3:25 AMmaybe with https://pypi.org/project/stringcase/
u
user
11/11/2020, 3:25 AMthat is cool with me. I like the idea of enforcing naming
u
user
11/11/2020, 3:25 AMthat would probably tide us over on the destination side for release too
u
user
11/11/2020, 3:26 AMwhat do we do if a source has
aBcABcu
user
11/11/2020, 3:27 AMfor the release I mean
u
user
11/11/2020, 3:27 AMdeterministically pick one.
u
user
11/11/2020, 3:27 AMso i actually don’t think stringcase works.
u
user
11/11/2020, 3:27 AMfor this
u
user
11/11/2020, 3:28 AMi don’t think any of our infrastructure can normalize for the integration
u
user
11/11/2020, 3:28 AMthe integration needs to do it right.
u
user
11/11/2020, 3:28 AMwe can't do that for the release though
u
user
11/11/2020, 3:29 AMif the field name that the source emits in discover is
fooBarfoobarfoobarfooBaru
user
11/11/2020, 3:29 AMah. i see what you’re saying.
u
user
11/11/2020, 3:29 AMjust as a stop gap.
u
user
11/11/2020, 3:29 AMyeah
u
user
11/11/2020, 3:29 AMi mean as far as i know there’s only one source that causes problems right now. google sheets.
u
user
11/11/2020, 3:29 AMi can fix that one tomorrow
u
user
11/11/2020, 3:30 AMsounds like maybe adwords too.
u
user
11/11/2020, 3:30 AMcan we just fix those two and not mess with entrypoint.py?
u
user
11/11/2020, 3:30 AMand salesforce? or was that something else
u
user
11/11/2020, 3:30 AMI think it can also happen in sql sources too
u
user
11/11/2020, 3:30 AMdepending on the data
u
user
11/11/2020, 3:30 AMyeah…. i think if we put a stop gap thing in entrypoint.py we will cause new bugs
u
user
11/11/2020, 3:31 AMyeah...
u
user
11/11/2020, 3:31 AMi think we try our best to get by with what we have now. fix the sources we know are really bad and then gun for doing the right thing after release as a top priority.
u
user
11/11/2020, 3:31 AMis it actually hard to support all casings in the jdbc destinations?
u
user
11/11/2020, 3:32 AMis there a reason we can't just quote all table names and call it a day?
u
user
11/11/2020, 3:32 AMyou’re just talking for sources?
u
user
11/11/2020, 3:32 AMno in the destinations
u
user
11/11/2020, 3:32 AMbasically support all cases
u
user
11/11/2020, 3:32 AM🤦♀️ right. dumb question.
u
user
11/11/2020, 3:33 AMi don’t know enough off the top of my head to know if that will work.
u
user
11/11/2020, 3:33 AMyeah I don't want to make any decisions on this at this stage
u
user
11/11/2020, 3:33 AMgoing to sleep on it and revisit in the morning
u
user
11/11/2020, 3:33 AM🤝
u
user
11/11/2020, 3:33 AMmaybe poke around a bit to see how we're quoting
u
user
11/11/2020, 3:33 AM🥨 🥨 🥨 🥨
u
user
11/11/2020, 3:33 AMagreed.
u
user
11/11/2020, 4:28 PMthought about this some more this morning. i think alot of my thinking was wrong.
u
user
11/11/2020, 4:28 PMi think we should avoid touching sources at all pre-release if we can.
u
user
11/11/2020, 4:29 PMi think investing in getting destinations to normalize stuff is generally worthwhile.
u
user
11/11/2020, 4:31 PMpriority of work from my point of view now is as follows:
• (pre-release) add to standard tests a data set that includes streams / fields with spaces and weird casing in the name. get all destinations (with normalization) to success given these cases
• (pre-release or right after release) add to sync worker something that applies a normalization to stream and field names before they get to destinations (remove white space, stripe out special characters)
• (post-release) at least highly encourage that sources conform to a certain set of rules but maybe don’t fail on them.
u
user
11/11/2020, 4:32 PMi could be convinced that i have the order of steps 1 and 2 wrong. what do you think.
u
user
11/11/2020, 4:33 PMPre release we can add quoting to identifiers in the Postgres destination too
u
user
11/11/2020, 4:34 PMI think it’s easier to support case sensitivity short term than it is to consider name normalization
u
user
11/11/2020, 4:34 PMi came to this ordering because, no matter what destinations will need to do some name normalization from what comes in no matter what. Either it receives any possible string or it receives a string that is already normalized to whatever standard airbyte sets, but not necessarily what the db supports). it will also be easy to test this and give us high confidence it’s working in the standard tests.
u
user
11/11/2020, 4:35 PMi have meeting with zazmic and need to review their code now, but i can work on adding the extra data set to the standard tests once i’m done unless someone beats me to it.
u
user
11/11/2020, 4:35 PMjared, sounds like you’re focusing on fixing the pg stuf?
u
user
11/11/2020, 4:36 PMYeah I think it’s a short fix, I’ll start on it in 10-15min
u
user
11/11/2020, 4:36 PMkk
u
user
11/11/2020, 4:52 PMPre release we can add quoting to identifiers in the Postgres destination tooThat's what i end up doing in the normalization pieces by DBT with the default
quotingu
user
11/11/2020, 5:17 PMu
user
11/11/2020, 5:59 PMgoing to publish postgres with this change
u
user
11/11/2020, 5:59 PMhotfixing the current version
u
user
11/11/2020, 6:00 PMtu
u
user
11/11/2020, 6:05 PMdone
u
user
11/11/2020, 6:12 PMException in thread "main" org.jooq.exception.DataAccessException: SQL [insert into streamWithCamelCase_1605118306833 (ab_id, data, emitted_at) values (?, cast(? as jsonb), cast(? as timestamp with time zone))]; ERROR: relation "streamwithcamelcase_1605118306833" does not existu
user
11/11/2020, 6:12 PMi’m getting this error. should your change have fixed this?
u
user
11/11/2020, 6:17 PMor that was just for whitspacing?
u
user
11/11/2020, 6:27 PM@Jared Rhizor (Airbyte)
u
user
11/11/2020, 6:27 PMmight fix this
u
user
11/11/2020, 6:28 PMdid you pull the latest version?
u
user
11/11/2020, 6:28 PMyes
u
user
11/11/2020, 6:28 PMI would expect the logged sql to include the quotes then
u
user
11/11/2020, 6:28 PM Copy code
→ docker image ls airbyte/destination-postgres:0.1.1
REPOSITORY TAG IMAGE ID CREATED SIZE
airbyte/destination-postgres 0.1.1 2d00d190bca9 28 minutes ago 440MBu
user
11/11/2020, 6:29 PMCan you verify you're on that image?
u
user
11/11/2020, 6:29 PMAnd also, is the test you're running based off of a dev image or 0.1.1? If it's off of dev, have you merged master into your test branch?
u
user
11/11/2020, 6:33 PMyes to all of those things.
u
user
11/11/2020, 6:33 PMi’m using :dev tag
u
user
11/11/2020, 6:34 PMand your change is included
u
user
11/11/2020, 6:35 PMoh this is the insert
u
user
11/11/2020, 6:35 PMso not impacted by my change
u
user
11/11/2020, 6:35 PMand this is postgres?
u
user
11/11/2020, 6:35 PMI thought insert would quote
u
user
11/11/2020, 6:35 PMmaybe there's some jooq setting
u
user
11/11/2020, 6:36 PMBy default, jOOQ will always generate quoted names for all identifiers
u
user
11/11/2020, 6:37 PMi’m going to get the new standard tests in a working state and then we can just divide up destinations and get them passing.
u
user
11/11/2020, 7:07 PMdo you want me to include field names that include special charactrers. e.g.
field*name*with*asterisk*u
user
11/11/2020, 7:07 PMI think case sensitivity with _ is all we've seen and all we reasonably need to support
u
user
11/11/2020, 7:08 PMgoogle sheets regularly has spaces too.
u
user
11/11/2020, 7:08 PMI'm fine failing if anyone does something
super$nonstandard!@u
user
11/11/2020, 7:08 PMoh
u
user
11/11/2020, 7:08 PMhmm
u
user
11/11/2020, 7:08 PMi’ll do spaces but not asterisk.
u
user
11/11/2020, 7:08 PMgoogle sheets probably has special characters too
u
user
11/11/2020, 7:08 PMcool?
u
user
11/11/2020, 7:08 PMoh yeah. i guess $ could be a thing
u
user
11/11/2020, 7:09 PMi mean how much exta work is it to normalize everything that’s not a normal character to an underscore for our existing destinations?
u
user
11/11/2020, 7:09 PMif it’s little extra work then we should just do it now.
u
user
11/11/2020, 7:09 PMmaybe?
u
user
11/11/2020, 7:09 PMI mean we can replace special chars with an
_u
user
11/11/2020, 7:10 PMbut then it’s ambiguous because
x$*y == x*$yu
user
11/11/2020, 7:10 PMare there any destinations that don't support special characters?
u
user
11/11/2020, 7:10 PMsnowflake/postgres do?
u
user
11/11/2020, 7:10 PMi don’t know.
u
user
11/11/2020, 7:10 PMbigquery does too
u
user
11/11/2020, 7:11 PMand i guess the filesystem is pretty tolerant for the csv stuff.
u
user
11/11/2020, 7:11 PMyeah
u
user
11/11/2020, 7:11 PMI guess we should just add some special character tests then
u
user
11/11/2020, 7:12 PMagreed
u
user
11/11/2020, 7:15 PMok
u
user
11/11/2020, 7:15 PMi do this.
u
user
11/11/2020, 7:23 PMokay PR ready https://github.com/airbytehq/airbyte/pull/911
u
user
11/11/2020, 7:23 PM@s re-requested your reivew since it changed kinda substantially from when you looked at in an hour ago.
u
user
11/11/2020, 7:23 PMplan is to merge this. it will break integration tests for bq, snowflake, and pg, and then we split them up and fix them.