Skip to content

feat: support persistent LIST properties for storage - #829

Open
zhanglei1949 wants to merge 4 commits into
alibaba:mainfrom
zhanglei1949:zl/feat-list-column
Open

feat: support persistent LIST properties for storage#829
zhanglei1949 wants to merge 4 commits into
alibaba:mainfrom
zhanglei1949:zl/feat-list-column

Conversation

@zhanglei1949

Copy link
Copy Markdown
Member

What do these changes do?

Add end-to-end support for persistent LIST<T> properties (recursively LIST / ARRAY / storable-scalar children) on vertex and non-bundled edge schemas. Supersedes #157.

  • Storage: new ListPropertyColumn with per-level offsets + lengths + elements. Same-length updates overwrite in place; length changes append; checkpoint dump compacts live spans only. Clone/Detach propagate to all three sub-columns for TP COW isolation.
  • Types / DDL: column factory branch, YAML encode (including the previously missing TIMESTAMP/INTERVAL branches), common-PB and reader type-converter round-trips for recursive LIST.
  • Ingestion: recursive list-like CSV parsing, recursive JSON LIST parsing (nested JSON nulls normalize to the declared child default), COPY into vertex and non-bundled edge tables.
  • Queries: explicit CAST between LIST and ARRAY with recursive per-level ARRAY length validation (implicit conversion remains unsupported), execution-side LIST validity (NULL vs empty list), result-sink validity bitmap and uint32 offset bound.
  • Semantics: top-level NULL persists as []; NULL children normalize to declared defaults; WAL reuses the existing recursive Value redo (no new opcode or proto field); LIST primary keys / indexes / ORDER BY are out of scope for this round.

Verified with a full build plus targeted suites: column lifecycle/resize/type-contract tests, JSON/YAML/PB round-trip tests, TP snapshot-isolation/abort/WAL-replay tests covering LIST on both vertices and edges, and Python end-to-end tests (AP create/set/merge + close-reopen, COPY from CSV/JSON including negative cases, explicit CAST contract).

Related issue number

Fixes #159
Part of #446

Copilot AI lite review requested due to automatic review settings August 6, 2026 02:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds end-to-end, persistent support for recursive LIST<T> properties (including nested LIST/ARRAY) across storage, type/DDL round-trips, ingestion, query casting, and result materialization—validated via expanded C++ and Python test coverage.

Changes:

  • Introduces a persistent storage layout for LIST via a new ListPropertyColumn and wires it into the column factory/reference columns.
  • Extends type round-trips (YAML + common PB) and ingestion (CSV/JSON) to correctly parse and normalize recursive list-like values.
  • Enables explicit CAST conversions between LIST and ARRAY (with recursive length validation) and ensures result sink correctness for optional LIST values and offset bounds.

Reviewed changes

Copilot reviewed 23 out of 23 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
tools/python_bind/tests/test_db_list.py Adds Python E2E tests for nested LIST persistence, COPY from CSV/JSON, and explicit CAST contracts.
tools/python_bind/tests/test_db_array.py Updates ARRAY tests to reflect explicit LIST↔ARRAY CAST behavior and validates conversions.
tests/utils/test_utils.cc Adds YAML/common-PB recursive LIST type round-trip coverage (including defaults).
tests/utils/json_test.cc Adds JSON reader tests for recursive LIST parsing and nested null normalization.
tests/unittest/test_column.cc Adds lifecycle/resize/type-contract tests for the new ListPropertyColumn.
tests/transaction/test_update_transaction.cc Adds TP snapshot/abort + WAL replay tests for nested LIST on vertices and edges.
tests/execution/test_value_column.cc Adds LIST/ARRAY nullability + shuffle/unfold execution column behavior tests.
src/utils/yaml_utils.cc Implements YAML encoding for LIST in schema/property YAML emission.
src/utils/property/list_property_column.cc Implements persistent LIST column storage (offsets/lengths/elements), dump/clone/detach semantics.
src/utils/property/column.cc Extends column factory/ref-column creation to support LIST.
src/utils/pb_utils.cc Adds common-PB → internal DataType::List parsing support.
src/utils/io/read/json/json_reader.cc Implements JSON parsing for LIST and nested-null normalization to defaults.
src/utils/io/read/common/type_converter.cc Adds internal↔common type conversions for recursive LIST.
src/storages/loader/loader_utils.cc Extends CSV parsing + COPY pipeline to support recursive list-like parsing for LIST.
src/storages/graph/edge_table.cc Enables copying nested (ARRAY/LIST) property values into edge storage for non-bundled edges.
src/execution/common/operators/retrieve/sink.cc Adds LIST validity bitmap support and guards response offsets for LIST results.
src/compiler/function/vector_cast_functions.cpp Enables explicit recursive LIST↔ARRAY casts and special-cases empty list literal binding.
src/compiler/function/cast/cast_array.cpp Extends validation rules for LIST↔ARRAY casts, including fixed-length checks for ARRAY targets.
src/compiler/binder/expression_binder.cpp Prevents folding ANY-typed constants so CAST can retype empty list literals correctly.
src/common/columns/list_columns.cc Updates LIST execution column shuffle/reorder behavior to preserve optional validity.
include/neug/utils/property/types.h Adds YAML encoding branches for TIMESTAMP/INTERVAL and recursive LIST types.
include/neug/utils/property/list_property_column.h Declares ListPropertyColumn and ListPropertyRefColumn.
include/neug/common/columns/list_columns.h Adds LIST nullability/validity semantics and stricter builder type checks.

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

