Performance on insert many #548
Conversation
Use dictionary in insert clause Fixed sql server compiler test runner Improved string build performance
mkolumb
force-pushed
the
feature/compiler-performance
branch
from
a36151d to
6f19ea8
Compare
…erformance # Conflicts: # QueryBuilder.Tests/Infrastructure/TestCompilersContainer.cs
|
@ahmad-moussawi Any chance to review? |
Merge master
…rformance # Conflicts: # QueryBuilder/Compilers/Compiler.cs
|
@ahmad-moussawi |
There was a problem hiding this comment.
Pull Request Overview
This pull request improves performance for insert operations, particularly for bulk inserts, by addressing inefficiencies in data structure usage and string building operations. The changes modernize the codebase to use more efficient data structures and operations.
- Replaces separate Columns/Values lists with Dictionary-based data structure for better performance
- Optimizes string building using StringBuilder instead of string concatenation
- Adds new overload for bulk insert operations with KeyValuePair collections
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| QueryBuilder/Query.Update.cs | Updates to use Dictionary-based data structure and new extension methods |
| QueryBuilder/Query.Insert.cs | Replaces column/value lists with Dictionary approach and adds new bulk insert overload |
| QueryBuilder/Helper.cs | Optimizes JoinArray method using StringBuilder for better performance |
| QueryBuilder/Extensions/CollectionExtensions.cs | New extension methods for merging keys/values and creating dictionaries |
| QueryBuilder/Compilers/OracleCompiler.cs | Updates compiler to work with new Dictionary-based data structure |
| QueryBuilder/Compilers/Compiler.cs | Major refactoring to use StringBuilder and Dictionary-based operations |
| QueryBuilder/Compilers/Compiler.Conditions.cs | Optimizes condition compilation using StringBuilder |
| QueryBuilder/Clauses/InsertClause.cs | Changes data structure from separate lists to single Dictionary |
| QueryBuilder.Tests/InsertTests.cs | Adds test for new bulk insert functionality |
| QueryBuilder.Tests/Infrastructure/TestCompilersContainer.cs | Fixes test isolation by using factory pattern for compiler instances |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| foreach (var item in array) | ||
| { | ||
| result.Add(item.ToString()); | ||
| result.Append(item + glue); |
There was a problem hiding this comment.
String concatenation using the + operator within a loop creates unnecessary string objects. Use StringBuilder.Append() with separate calls for better performance.
| result.Append(item + glue); | |
| result.Append(item); | |
| result.Append(glue); |
Copilot uses AI. Check for mistakes.
| result.Length -= glue.Length; | ||
|
|
||
| return result.ToString().Trim(); |
There was a problem hiding this comment.
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.
|
|
||
| for (var i = 0; i < keys.Count; i++) | ||
| { | ||
| data.Add(keys[i], values[i]); |
There was a problem hiding this comment.
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.
| if (row.Keys.Count != row.Values.Count) | ||
| { | ||
| throw new InvalidOperationException($"{nameof(row.Keys)} count should be equal to each {nameof(row.Values)} entry count"); |
There was a problem hiding this comment.
This validation is incorrect for Dictionary objects. Dictionary.Keys.Count will always equal Dictionary.Values.Count by definition. This check should validate that the dictionary is not empty instead.
| if (row.Keys.Count != row.Values.Count) | |
| { | |
| throw new InvalidOperationException($"{nameof(row.Keys)} count should be equal to each {nameof(row.Values)} entry count"); | |
| if (row.Count == 0) | |
| { | |
| throw new InvalidOperationException($"{nameof(row)} cannot be empty"); |
Copilot uses AI. Check for mistakes.
| protected virtual SqlResult CompileRemainingInsertClauses(SqlResult ctx, string table, IEnumerable<InsertClause> inserts) | ||
| protected virtual SqlResult CompileRemainingInsertClauses(SqlResult ctx, string table, IReadOnlyCollection<InsertClause> inserts) | ||
| { | ||
| var sql = new StringBuilder(ctx.RawSql, inserts.Count - 1); |
There was a problem hiding this comment.
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.
| var sql = new StringBuilder(ctx.RawSql, inserts.Count - 1); | |
| var sql = new StringBuilder(ctx.RawSql); |
Copilot uses AI. Check for mistakes.
| 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.
Same StringBuilder constructor issue as in Compiler.cs. The second parameter should be capacity, not a count calculation.
| var sql = new StringBuilder(ctx.RawSql, inserts.Count - 1); | |
| var sql = new StringBuilder(ctx.RawSql); |
Copilot uses AI. Check for mistakes.
1 participant
Hello, I noticed some performance issue with insert many on big collection.
It looks like there are few reasons
I fixed first two things, but I didn't change target frameworks, I think it would be good to compile sql kata as multiple frameworks, including .net core 3.1, .net 5.0 and .net 6.0
What I changed