-
-
Notifications
You must be signed in to change notification settings - Fork 519
Performance on insert many #548
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
base: master
Are you sure you want to change the base?
Changes from all commits
6f19ea8
c861e95
24f3679
7716c1b
9424ec4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,6 +2,7 @@ | |||||
using System.Collections.Generic; | ||||||
using System.Diagnostics; | ||||||
using System.Linq; | ||||||
using System.Text; | ||||||
using System.Text.RegularExpressions; | ||||||
|
||||||
namespace SqlKata.Compilers | ||||||
|
@@ -156,20 +157,24 @@ protected override string CompileBasicDateCondition(SqlResult ctx, BasicDateCond | |||||
} | ||||||
|
||||||
protected override SqlResult CompileRemainingInsertClauses( | ||||||
SqlResult ctx, string table, IEnumerable<InsertClause> inserts) | ||||||
SqlResult ctx, string table, IReadOnlyCollection<InsertClause> inserts) | ||||||
{ | ||||||
var sql = new StringBuilder(ctx.RawSql, inserts.Count - 1); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same StringBuilder constructor issue as in Compiler.cs. The second parameter should be capacity, not a count calculation.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||
|
||||||
foreach (var insert in inserts.Skip(1)) | ||||||
{ | ||||||
string columns = GetInsertColumnsList(insert.Columns); | ||||||
string values = string.Join(", ", Parameterize(ctx, insert.Values)); | ||||||
string columns = GetInsertColumnsList(insert.Data.Keys); | ||||||
string values = string.Join(", ", Parameterize(ctx, insert.Data.Values)); | ||||||
|
||||||
string intoFormat = " INTO {0}{1} VALUES ({2})"; | ||||||
var nextInsert = string.Format(intoFormat, table, columns, values); | ||||||
const string intoFormat = " INTO {0}{1} VALUES ({2})"; | ||||||
|
||||||
ctx.RawSql += nextInsert; | ||||||
sql.Append(string.Format(intoFormat, table, columns, values)); | ||||||
} | ||||||
|
||||||
ctx.RawSql += " SELECT 1 FROM DUAL"; | ||||||
sql.Append(" SELECT 1 FROM DUAL"); | ||||||
|
||||||
ctx.RawSql = sql.ToString(); | ||||||
|
||||||
return ctx; | ||||||
} | ||||||
} | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
using System.Collections.Generic; | ||
using System.Linq; | ||
|
||
namespace SqlKata.Extensions | ||
{ | ||
public static class CollectionExtensions | ||
{ | ||
public static Dictionary<string, object> MergeKeysAndValues(this List<string> keys, List<object> values) | ||
{ | ||
var data = new Dictionary<string, object>(); | ||
|
||
for (var i = 0; i < keys.Count; i++) | ||
{ | ||
data.Add(keys[i], values[i]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential IndexOutOfRangeException if values list is shorter than keys list. The validation should happen in this method or the caller should guarantee equal lengths. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||
} | ||
|
||
return data; | ||
} | ||
|
||
public static Dictionary<string, object> CreateDictionary(this IEnumerable<KeyValuePair<string, object>> values) | ||
{ | ||
if (values is Dictionary<string, object> dictionary) | ||
{ | ||
return dictionary; | ||
} | ||
|
||
return values.ToDictionary(x => x.Key, x => x.Value); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -96,14 +96,16 @@ public static string ReplaceAll(string subject, string match, Func<int, string> | |||||||
|
||||||||
public static string JoinArray(string glue, IEnumerable array) | ||||||||
{ | ||||||||
var result = new List<string>(); | ||||||||
var result = new StringBuilder(); | ||||||||
|
||||||||
foreach (var item in array) | ||||||||
{ | ||||||||
result.Add(item.ToString()); | ||||||||
result.Append(item + glue); | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. String concatenation using the + operator within a loop creates unnecessary string objects. Use StringBuilder.Append() with separate calls for better performance.
Suggested change
Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||
} | ||||||||
|
||||||||
return string.Join(glue, result); | ||||||||
result.Length -= glue.Length; | ||||||||
|
||||||||
return result.ToString().Trim(); | ||||||||
Comment on lines
+106
to
+108
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This approach will fail when the array is empty, causing result.Length to become negative. Add a check for empty arrays before attempting to modify the length. Copilot uses AI. Check for mistakes. Positive FeedbackNegative Feedback |
||||||||
} | ||||||||
|
||||||||
public static string ExpandParameters(string sql, string placeholder, object[] bindings) | ||||||||
|
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.
StringBuilder constructor parameters are (string, int capacity), but you're passing (string, int length). The second parameter should be the initial capacity, not the count calculation. Use
new StringBuilder(ctx.RawSql)
or provide a proper capacity estimate.Copilot uses AI. Check for mistakes.