Conversation
added 10 commits
May 25, 2025 21:59
Contributor
There was a problem hiding this comment.
Pull Request Overview
This pull request adds Oracle support to the bulk insert functionality. Key changes include:
- Adding Oracle-specific DbContext and test container classes.
- Introducing an Oracle bulk insert provider and its supporting classes (dialect builder, options, and DbContext options extension).
- Updating benchmarks, project files, and documentation to incorporate Oracle support.
Reviewed Changes
Copilot reviewed 26 out of 26 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/PhenX.EntityFrameworkCore.BulkInsert.Tests/DbContext/TestDbContext.cs | Removed an extraneous blank line and added Oracle DbContext. |
| tests/PhenX.EntityFrameworkCore.BulkInsert.Tests/DbContainer/TestDbContainerOracle.cs | Added an Oracle test container with connection configuration adjustments. |
| tests/PhenX.EntityFrameworkCore.BulkInsert.Benchmark/Providers/LibComparatorOracle.cs | Introduced a benchmark comparator for Oracle. |
| tests/PhenX.EntityFrameworkCore.BulkInsert.Benchmark/Program.cs | Updated benchmark runner to include Oracle tests. |
| tests/PhenX.EntityFrameworkCore.BulkInsert.Benchmark/PhenX.EntityFrameworkCore.BulkInsert.Benchmark.csproj | Included Oracle package reference and project reference. |
| tests/PhenX.EntityFrameworkCore.BulkInsert.Benchmark/LibComparator.cs | Added Oracle branch in raw insert logic. |
| tests/PhenX.EntityFrameworkCore.BulkInsert.Benchmark/LibComparator.RawInsert.cs | Added raw Oracle bulk insert implementation using OracleBulkCopy. |
| tests/PhenX.EntityFrameworkCore.BulkInsert/BulkInsertProviderBase.cs | Updated logic to conditionally skip adding an ID column for providers that do not require it. |
| src/PhenX.EntityFrameworkCore.BulkInsert/Extensions/InternalExtensions.cs | Adapted provider checking to support multiple provider types. |
| src/PhenX.EntityFrameworkCore.BulkInsert/Enums/ProviderType.cs | Extended enum to include Oracle. |
| src/PhenX.EntityFrameworkCore.BulkInsert.Sqlite/SqliteBulkInsertProvider.cs | Renamed temp table format to be more generic. |
| src/PhenX.EntityFrameworkCore.BulkInsert.Oracle/* | New Oracle provider classes, options, and dialect implementations. |
| README.md | Updated documentation to mention Oracle support and added relevant benchmarks. |
| PhenX.EntityFrameworkCore.BulkInsert.sln | Included the new Oracle project in the solution. |
Comments suppressed due to low confidence (1)
src/PhenX.EntityFrameworkCore.BulkInsert.Oracle/OracleBulkInsertProvider.cs:23
- [nitpick] Using '#' as a prefix in the generated temporary table name could conflict with Oracle naming conventions. Consider revising the naming to ensure compatibility with Oracle identifier rules.
protected override string GetTempTableName(string tableName) => "$#temp_bulk_insert_{Guid.NewGuid().ToString("N")[..8]}";
…rtProvider.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Collaborator
SebastianStehle
left a comment
SebastianStehle
left a comment
There was a problem hiding this comment.
LGTM. Only super small thing
|
|
||
| foreach (var column in columns) | ||
| { | ||
| q.Append($"{column.QuotedColumName} {column.StoreDefinition}"); |
Collaborator
There was a problem hiding this comment.
I think I have made an extension method for this case AppenJoined(",\n", column, (c, sb) => sb.Append("...")); or something like that
| protected override string AddTableCopyBulkInsertId => ""; // No need to add an ID column in Oracle | ||
|
|
||
| /// <inheritdoc /> | ||
| protected override string GetTempTableName(string tableName) => $"#temp_bulk_insert_{Guid.NewGuid().ToString("N")[..8]}"; |
Collaborator
There was a problem hiding this comment.
Why do you not take the whole guid?
Owner
Author
There was a problem hiding this comment.
oracle under a certain version (12 I think) does not allow indentifiers longer than 30, yay
Collaborator
There was a problem hiding this comment.
I would probably comment that, because I would have forgotten that next week.
Owner
Author
There was a problem hiding this comment.
oh I will remember 😅 but yeah I'll add a comment
Collaborator
There was a problem hiding this comment.
I ahve said this very often to myself :D
| { | ||
| /// <inheritdoc cref="OracleBulkCopyOptions"/> | ||
| public OracleBulkCopyOptions CopyOptions { get; set; } = OracleBulkCopyOptions.Default; | ||
|
|
|
|
||
| q.AppendLine(";"); | ||
|
|
||
| var result = q.ToString(); |
Collaborator
There was a problem hiding this comment.
Have you not got rid of this syntax in another PR
aka
return q.ToString();
| } | ||
| } | ||
|
|
||
| // No conflict handling |
Collaborator
There was a problem hiding this comment.
Perhaps two methods so that the comments are not necessary?
# Conflicts: # tests/PhenX.EntityFrameworkCore.BulkInsert.Benchmark/GetValueComparator.cs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #46
It is really partial for now, only supports the basic features, and not returning, even though there is some code to support it, in the future