5
As dfhwze says, you should probably pull in the connection string/information from an external source: the production database hopefully won't be called 'TEST', which means you can't use this code in production, which means you will be adding time, effort, and the opportunity for things to go wrong to deployment. Similarly for the web API URL: it might ...
5
Review
Single responsibility principle
Method save_vendor_info (which we'll rename later) does data retrieval from an external source, xml to db mapping, and SQL command execution on a database. I would expect a method with the name Save* to only store data, in this case in the database. I would also suggest to split up loading the input data and saving to ...
4
Comments on your approach
The most obvious thing to me is that you're already inserting all of your data into the temp table; why not just join to it? You can dynamically pivot into a better, strongly-typed table instead (see this post), and then you don't need to muck around with the parameter stuff.
I also think I think you're overcomplicating this by a ...
4
Create a dictionary mapping the old property IDs to the new ones, then do a lookup:
# I'm only showing few pairs of transformations for brevity.
# You'll need to expand this to include the rest
OLD_TO_NEW_IDS = {'2502': '95560',
'21489': '2502',
'660133': '21489',
'921010': '660133'}
# Get the old ID
...
4
As a heads up - we have very different formatting styles for our code. Feel free to ignore that difference and don't consider it a comment on your style (unless you prefer mine, in which case please do).
Aliases
I really dislike the practice of using short aliases for tables. I've never seen a point, and it always makes it harder for me to understand. You ...
3
Here are a few things that caught my eye while quickly looking through your code:
Imports
As per the official Style Guide for Python Code (aka PEP8), imports should best be grouped. Imports from the standard library come first, then third-party modules, followed by any library/application specific imports. The groups should be seperated by blank lines. In ...
3
Answering your question
Overall, I think the solution is sound, but there are a few things you could make cleaner. I agree with everything in dfhwze's answer, but assuming this is the route you keep here are a few thoughts:
This piece is a little risky in case N'|' exists in the new value.
SELECT @value_separator_index = CHARINDEX( '|', @valueOrDefault );
...
3
Potential Issues
a naive string replacement could change more than just the placeholders; for instance, when another part of the script uses the same string as a literal
the replacement can introduce a value with the same format {%}; your sql has no way of knowing that this is a replacement value, and since the format matches that of a place holder, it will ...
2
I don't completely understand your code, which can be a problem when reviewing it, but I think I can get quite far.
Let me start with the two functions, promote() and demote(), that are at the center of your code. There are several things I notice straight away:
Both functions are, apart from an + or - sign, identical. There's a well known principle in ...
2
Here is another recent Code Review question that has a lot of similarities to yours: Daily SQL job to delete records older than 24 hours.
I think I would recommend three things here:
Snapshot isolation
Not using TOP
Doing the work in another table
Snapshot isolation
Snapshot isolation is a really cool tool that effectively prevents most things from ...
1
Assuming that #PurchaseOrderProducts is a realistic depiction of your real table but with less data (and that your real table is also a heap), then I get this plan.
This is a pretty reasonable plan, where I wouldn't necessarily expect it to be that bad.
There are a few things we could do to make this better, including:
Batch-mode for rowstore is enabled (...
1
I'm pleased to see you included DDL and dataload SQL. This makes reviewing a bliss.
Design
There isn't really much I can say about this design. You wondered whether your many-to-many relation is well being dealt with. Well, you created a composite primary key, which is correct for this design because a car and a feature are not strongly dependent on each ...
1
Review
Your temporary table names don't say much about what they present. This makes it hard to figure out what they mean. Consider using better, more meaningful names.
DROP TABLE #TEMPA;
DROP TABLE #TEMPB;
DROP TABLE #TEMPC;
DROP TABLE #TEMPD;
It gets worse by aliasing these temporary tables with different letters:
FROM #TEMPA AS A -- Fair enough
FROM ...
1
From Comments
Have you tested performance of copying data of a single day to a temporary table, truncating the target table, and re-inserting the copied data back to the target table?
Yes, it's not taking much time to copy or insert, it is taking time to delete and select. One of the column has huge XML string.
Proposed Solution
Since inserting and ...
1
Review
Your SQL seems the generally accepted way (Discussed Before) of deleting duplicates.
WITH cte
AS (SELECT id,
student_code,
exam_code,
mark,
Row_number()
OVER(
partition BY id, student_code, exam_code, mark
ORDER BY ...
1
Review
You don't need to calculate the difference from two temporary values GetUTCDate and GETDATE.
SELECT @DifferenceInHoursBetween = DATEDIFF(HH,GetUTCDate(), GETDATE())
Proposed Solution
Instead, you could apply the local time zone offset to the specified datetime.
SELECT
USER_ID,
CreateDate AS [DateTime IN UTC],
CONVERT(datetime,
...
1
I'd like commentary on the overall query, structure, organization, style.
The query is consistent for style. Its structure and organization seems to match the functional decomposition you did to achieve the desired result.
While the use of a temporary table to shape, reduce and transform your data isn't uncommon SQL Server does offer an alternative ...
1
Broad feedback
For each value of $x less than $y there are 3 API calls - it would be wise to cache those values (e.g. in a local database, local file, etc.) and refresh values when necessary (e.g. daily, weekly, monthly, etc.
Targeted feedback
The naming of variables could be better - e.g.
$symbols instead of $sym
$prices instead of $pri
$volumes ...
1
I took a very, very wild swing at this. I also couldn't get it down to something entirely set-based, but I was able to get it to an inline TVF with a recursive CTE (two of them, unfortunately). With 1000 iterations, I needed a MAXRECURSION of at least 998 to get it to finish.
I wouldn't trust this as a cryptographically secure implementation. I strongly ...
Only top voted, non community-wiki answers of a minimum length are eligible