Comment thread src/utils/property/list_property_column.cc
Comment thread src/utils/property/list_property_column.cc
@zhanglei1949 zhanglei1949 changed the title feat: support persistent LIST properties for vertices and edges feat: support persistent LIST properties for storage Aug 6, 2026
Copilot AI review requested due to automatic review settings August 6, 2026 03:28
Add ListPropertyColumn with per-level offsets/lengths/elements so
LIST<T> (recursively LIST/ARRAY/scalar) can be persisted on vertex
and non-bundled edge schemas. Wire it through the column factory,
YAML/PB/type-converter round-trips, CSV/JSON/COPY ingestion,
explicit LIST<->ARRAY CAST, execution-side LIST validity, and the
result sink. NULL writes normalize to declared defaults; checkpoint
dump compacts live spans only.

add doc revert changes to execution

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/utils/property/list_property_column.cc:178

  • ListPropertyColumn::Dump resizes the compact_elements column once per row (compact_elements->resize(tail + length)). For many rows this can cause repeated reallocations/initialization work. Precompute the total element count from lengths_ and resize once before copying.
    auto offset = offsets_->get_view(row);
    auto length = lengths_->get_view(row);
    compact_offsets.set_value(row, tail);
    compact_lengths.set_value(row, length);
    compact_elements->resize(tail + length);

src/utils/property/list_property_column.cc:284

  • ListPropertyColumn::set_any ignores the insert_safe contract when list length changes: it always appends and calls elements_->resize(...). Other variable-length columns (e.g., Varchar/Vec) throw when insert_safe is false, to avoid unsynchronized resizes. This can lead to unexpected resizes (and potential races) when callers pass insert_safe=false.
  auto new_offset = elements_->size();
  elements_->resize(new_offset + children.size());
  for (size_t i = 0; i < children.size(); ++i) {
    write_child(new_offset + i, children[i], true);
  }

Copilot AI review requested due to automatic review settings August 6, 2026 03:34

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/utils/property/list_property_column.cc:284

  • ListPropertyColumn::set_any() resizes/appends to the elements_ column even when insert_safe is false (length-changing update path). This breaks the ColumnBase insert_safe contract (callers may pass false assuming no resize/allocation and no external synchronization), and can lead to unexpected concurrent resizes.
  auto new_offset = elements_->size();
  elements_->resize(new_offset + children.size());
  for (size_t i = 0; i < children.size(); ++i) {
    write_child(new_offset + i, children[i], true);
  }

