Repository navigation
1118 build combined sales qc view - #1120
Conversation
| -- matching sale in iasWorld. | ||
| WITH mydec AS ( | ||
| SELECT | ||
| mydec.*, |
There was a problem hiding this comment.
I presume that some PINs are getting rejected since they don't align with a valid address. I tried to match them with our vw_pin_address, but I don't know if we have a better way.
There was a problem hiding this comment.
Looks like a good start to me! Let's get @wrridgeway to weigh in as the MyDec expert before we merge and share with stakeholders. I haven't requested his review yet since tests aren't yet passing; go ahead and do that once tests are passing and this is good to share.
| -- Normalize addresses so that MyDec unit numbers (e.g. "# 6S") | ||
| -- match the iasWorld format (e.g. "6S") | ||
| TRIM(REGEXP_REPLACE( | ||
| REPLACE(UPPER(mydec.line_1_street), '#', ''), '\s+', ' ' | ||
| )) AS address_normalized, |
There was a problem hiding this comment.
[Suggestion, non-blocking] I don't really think we need to bother with address matching at this point, since our stakeholders haven't identified it as a priority. Personally I would skip it at this stage, but I also don't mind leaving it in if you think it's important.
There was a problem hiding this comment.
I would leave it in personally. I think some sales were based on firm headquarters rather than the addresses, but I'm not sure.
EDIT: And the best way to look through to validate the sales is also to query MLS sites, and address is needed for that. But, not all of them were returning to the listed PIN. It may be better as a T/F indicator rather than matching.
There was a problem hiding this comment.
As long as we caveat that it is just for QC work.
|
@wrridgeway @jeancochrane |
wrridgeway
left a comment
wrridgeway
left a comment
There was a problem hiding this comment.
I just reviewed construction correctness, not whether this output meets the intended use. Great work, just some minor suggestions primarily for readability and maintainability.
| mydec AS ( | ||
| SELECT * | ||
| FROM mydec_all | ||
| WHERE doc_row_num = 1 | ||
| ), |
There was a problem hiding this comment.
Probably clearest just to let this (WHERE doc_row_num = 1) be a conditional in your main query, I don't think there needs to be an entire CTE for it.
| WHERE NOT EXISTS ( | ||
| SELECT 1 | ||
| FROM {{ source('iasworld', 'sales') }} AS sales | ||
| WHERE sales.deactivat IS NULL | ||
| AND sales.cur = 'Y' | ||
| -- Strip non-digit characters from iasWorld | ||
| AND REGEXP_REPLACE(sales.instruno, '[^0-9]', '') | ||
| = mydec_clean.document_number_clean | ||
| ) |
There was a problem hiding this comment.
This code isn't wrong, but it feels a lot less clear than just doing a typical sql left anti join. This CTE could also use some commenting to explain what it's doing.
| WHERE NOT EXISTS ( | |
| SELECT 1 | |
| FROM {{ source('iasworld', 'sales') }} AS sales | |
| WHERE sales.deactivat IS NULL | |
| AND sales.cur = 'Y' | |
| -- Strip non-digit characters from iasWorld | |
| AND REGEXP_REPLACE(sales.instruno, '[^0-9]', '') | |
| = mydec_clean.document_number_clean | |
| ) | |
| LEFT JOIN iasworld.sales AS sales | |
| ON sales.deactivat IS NULL | |
| AND sales.cur = 'Y' | |
| AND REGEXP_REPLACE(sales.instruno, '[^0-9]', '') = mydec_clean.document_number_clean | |
| WHERE sales.instruno is nul |
| WHERE doc_row_num = 1 | ||
| ), | ||
|
|
||
| pin_address AS ( |
There was a problem hiding this comment.
Let's make sure every CTE has a comment explaining its existence.
| INNER JOIN pin_address | ||
| ON mydec.address_normalized = pin_address.address_normalized | ||
| AND mydec.city = pin_address.city | ||
| AND mydec.zip = pin_address.zip |
There was a problem hiding this comment.
Not sure if you tested this, or if it makes any difference, but i'd be interested to see what ignoring zip does. Hopefully nothing, but it would be a small bummer to say addresses that are the same but for a zip don't match.
|
@wrridgeway I simplified it a lot. Do you mind one more look at it? |
|
I removed the address join. It didn't seem to work great and added in multi-pin sales as a flag. |
wrridgeway
left a comment
wrridgeway
left a comment
There was a problem hiding this comment.
This looks good to me! I'd bring back the window function you had before just to be sure this view always produces a unique set of document numbers and then you're good to go if this meets the stakeholders' desires.
| @@ -0,0 +1,44 @@ | |||
| -- View that returns all MyDec sales from 2020 onward that do not have a | |||
| -- matching sale in iasWorld. Output is unique by document_number. | |||
There was a problem hiding this comment.
Output is unique by document_number, but I'm not sure we're doing anything here to actually enforce that assertion. You might want to bring back your window function just to be sure.
There was a problem hiding this comment.
Yeah I took it out because it wasn't necessary, but I'll just add it back in.
There was a problem hiding this comment.
Yeah, I tested and saw that it isn't currently necessary, but I believe mydec.sale could be non-unique by doc no under certain conditions in the future.
View with all sales in
sale.mydecfrom 2020 onward that do not have a matching sale iniasworld.sales, joined on document number.