Skip to content

fix: disable resizing during the INSERT transaction#283

Merged
liulx20 merged 11 commits into
alibaba:mainfrom
liulx20:resize
Apr 23, 2026
Merged

fix: disable resizing during the INSERT transaction#283
liulx20 merged 11 commits into
alibaba:mainfrom
liulx20:resize

Conversation

@liulx20
Copy link
Copy Markdown
Collaborator

@liulx20 liulx20 commented Apr 23, 2026

This pull request refactors several method signatures in the storage layer to require explicit specification of the insert_safe parameter, removing its default value. This change enforces clarity and consistency when inserting or updating data in tables and columns, and ensures that all call sites and tests explicitly specify whether insert operations should be performed safely (with buffer resizing or checks). Additionally, error handling is improved for unsafe insertions when buffer space is insufficient.

Fixes

@liulx20 liulx20 changed the title disable resizing during the INSERT transaction fix: disable resizing during the INSERT transaction Apr 23, 2026
@liulx20 liulx20 requested a review from Copilot April 23, 2026 04:45
@liulx20 liulx20 linked an issue Apr 23, 2026 that may be closed by this pull request
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enforces explicit insert_safe semantics across the storage/property layer so callers must choose whether inserts/updates may resize buffers, and it adjusts loader/runtime code and tests accordingly to avoid implicit resizing during insert transactions.

Changes:

  • Removed default values for insert_safe in Table::insert, ColumnBase::set_any, VertexTable::AddVertex, and EdgeTable::AddEdge, updating call sites in tests/benchmarks and loader code.
  • Updated TypedColumn<std::string_view>::set_any to only resize when insert_safe=true, otherwise throwing when buffer space is insufficient.
  • Updated VertexTable::UpdateProperty to call set_any(..., /*insert_safe=*/true).

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
tests/utils/test_table.cc Updates Table::insert call to pass insert_safe explicitly.
tests/storage/vertex_table_benchmark.cc Updates VertexTable::AddVertex calls to pass explicit ts and insert_safe.
tests/storage/test_vertex_table.cc Updates VertexTable::AddVertex calls to pass explicit ts and insert_safe.
tests/storage/test_edge_table.cc Updates EdgeTable::AddEdge calls to pass insert_safe explicitly.
src/storages/loader/loader_utils.cc Updates ColumnBase::set_any call sites to pass insert_safe explicitly (mostly false).
src/storages/graph/vertex_table.cc Uses explicit insert_safe in set_any for updates.
include/neug/utils/property/table.h Removes default insert_safe from Table::insert declaration.
include/neug/utils/property/column.h Removes default insert_safe from ColumnBase::set_any and enforces it for varchar storage (resize vs throw).
include/neug/storages/graph/vertex_table.h Removes defaults from VertexTable::AddVertex signature.
include/neug/storages/graph/edge_table.h Removes default from EdgeTable::AddEdge signature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread include/neug/storages/graph/vertex_table.h
Comment thread src/storages/graph/vertex_table.cc
Comment thread include/neug/utils/property/column.h
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR enforces explicit insert_safe selection across the storage/property layer APIs (removing default arguments) to make insert/update behavior unambiguous, and tightens behavior for unsafe inserts when backing buffers are full.

Changes:

  • Remove default values for insert_safe in several insert/update APIs and update call sites/tests to pass it explicitly.
  • Change TypedColumn<std::string_view>::set_any to resize only when insert_safe=true, otherwise throw when buffer space is insufficient.
  • Refactor LFIndexer APIs (notably init and insert) and update related tests.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
tests/utils/test_table.cc Updates table insert call to explicitly pass insert_safe.
tests/unittest/test_indexer.cc Updates indexer inserts and varchar init usage to align with explicit safety behavior.
tests/storage/vertex_table_benchmark.cc Updates benchmark vertex insertions to pass insert_safe explicitly.
tests/storage/test_vertex_table.cc Updates vertex insertion test calls to pass timestamp + insert_safe explicitly.
tests/storage/test_edge_table.cc Updates edge insertions and indexer insert calls to pass insert_safe explicitly.
src/storages/loader/loader_utils.cc Passes insert_safe=false when setting properties during load for non-resizing paths.
src/storages/graph/vertex_table.cc Updates property updates to explicitly request safe insertion for column writes.
include/neug/utils/property/table.h Removes default insert_safe from Table::insert signature.
include/neug/utils/property/column.h Removes default insert_safe from ColumnBase::set_any; enforces throw-on-full when unsafe for varchar.
include/neug/utils/id_indexer.h Refactors LFIndexer::init signature and makes insert_safe explicit for inserts.
include/neug/storages/graph/vertex_table.h Removes default args from VertexTable::AddVertex and internal pk insertion helper.
include/neug/storages/graph/edge_table.h Removes default insert_safe from EdgeTable::AddEdge signature.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread include/neug/utils/id_indexer.h Outdated
Comment thread include/neug/utils/id_indexer.h
Comment thread include/neug/storages/graph/vertex_table.h
Comment thread src/storages/loader/loader_utils.cc
Comment thread include/neug/utils/property/column.h
}
for (const auto& v : long_values) {
indexer.insert(Property::from_string_view(v));
indexer.insert(Property::from_string_view(v), true);
Copy link
Copy Markdown
Collaborator Author

@liulx20 liulx20 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs resize here? @zhanglei1949

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

}
for (const auto& v : long_values) {
indexer.insert(Property::from_string_view(v));
indexer.insert(Property::from_string_view(v), true);
Copy link
Copy Markdown
Collaborator Author

@liulx20 liulx20 Apr 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs resize here? @zhanglei1949

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes.

@liulx20 liulx20 requested a review from zhanglei1949 April 23, 2026 08:17
Copy link
Copy Markdown
Member

@zhanglei1949 zhanglei1949 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@liulx20 liulx20 merged commit 82fcd99 into alibaba:main Apr 23, 2026
16 checks passed
@lnfjpt lnfjpt mentioned this pull request May 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] Can't resizing during the INSERT transaction.

3 participants