V2/vt refl #1
V2/vt refl #1
Conversation
WalkthroughRefactors index handling and column management: columns track index usage, indexes are populated incrementally, index exclusion APIs added, unchecked column creation introduced, builder-based type factories replaced with direct TypeDescription construction, and TypeDescriptionBuilder now uses a SortedSet. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (1)
132-147: Use trimmed field name in error message.Line 142 throws an exception with the original
fieldNamethat includes whitespace, while the actual lookup uses the trimmedname. This could confuse users since they won't see the whitespace in error messages.🔎 Suggested fix
foreach (var fieldName in fieldNames) { var name = fieldName.Trim(); - var field = source.GetField(name) ?? throw ColumnException.WrongColumnName(fieldName); + var field = source.GetField(name) ?? throw ColumnException.WrongColumnName(name); fields.Add(field); }
🧹 Nitpick comments (5)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (1)
87-97: Consider extracting the adjusted value to reduce duplication.The
column.ValueType.AdjustValue(value)call is duplicated in both branches. Extracting it before the conditional would be slightly cleaner.🔎 Proposed refactor
public void Set(ValueTableColumn column, IValue value) { + var adjustedValue = column.ValueType.AdjustValue(value); if (column.IsIndexable) { _owner.Indexes.ElementRemoved(this); - _data[column] = column.ValueType.AdjustValue(value); + _data[column] = adjustedValue; _owner.Indexes.ElementAdded(this); } else - _data[column] = column.ValueType.AdjustValue(value); + _data[column] = adjustedValue; }src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
162-162: Minor formatting issue.There's an extra leading space on this line compared to standard indentation.
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (1)
38-50: Consider makingAddUncheckedmethods internal or documenting the risks.These public methods bypass name validation and duplicate checking, which could allow invalid identifiers or duplicate columns. While the "Unchecked" name signals intent, exposing this as a public API is risky.
Consider:
- Making these methods
internalif they're only for internal use (e.g., Reflector)- Adding strong XML documentation comments warning callers about validation bypass
Option 1: Make methods internal
-public ValueTableColumn AddUnchecked(string name, string title, TypeDescription type = null) +internal ValueTableColumn AddUnchecked(string name, string title, TypeDescription type = null) { var column = new ValueTableColumn(this, name, title, type, 0); _columns.Add(column); return column; } -public ValueTableColumn AddUnchecked(string name, string title = null) +internal ValueTableColumn AddUnchecked(string name, string title = null) { var column = new ValueTableColumn(this, name, title ?? name, null, 0); _columns.Add(column); return column; }Option 2: Add XML documentation warning
+/// <summary> +/// Adds a column without validation. Use with caution. +/// </summary> +/// <remarks> +/// This method does NOT validate the column name or check for duplicates. +/// Only use when you are certain the name is valid and unique. +/// </remarks> public ValueTableColumn AddUnchecked(string name, string title, TypeDescription type = null)src/OneScript.StandardLibrary/Reflector.cs (1)
176-205: Consider usingAddUncheckedfor consistency.Lines 179-180 still use
Addfor the parameter table columns, while the annotation table (line 150-151) usesAddUnchecked. For consistency and performance, consider usingAddUncheckedhere as well since the column names are hardcoded literals.🔎 Suggested change
var parametersTable = new ValueTable(); -var parameterNameColumn = parametersTable.Columns.Add("Имя"); -var parameterValueColumn = parametersTable.Columns.Add("Значение"); +var parameterNameColumn = parametersTable.Columns.AddUnchecked("Имя"); +var parameterValueColumn = parametersTable.Columns.AddUnchecked("Значение");src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (1)
85-89: Consider consolidating multiple Any() calls into a single iteration.The method performs four separate
Any()calls, each iterating through_types. While this works correctly and is likely acceptable for small type sets, you could optimize by iterating once:🔎 Optional optimization to reduce iterations
public TypeDescription Build() { - var hasNumber = _types.Any(type => type.TypeValue == BasicTypes.Number); - var hasString = _types.Any(type => type.TypeValue == BasicTypes.String); - var hasDate = _types.Any(type => type.TypeValue == BasicTypes.Date); - var hasBinaryData = _types.Any(x => x.TypeValue.Name == TYPE_BINARYDATA_NAME); + bool hasNumber = false, hasString = false, hasDate = false, hasBinaryData = false; + foreach (var type in _types) + { + if (type.TypeValue == BasicTypes.Number) hasNumber = true; + else if (type.TypeValue == BasicTypes.String) hasString = true; + else if (type.TypeValue == BasicTypes.Date) hasDate = true; + else if (type.TypeValue.Name == TYPE_BINARYDATA_NAME) hasBinaryData = true; + } if (!hasNumber || _numberQualifiers == null) _numberQualifiers = new NumberQualifiers();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs(8 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs(1 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs(4 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs(2 hunks)src/OneScript.StandardLibrary/Reflector.cs(18 hunks)src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cs(1 hunks)src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs(1 hunks)src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (7)
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (1)
Count(120-123)src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (7)
ColumnException(13-53)ColumnException(15-18)ColumnException(20-22)ColumnException(24-29)ColumnException(31-36)ColumnException(38-43)ColumnException(46-51)
src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (2)
src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cs (2)
TypeComparer(14-64)TypeComparer(54-62)src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs (1)
IEnumerable(79-82)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (3)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)
ValueTableColumn(29-38)src/OneScript.Core/Commons/Utils.cs (1)
Utils(12-54)src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (7)
ColumnException(13-53)ColumnException(15-18)ColumnException(20-22)ColumnException(24-29)ColumnException(31-36)ColumnException(38-43)ColumnException(46-51)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (3)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
ValueTableColumn(319-321)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (1)
ValueTableColumn(146-149)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (6)
ValueTableColumn(38-43)ValueTableColumn(45-50)ValueTableColumn(167-170)ValueTableColumn(172-177)ValueTableColumn(221-240)ValueTableColumnCollection(33-36)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (5)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (3)
Count(120-123)ElementAdded(73-84)ToString(53-56)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (4)
Count(27-27)Count(29-29)ValueTableRow(22-25)ValueTableColumn(146-149)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (6)
Count(122-122)ValueTableColumn(38-43)ValueTableColumn(45-50)ValueTableColumn(167-170)ValueTableColumn(172-177)ValueTableColumn(221-240)src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (1)
ElementAdded(111-117)src/OneScript.StandardLibrary/Collections/Exceptions/ColumnException.cs (7)
ColumnException(13-53)ColumnException(15-18)ColumnException(20-22)ColumnException(24-29)ColumnException(31-36)ColumnException(38-43)ColumnException(46-51)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndexKey.cs (3)
CollectionIndexKey(15-82)CollectionIndexKey(20-24)CollectionIndexKey(37-48)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (1)
ValueTableColumn(29-38)
src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs (6)
src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (1)
TypeDescription(83-102)src/OneScript.Core/Values/BslTypeValue.cs (3)
BslTypeValue(12-54)BslTypeValue(16-19)BslTypeValue(21-21)src/OneScript.StandardLibrary/TypeDescriptions/NumberQualifiers.cs (2)
NumberQualifiers(21-28)ScriptConstructor(92-104)src/OneScript.StandardLibrary/TypeDescriptions/StringQualifiers.cs (2)
StringQualifiers(19-24)ScriptConstructor(84-89)src/OneScript.StandardLibrary/TypeDescriptions/DateQualifiers.cs (2)
DateQualifiers(20-23)ScriptConstructor(70-74)src/OneScript.StandardLibrary/TypeDescriptions/BinaryDataQualifiers.cs (2)
BinaryDataQualifiers(18-23)ScriptConstructor(50-54)
🔇 Additional comments (31)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (2)
24-27: LGTM!Making
_valueTypereadonly improves immutability, and the newIsIndexableproperty with an internal setter provides appropriate encapsulation for index-tracking functionality.
29-38: LGTM!The constructor signature simplification (removing unused
idparameter) and initialization ofIsIndexable = falseare appropriate. The column will be marked as indexable when added to aCollectionIndex.src/OneScript.StandardLibrary/TypeDescriptions/TypeDescription.cs (3)
157-163: LGTM!Direct instantiation is cleaner and produces the same result as the builder pattern. The qualifier parameters are correctly ordered to match the internal constructor signature.
167-173: LGTM!The direct instantiation correctly sets up an integer type with the specified
NumberQualifiers(length, 0, allowedSign)for zero fractional digits.
176-184: LGTM!The boolean type factory and the
[ScriptConstructor]attribute placement are correct.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (1)
27-28: LGTM!Clean conversion to expression-bodied member.
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (8)
38-39: LGTM!Minor formatting change.
69-76: LGTM!The conditional check
Indexes.Count() > 0before callingIndexes.ElementAdded(row)is a sensible optimization to avoid unnecessary work when no indexes exist.
85-96: Verify the Insert boundary behavior matches expectations.The condition
index < 0 || index > _rows.Countwill cause indices equal to_rows.Count(inserting at the end) to go through the normal insert path, which is correct. However, indices greater than_rows.Countnow silently fall back toAdd()for compatibility. Ensure this matches the documented 1C behavior.
317-322: LGTM!Good DRY improvement. The
GetColumnOrThrowhelper consolidates the pattern of finding a column and throwingColumnException.WrongColumnNameif not found.
328-328: LGTM!Clean usage of the new helper method.
351-351: LGTM!The null-conditional operator provides a more concise fallback to
_rowswhen no suitable index exists.
368-368: LGTM!Consistent use of
GetColumnOrThrow.
657-657: LGTM!Consistent use of
GetColumnOrThrowin sort rule parsing.src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (3)
21-25: LGTM!Using the concrete
Dictionarytype instead ofIDictionaryis fine for private fields and can enable minor optimizations.
43-46: LGTM!The logic is correct: an index can be used for a search if all the index's fields are present in the search criteria. This allows an index on columns
{A, B}to be used when searching by{A, B, C}.
95-95: LGTM!Clean conversion to expression-bodied member.
src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs (1)
42-42: LGTM: Property access is more idiomatic.Using the
Countproperty instead of theCount()method is the standard pattern forList<T>.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (4)
28-28: LGTM: Static comparer improves performance.Using a static
StringComparer.OrdinalIgnoreCaseis more efficient than creating comparison logic per-instance.
53-60: LGTM: Good refactoring to centralize validation.The
CheckColumnNamehelper eliminates code duplication and ensures consistent validation acrossAddandInsertmethods.
71-79: LGTM: Cleaner validation usingCheckColumnName.Both
AddandInsertmethods now use the centralizedCheckColumnNamehelper, improving maintainability and consistency.Also applies to: 91-99
130-133: LGTM: Cleaner implementation with null-coalescing.The simplified
Findmethod preserves the original behavior (returning Undefined when not found) while being more concise.src/OneScript.StandardLibrary/Reflector.cs (5)
145-146: LGTM: Constants improve maintainability.Using named constants for column indices eliminates magic numbers and makes the code more maintainable.
147-154: LGTM:AddUncheckedis appropriate for internal table construction.Using
AddUncheckedfor hardcoded column names in internal tables is safe and provides a performance benefit by skipping validation.
156-174: LGTM: Index-based access is more efficient and reliable.Using
FindColumnByIndexwith constants instead of name-based lookup improves performance and eliminates the risk of typos in column names.
349-394: LGTM: Consistent use ofAddUncheckedfor performance.Using
AddUncheckedthroughout the method table construction is appropriate and provides consistent performance benefits for internal table creation.
458-462: LGTM: Consistent pattern for internal table construction.The use of
AddUncheckedfor properties and known types tables follows the same safe pattern established elsewhere in the file.Also applies to: 534-542
src/OneScript.StandardLibrary/TypeDescriptions/TypeComparer.cs (1)
17-17: LGTM: Type narrowing improves clarity.Narrowing from
IDictionarytoDictionaryfor this private field is appropriate and improves type clarity without affecting behavior.src/OneScript.StandardLibrary/TypeDescriptions/TypeDescriptionBuilder.cs (3)
24-25: LGTM: SortedSet improves performance and maintainability.Replacing the collection with
SortedSet<BslTypeValue>is a beneficial refactor that provides automatic ordering and deduplication, eliminating the need for explicit sorting and deduplication logic inBuild().
52-55: LGTM: RemoveTypes implementation is correct.The implementation correctly removes each type from the SortedSet with O(log n) complexity per removal.
42-46: Add a comment explaining why BslUndefinedValue types are filtered in AddTypes().The new filtering logic at line 44 silently excludes types with
ImplementingClass == typeof(BslUndefinedValue)without explanation. SinceBasicTypes.Undefinedis a valid type in the system andBslTypeValue(BasicTypes.Undefined)can be created elsewhere in the codebase (e.g., in XDTOSerializer), the reason for this exclusion should be documented. Additionally, note the asymmetry:RemoveTypes()does not apply this filter, which could lead to unexpected behavior if undefined types are somehow included and later removal is attempted.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs(3 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs(2 hunks)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/OneScript.StandardLibrary/Collections/ValueTable/CollectionIndexes.cs
🧰 Additional context used
🧬 Code graph analysis (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (1)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (3)
ValueTableColumn(32-40)AddToIndex(29-29)DeleteFromIndex(30-30)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (3)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableRow.cs (1)
ValueTableColumn(146-149)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumnCollection.cs (6)
ValueTableColumn(38-43)ValueTableColumn(45-50)ValueTableColumn(167-170)ValueTableColumn(172-177)ValueTableColumn(221-240)ValueTableColumnCollection(33-36)src/OneScript.StandardLibrary/Collections/ValueTable/ValueTable.cs (1)
ValueTableColumn(319-321)
🔇 Additional comments (6)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (4)
43-46: LGTM: CanBeUsedFor logic is clear and correct.The updated containment check correctly verifies that all index fields are present in the search fields, with an appropriate guard against empty field lists.
110-110: LGTM: Expression-bodied method is appropriate here.The conversion to expression-bodied syntax is idiomatic and clear for this one-line method.
27-41: Verify cleanup logic when deleting collection indexes.The constructor properly calls
AddToIndex()forValueTableColumnfields to track index participation. Ensure that when an index is deleted viaCollectionIndexes.Delete()orCollectionIndexes.Clear(), the correspondingDeleteFromIndex()is called on eachValueTableColumnto maintain accurate tracking. Without this cleanup,_indicesCountcould become incorrect, leading to resource leaks or incorrect index state management.
64-77: Prevent duplicate fields from being added to the indexThe
FieldAddedmethod (line 33) unconditionally adds fields to_fieldswithout checking for duplicates. This allows the same field to be added multiple times, which necessitates the while loop inFieldRemovedto remove all instances. Consider validating inFieldAddedwhether a field already exists before adding it, or document this behavior if duplicate fields are intentional.src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (2)
32-40: LGTM: Constructor signature simplified appropriately.The removal of the
idparameter simplifies the constructor and aligns with theAddUncheckedmethods shown inValueTableColumnCollection.cs, which don't use id-based tracking. The initialization logic is correct.
24-24: LGTM: Making _valueType readonly improves immutability.Marking
_valueTypeasreadonlyprevents accidental modification after construction, which is appropriate since a column's type description should not change after the column is created.
| internal void ExcludeFields() | ||
| { | ||
| foreach (var field in _fields) | ||
| { | ||
| if (field is ValueTable.ValueTableColumn column) | ||
| column.AddToIndex(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Method name contradicts its behavior.
The method ExcludeFields() calls AddToIndex() on each ValueTableColumn, which increments the index count. The name suggests removal or exclusion, but the implementation performs inclusion. This naming mismatch will confuse maintainers.
Consider renaming to IncludeFields(), ReactivateFields(), or similar, or clarify the intended semantics if "exclude" refers to a different operation context.
🔎 Proposed fix: Rename to match behavior
-internal void ExcludeFields()
+internal void IncludeFields()
{
foreach (var field in _fields)
{
if (field is ValueTable.ValueTableColumn column)
column.AddToIndex();
}
}🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs around
lines 79 to 86, the method ExcludeFields() actually calls AddToIndex() on each
ValueTableColumn (which increments/activates index counts) so its name
contradicts behavior; rename the method to a name matching intent (e.g.,
IncludeFields or ReactivateFields), update the method declaration and all
call-sites, update any XML docs/comments and unit tests to reflect the new name,
and run a build/grep to ensure no remaining references to ExcludeFields remain.
| private int _indicesCount = 0; | ||
| public bool IsIndexable => _indicesCount != 0; | ||
| public void AddToIndex() { _indicesCount++; } | ||
| public void DeleteFromIndex() { if (_indicesCount != 0) _indicesCount--; } |
There was a problem hiding this comment.
Asymmetric AddToIndex/DeleteFromIndex can cause index-count drift.
AddToIndex() unconditionally increments _indicesCount, but DeleteFromIndex() stops at zero. If these methods are not called in perfectly matching pairs—for example, if an index is added twice to the same column or deletion fails midway—the count can drift upward. This will cause IsIndexable to remain true even after all legitimate indexes are removed, leading to unnecessary index operations on every row update (as seen in ValueTableRow.Set() at lines 89-93).
Additionally, the increment and decrement operations are not thread-safe, which could cause race conditions if indexes are modified concurrently.
🔎 Proposed fix: Add symmetry validation and thread safety
private int _indicesCount = 0;
+private readonly object _lockObject = new object();
+
public bool IsIndexable => _indicesCount != 0;
-public void AddToIndex() { _indicesCount++; }
-public void DeleteFromIndex() { if (_indicesCount != 0) _indicesCount--; }
+
+public void AddToIndex()
+{
+ lock (_lockObject)
+ {
+ _indicesCount++;
+ }
+}
+
+public void DeleteFromIndex()
+{
+ lock (_lockObject)
+ {
+ if (_indicesCount == 0)
+ throw new InvalidOperationException("Cannot delete from index: no indexes are tracking this column.");
+ _indicesCount--;
+ }
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private int _indicesCount = 0; | |
| public bool IsIndexable => _indicesCount != 0; | |
| public void AddToIndex() { _indicesCount++; } | |
| public void DeleteFromIndex() { if (_indicesCount != 0) _indicesCount--; } | |
| private int _indicesCount = 0; | |
| private readonly object _lockObject = new object(); | |
| public bool IsIndexable => _indicesCount != 0; | |
| public void AddToIndex() | |
| { | |
| lock (_lockObject) | |
| { | |
| _indicesCount++; | |
| } | |
| } | |
| public void DeleteFromIndex() | |
| { | |
| lock (_lockObject) | |
| { | |
| if (_indicesCount == 0) | |
| throw new InvalidOperationException("Cannot delete from index: no indexes are tracking this column."); | |
| _indicesCount--; | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs
around lines 27-30, the AddToIndex/DeleteFromIndex pair can let _indicesCount
drift upward and is not thread-safe; change both methods to use thread-safe
atomic operations (System.Threading.Interlocked) and enforce symmetry by
preventing _indicesCount from going negative or growing incorrectly: use
Interlocked.Increment in AddToIndex; in DeleteFromIndex use an atomic loop
(CompareExchange) to decrement only if current > 0 or perform
Interlocked.Decrement and if the result is negative, correct it back to zero
with Interlocked.Exchange (and optionally log the unexpected state). This
ensures counts remain non-negative and safe under concurrency so IsIndexable
reflects the true state.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (3)
45-45: Consider optimizing theContainslookup.The method iterates
_fieldsand callsContainson thesearchFieldsenumerable for each field. IfsearchFieldsis a large list, this becomes O(n×m). Consider convertingsearchFieldsto aHashSetfirst if this method is called frequently in hot paths.🔎 Proposed optimization
internal bool CanBeUsedFor(IEnumerable<IValue> searchFields) { - return _fields.Count > 0 && _fields.All(f => searchFields.Contains(f)); + if (_fields.Count == 0) + return false; + var searchFieldsSet = searchFields as HashSet<IValue> ?? searchFields.ToHashSet(); + return _fields.All(f => searchFieldsSet.Contains(f)); }
64-77: The while-loop with Contains is inefficient for multiple duplicates.The
while (_fields.Contains(field))pattern results in O(n²) behavior when removing multiple duplicate fields, sinceContainsis O(n) and is called repeatedly. Consider collecting all indices first and removing in reverse order, or usingRemoveAll.🔎 Proposed optimization
internal void FieldRemoved(IValue field) { - if (_fields.Contains(field)) + int removalCount = 0; + for (int i = _fields.Count - 1; i >= 0; i--) { - while (_fields.Contains(field)) + if (_fields[i].Equals(field)) { - if (field is ValueTable.ValueTableColumn column) - column.DeleteFromIndex(); - - _fields.Remove(field); + _fields.RemoveAt(i); + removalCount++; } + } + + if (removalCount > 0) + { + if (field is ValueTable.ValueTableColumn column) + { + for (int i = 0; i < removalCount; i++) + column.DeleteFromIndex(); + } Rebuild(); } }
79-86: Consider a clearer method name for cleanup semantics.The
ExcludeFields()method decrements column index counts without modifying_fieldsor rebuilding_data. This suggests it's cleanup logic called during index disposal. The name "Exclude" is ambiguous—it could suggest partial removal of some fields. Consider renaming toDetachFromColumns(),UnregisterFromColumns(), orCleanup()to better reflect its purpose.🔎 Proposed rename
-internal void ExcludeFields() +internal void DetachFromColumns() { foreach (var field in _fields) { if (field is ValueTable.ValueTableColumn column) column.DeleteFromIndex(); } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (1)
src/OneScript.StandardLibrary/Collections/ValueTable/ValueTableColumn.cs (3)
ValueTableColumn(32-40)AddToIndex(29-29)DeleteFromIndex(30-30)
🔇 Additional comments (2)
src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs (2)
24-25: Dictionary type made more concrete.The field
_datawas changed fromIDictionarytoDictionary. This removes a layer of abstraction. For an internal field with no evidence of multiple implementations, this is acceptable, though it slightly reduces flexibility for testing with mock dictionaries.
110-110: LGTM!The conversion to expression-bodied syntax is appropriate for this simple one-liner and follows modern C# conventions.
| public CollectionIndex(IIndexCollectionSource source, IEnumerable<IValue> fields) | ||
| { | ||
| foreach (var field in fields) | ||
| { | ||
| if (field is ValueTable.ValueTableColumn column) | ||
| column.AddToIndex(); | ||
| _fields.Add(field); | ||
| } | ||
|
|
||
| _source = source; | ||
| _fields.AddRange(fields); | ||
| foreach (var value in _source) | ||
| { | ||
| ElementAdded(value); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for CollectionIndex constructor call sites and duplicate field validation
echo "=== CollectionIndex constructor call sites ==="
rg -n "new CollectionIndex\(" -A 2 -B 2 -t cs
echo ""
echo "=== CollectionIndex class definition ==="
rg -n "class CollectionIndex" -A 20 -t cs | head -40
echo ""
echo "=== AddToIndex method definition ==="
rg -n "AddToIndex" -B 2 -A 5 -t cs
echo ""
echo "=== Check _fields field initialization and usage ==="
rg -n "_fields" -B 1 -A 1 -t cs | head -50Repository: Mr-Rm/OneScript
Length of output: 11094
🏁 Script executed:
#!/bin/bash
# Need to examine BuildFieldList and understand duplicate handling
echo "=== BuildFieldList method ==="
rg -n "BuildFieldList" -B 2 -A 15 -t cs
echo ""
echo "=== CollectionIndexKey.Extract method ==="
rg -n "Extract" -B 2 -A 20 -t cs src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndexKey.csRepository: Mr-Rm/OneScript
Length of output: 4888
🏁 Script executed:
#!/bin/bash
# Check for tests or comments about duplicate fields in indexes
echo "=== Search for test files related to CollectionIndex ==="
fd -t f "*.cs" -i "*test*" | xargs rg -l "CollectionIndex|BuildFieldList" 2>/dev/null | head -10
echo ""
echo "=== Look for any comments about duplicates near field handling ==="
rg -n "duplicate|forbid|prevent|unique" -i -B 2 -A 2 -t cs src/OneScript.StandardLibrary/Collections/Indexes/ src/OneScript.StandardLibrary/Collections/ValueTable/
echo ""
echo "=== Check CollectionIndex constructor more carefully ==="
sed -n '27,41p' src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.csRepository: Mr-Rm/OneScript
Length of output: 3384
Prevent duplicate fields in the constructor with validation.
The constructor accepts duplicate fields in the IEnumerable<IValue> fields parameter without validation. If the same column appears multiple times, AddToIndex() will be called multiple times, inflating the _indicesCount counter. While CollectionIndexKey.Extract() uses Dictionary.TryAdd() to silently ignore duplicate keys (and RemoveField() handles duplicates symmetrically), allowing duplicates creates unnecessary confusion and overhead. Add validation in BuildFieldList() to reject duplicate field names, or add a check in the constructor to throw if duplicates are detected.
🤖 Prompt for AI Agents
In src/OneScript.StandardLibrary/Collections/Indexes/CollectionIndex.cs around
lines 27-41, the constructor currently accepts duplicate fields and will call
AddToIndex() multiple times which inflates _indicesCount; update the constructor
(or BuildFieldList called by it) to validate and reject duplicate field names:
collect field identifiers (e.g. column name for ValueTableColumn or a stable key
for IValue), if a duplicate is found throw an ArgumentException (with a clear
message including the duplicate name) rather than adding it, and ensure
AddToIndex() is only invoked once per unique column (iterate the deduplicated
list when calling AddToIndex and when adding to _fields).
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.