src/utils/property/list_property_column.cc:180

  • ListPropertyColumn::Dump() repeatedly calls compact_elements->resize(...) inside the row loop, which can cause many reallocations/copies during compaction (especially for large tables). Precomputing the total element count and resizing once avoids this overhead.
    compact_offsets.set_value(row, tail);
    compact_lengths.set_value(row, length);
    compact_elements->resize(tail + length);
    for (size_t i = 0; i < length; ++i) {
      compact_elements->set_any(tail + i, elements_->get_any(offset + i), true);

tools/python_bind/tests/test_db_list.py:112

  • The assertion on collect(p.tags) assumes a stable row order, but MATCH (p:Person) RETURN collect(p.tags) has no ordering guarantee and can be flaky. Add an ORDER BY (e.g., by primary key) before collecting.
    assert _nested_list(
        list(conn.execute("MATCH (p:Person) RETURN collect(p.tags);"))[0][0]
    ) == [["c", "d", "e"], ["merged"]]

tests/transaction/test_update_transaction.cc:2282

  • nested_type is declared but never used, which can trigger -Wunused-variable warnings (and may break builds if warnings are treated as errors). Remove it or use it.
  auto nested_type = neug::DataType::List(pair_type);

1. Dump(): precompute total element count and resize once before the
   copy loop, avoiding O(n^2) memcpy from repeated resize calls.

2. set_any(): respect the insert_safe contract — when list length
   changes and insert_safe is false, throw instead of silently
   resizing elements_.
Copilot AI review requested due to automatic review settings August 6, 2026 03:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (4)

tools/python_bind/tests/test_db_list.py:111

  • collect(p.tags) without an explicit ordering can be nondeterministic, which may make this assertion flaky if the engine changes scan order. Order the rows before collecting to make the result deterministic.
        list(conn.execute("MATCH (p:Person) RETURN collect(p.tags);"))[0][0]

tests/transaction/test_update_transaction.cc:2412

  • EXPECT_NE does not stop the test on failure, so ed_accessor.get_data(it) could dereference an end iterator if no edges are returned (undefined behavior). Use ASSERT_NE here to fail fast before dereferencing.
    auto it = edges.begin();
    EXPECT_NE(it, edges.end());
    return ed_accessor.get_data(it);

include/neug/storages/README.md:136

  • This description of insert_safe is narrower than the actual contract in ColumnBase/ListPropertyColumn: for LIST properties, a list-length change throws whenever a resize would be required, not only when there is "insufficient space". Adjust wording to match the API contract.
The `insert_safe` parameter controls resize behavior, inherited from the `ColumnBase` interface:
- When `false`, throws on insufficient space in the elements column.
- When `true`, the elements column is resized as needed; the caller must provide external

src/compiler/function/cast/cast_array.cpp:127

  • When casting LIST/ARRAY to a fixed-size ARRAY, a length mismatch currently throws a generic "Unsupported casting function" message, which is misleading. Emit a specific length-mismatch conversion error to help users diagnose bad CASTs.
    if (listEntry.size != ArrayType::GetNumElements(resultType)) {
      THROW_CONVERSION_EXCEPTION(
          stringFormat("Unsupported casting function from {} to {}.",
                       inputType.ToString(), resultType.ToString()));
    }

Copilot AI review requested due to automatic review settings August 6, 2026 04:14

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 22 out of 22 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/utils/property/list_property_column.cc:295

  • set_any() currently throws whenever the list length changes and insert_safe is false. Per ColumnBase::set_any contract, insert_safe=false should be fine as long as the write does not require resizing; shrinking a list (or setting it to empty) can be handled in-place by overwriting the prefix and updating lengths_ without appending/resizing. As-is, callers that pass insert_safe=false (e.g., some insert/WAL replay paths) will be unable to write non-empty LIST values at all unless they switch to insert_safe=true for growth cases.
  // List length changed: new elements must be appended to the tail of
  // elements_, which requires a resize. Respect the insert_safe contract:
  // when false, the caller expects no reallocation.
  if (!insert_safe) {
    THROW_STORAGE_EXCEPTION(

src/compiler/function/cast/cast_array.cpp:127

  • When casting LIST/ARRAY → fixed-size ARRAY, a length mismatch currently throws the generic "Unsupported casting function…" message. This is misleading because the cast is supported but the value length is invalid; it should report an ARRAY length mismatch (and ideally include expected vs actual sizes) to make debugging CAST failures easier.
    if (listEntry.size != ArrayType::GetNumElements(resultType)) {
      THROW_CONVERSION_EXCEPTION(
          stringFormat("Unsupported casting function from {} to {}.",
                       inputType.ToString(), resultType.ToString()));
    }

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.

Introduce support for List type in storage.

2 participants