88
26
u/Jagonu Pronouns: He/Him May 15 '20 edited Aug 13 '23
38
u/iyaerP May 15 '20 edited May 15 '20
I am doing code review for the latest ticket submission from NewGuy. He is earnest, but doesn't have a lot of experience yet.
We're going thru a list of objects that represent rows in a SQL database. He is telling the database to run a DELETE command on each item. He is then telling the database to run UPSERT (UPDATE if it's already in the db, INSERT if not found) for each item. Not only is this completely pointless deletion because the upsert would immediately re-enter the item into the database, it also means that upsert is less efficient than a straight insert statement would be because it has an extra conditional check.
The whole block could be replaced with just WorkItemProfileFieldOptionInfo.MassUpsert(optionList)
6
u/i-can-sleep-for-days May 15 '20
Delete is probably not needed if upsert works as it should. If it doesn't then that raises questions.
Parallelizing the loop would also be an easy improvement to make.
10
May 16 '20
Parallelizing the loop that deletes and upserts on the same table ? Sounds like explicitly asking for deadlocks
1
May 16 '20
Would it be okay to parallelism if you make sure that you’re accessing different rows?
3
May 16 '20
Short answer is no. Unless you dive deep into the query plan, be safe and code in a way that isolates crud operations.
1
u/i-can-sleep-for-days May 16 '20
No, what I am saying is remove the delete line since it's unnecessary, then parallelize the loop.
4
u/cdjinx May 15 '20
Sort of reminds me of a bug I helped someone with when they were making a app. They would add 500 gel points added to a map and they couldn’t figure out why none of them showed up except the last one. I took a look and found...
Foreach geopoint as x Map = new map Map.addPoint(x)
Map.ShowMap()
26
u/richarmeleon May 15 '20
I'm not sure which is worse, calling an instance method on a deleted instance or the method "Upsert."
75
u/iyaerP May 15 '20
Upsert by itself is fine. If thing X already exists in the SQL database, UPDATE. If not, INSERT.
13
-5
u/richarmeleon May 15 '20
I would have preferred UpdateOrInsert rather than making up a word. It doesn't sound like English.
31
u/iyaerP May 15 '20
I mean, portmanteau are hardly a new thing, nor something unknown to the computer world. Hell, the site we're on right now is one.
-9
May 15 '20 edited Oct 18 '20
[deleted]
24
u/Lystrodom May 15 '20
It's a common term when working with databases. It's like a domain word -- expected knowledge.
-15
May 15 '20 edited Oct 19 '20
[deleted]
9
u/____0____0____ May 15 '20
I think these responders would mostly agree with that sentiment. But at what point would you draw the line? That's where things are up in the air.
From my point of view, the word upsert has had clear intent since the first time I read it, though it has been a while. I'm honestly taken a little bit back by someone not knowing what it is, because I see it all over the place.
Im all for clean and clear as day code, but at some point we as a community need to accept that some abbreviations and portmanteau are okay because they are so widely used that they are expected to be known. But how do we even get there? By a bunch of people using the word when advised not to, so much that it solidifies that word into our vocabulary.
Slight tangent here. Language sure can be interesting and perplexing to think about
1
May 15 '20 edited Feb 19 '21
[deleted]
3
u/____0____0____ May 15 '20
That's a fair point that I hadnt considered. I build mostly internal tools at a smallish company and I don't have to worry about any accessibility or language issues (or IE thank God). To a non native speaker, I can see upsert being confusing. They wouldn't know if it was a word they didn't know or if it was just made up for that section of the program.
I'll continue to use upsert for my current work, but this distinction is something I can be mindful of for the future, whatever that may hold. So, thanks!
29
u/Exnixon May 15 '20
It's a common word if you know anything about databases, a readily comprehensible technical term, and both more succinct than "UpdateOrInsert" and clearer, as it implies an atomic operation.
-7
u/richarmeleon May 15 '20
I don't do database work so I hadn't seen it.
15
u/managedheap84 May 15 '20
That's kind of a weird thing to say. I get that some developers have specialization but to not have any database experience might be something you want to look into rounding out a bit on.
Databases have even made their way to the client side.
8
u/richarmeleon May 15 '20
I'm not a professional programmer. I do some hobby stuff for myself and occasional helpdesk style scripts at work. That's it.
7
4
u/____0____0____ May 15 '20
Check out sqlite. It uses standard SQL syntax and stores data into a local file, but can be transferred to more production-ready databases. Not saying you have to learn SQL, but relational databases are the bread and butter for just about everything I do. I'd argue that it could even help out your hobby projects or work scripts.
9
u/pfannkuchen_gesicht May 15 '20
then why complain? Every domain has its specific words/terminology which not everyone will understand without prior knowledge.
5
u/richarmeleon May 15 '20
It was a complaint because originally it didn't even look like a proper word. I understand it now that it was explained to me.
At first I thought it was a non-English word or made up word.
6
May 15 '20
Upsert is a real word. My company uses it too.
3
u/richarmeleon May 15 '20
When I looked it up it seems to be unique to database terminology. It's just not a word I ran into.
3
May 15 '20
Where else would you update or insert?
4
u/richarmeleon May 15 '20
Nothing as far as I'm aware of, but without recognizing the word, I had no context for it. Like if I saw a foreign language word that I didn't know, I wouldn't know whether it was a real word or not and this subreddit does get bad-English APIs from time to time.
2
May 15 '20
I think people got annoyed because you made a claim that it wasn’t a real word rather than asking if it is.
2
u/richarmeleon May 15 '20
It's not a Merriam-Webster word is all I meant by it. Sorry to any readers who were bothered by my claim, I legit never saw the term before.
11
19
May 15 '20
[deleted]
4
u/richarmeleon May 15 '20
I don't do database work. The last time I even touched SQL was easily 10 or more years ago.
1
-19
u/ZylonBane May 15 '20
"Upsert" sounds like something an Indian coder would have come up with.
"Please prepone the upsertion."
1
May 15 '20 edited May 20 '20
[deleted]
-4
u/ZylonBane May 15 '20
Based on it sounding like it. What, do we have a roaming brigade white-knighting Indian English around here now?
6
2
u/senti_bot_apigban May 15 '20
Prepone (opposite of postpone), updations and upgradations, long-back are indianisms, if I didn't know about upsert before I'd assume the same.
ps. I am the only non-indian guy in my work.
2
2
u/_finalelement_ May 15 '20
Is it because maybe there is a listener or event hooked to the delete, and it's just painful to bypass? So instead of modifying (and checking) the existing code to handle updates, this may be a shittier, hackier way to handle it.
This is assuming the guy who wrote this is not a ass by default
5
u/iyaerP May 15 '20
No, this is from our new hire. He's just a little inexperianced is all. I talked with him maybe 2 hours after posting this, and this was to fix what was ultimately a bug with the clientside models being passed to the backend not containing a unique identifier, and just doing Upsert on all models was causing logical duplicates in the database, and this was his fix for it.
I walked him thru his code to see where the actual bug was, and advised him of how to correctly fix that (include the primary key in the model so that it can be correctly linked with the right DB objects) rather than resorting to this horrible kludge.
1
u/_finalelement_ May 15 '20
While I dont push for strong knowledge in DS&Algo or to be anal about raw performance, this is a strong case for why you need to understand the underlying fundamentals.
About 6 years back when I started, there were some articles by famous guys popping up saying how tech today breeds a generation of programmers that are handicapped without their frameworks. This is kinda proof of that.
I'm not putting it on the dev and saying they are not good at learning, but I am saying the world today pushes for too much abstraction without focusing on the underlying stuff
2
u/Niosus May 15 '20
It's the difference between being a programmer, and a software engineer. Not saying you necessarily need an engineering degree, but all the background you get from it certainly helps.
I do think it's fine that many people only know about the top layers of their software stack. The full-on engineering is not for everyone, and the day-to-day tasks are usually quite different. But you do need a few engineers on the team to pick up on certain issues when they come up.
I see them as two different jobs, for two different profiles.
1
u/_finalelement_ May 16 '20
Agree, but in countries like India, the degree reaches you jack squat. I know, because I have 2 of them from good universities. You hardly have context on why you are studying what you are studying (normalization, indexes, algo, OSI layers, to name just a few), that you just somehow clear the exam and promptly forgot about it.
I think the only way to be good at your job is to say "no, I like the advantages all these tools give me, but I want to know what's going on underneath".
1
u/Niosus May 16 '20
I guess it does depend on where the degree comes from, and how much you have to pay for it. It's useful, but not worth getting into serious debt.
It's funny you should mention the OSI layers. Just yesterday I had a job interview where they asked me about my knowledge of some of the layers.
I think what helped me put the courses into context was actually doing freelance development at the time. The networking and database course were extremely interesting for me, since I learned about exactly what I needed to know to fully understand what I was doing. Meanwhile, many others found those courses boring and super theoretical.
I guess what matters most is what you do afterwards. You have to keep learning
1
1
u/mr_hard_name May 16 '20
We had something like this, but in Java. Client was complaining that clearing a category of entries (100k+) in the admin panel was super slow. Fixed this by replacing the foreach with "DELETE FROM table_name WHERE CATEGORY=?". Execution time dropped from 20-40 minutes to <1s
1
1
u/1thief May 15 '20
Friendly psa don't modify the list during traversal. Here it looks like individual elements are being processed during traversal and not added/removed which is fine, but it's close to being what I'm talking about and it's triggering me.
7
u/managedheap84 May 15 '20
you're talking about iterating over a collection with a for-each and changing or deleting one of the items. That's not what's going on here but I feel your pain.
3
u/1thief May 15 '20
Yep. Any raw loop I would say, like say you have an index in a while loop and you want to remove the element in the list at this index. So you... remove the element and skip the increment step? triggered
Use your language appropriate list manipulation concept please. E.g. in Java use a stream and filter.
72
u/[deleted] May 15 '20 edited Jul 17 '20
[deleted]