From 2e444a01312c86d4f4126f6f7f1675ab10676a8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Mon, 24 Aug 2026 08:41:51 +0200 Subject: [PATCH 01/13] perf: stream MTG rows without materializing tables --- benchmark/README.md | 10 ++ benchmark/write_mtg_streaming.jl | 201 ++++++++++++++++++++++++ src/write_mtg/write_mtg.jl | 247 +++++++++++++++++++++++++----- test/test-write_mtg.jl | 254 +++++++++++++++++++++++++++++++ 4 files changed, 670 insertions(+), 42 deletions(-) create mode 100644 benchmark/write_mtg_streaming.jl diff --git a/benchmark/README.md b/benchmark/README.md index 22fc6ec1..17430ee7 100644 --- a/benchmark/README.md +++ b/benchmark/README.md @@ -8,6 +8,16 @@ Run the benchmark suite locally: julia --project=benchmark benchmark/benchmarks.jl ``` +Run the byte-parity, allocation, and timing gates for the streaming MTG writer: + +```bash +julia --project=benchmark benchmark/write_mtg_streaming.jl +``` + +The writer gate exercises 40 features at 1,000 and 10,000 nodes. It compares the +streaming path with the compatibility materialization path, measures seven alternating +samples after warmup, and checks linear scaling. + Workloads currently covered: - tiered datasets: `small` (~10k nodes), `medium` (~100k), `large` (~300k) diff --git a/benchmark/write_mtg_streaming.jl b/benchmark/write_mtg_streaming.jl new file mode 100644 index 00000000..a32a5607 --- /dev/null +++ b/benchmark/write_mtg_streaming.jl @@ -0,0 +1,201 @@ +using MultiScaleTreeGraph +using Logging: NullLogger, with_logger + +function _write_mtg_median(values::Vector{Float64}) + sorted = sort(values) + middle = length(sorted) ÷ 2 + return isodd(length(sorted)) ? sorted[middle + 1] : + (sorted[middle] + sorted[middle + 1]) / 2 +end + +function _write_mtg_performance_fixture(n_nodes::Int; n_features::Int=40) + feature_names = [Symbol("feature_$(lpad(i, 2, '0'))") for i in 1:n_features] + feature_types = [i % 4 == 1 ? "REAL" : i % 4 == 2 ? "INT" : + i % 4 == 3 ? "BOOLEAN" : "STRING" for i in 1:n_features] + + function attributes_for(id::Int) + attrs = Dict{Symbol,Any}() + @inbounds for i in eachindex(feature_names) + attrs[feature_names[i]] = if feature_types[i] == "REAL" + id + i / 100 + elseif feature_types[i] == "INT" + id * i + elseif feature_types[i] == "BOOLEAN" + isodd(id + i) + else + "n$(id)-f$(i)" + end + end + return attrs + end + + root = Node( + 1, + MutableNodeMTG(:/, :Plant, 1, 1), + attributes_for(1), + ) + nodes = Vector{typeof(root)}(undef, n_nodes) + nodes[1] = root + @inbounds for id in 2:n_nodes + parent_ = nodes[id ÷ 2] + node_symbol = id % 3 == 0 ? :Leaf : :Internode + node_link = id % 3 == 0 ? :+ : :< + node_scale = id % 3 == 0 ? 3 : 2 + nodes[id] = Node( + id, + parent_, + MutableNodeMTG(node_link, node_symbol, id, node_scale), + attributes_for(id), + ) + end + + features = MultiScaleTreeGraph.ColumnTable( + Symbol[:NAME, :TYPE], + AbstractVector[feature_names, feature_types], + ) + return (root=root, classes=get_classes(root), features=features) +end + +function _write_mtg_materialized(file, data) + open(file, "w") do io + MultiScaleTreeGraph.writedlm(io, ["CODE:" "FORM-A"]) + MultiScaleTreeGraph.writedlm(io, [""]) + MultiScaleTreeGraph.writedlm(io, ["CLASSES:"]) + MultiScaleTreeGraph.writedlm( + io, reshape(String.(names(data.classes)), (1, :)) + ) + classes_print = copy(data.classes) + symbols_ = String.(classes_print.SYMBOL) + replace!(symbols_, "Scene" => "\$") + classes_print.SYMBOL = symbols_ + MultiScaleTreeGraph._write_table_rows(io, classes_print) + + MultiScaleTreeGraph.writedlm(io, [""]) + MultiScaleTreeGraph.writedlm(io, ["DESCRIPTION:"]) + MultiScaleTreeGraph.writedlm(io, ["LEFT" "RIGHT" "RELTYPE" "MAX"]) + + MultiScaleTreeGraph.writedlm(io, [""]) + MultiScaleTreeGraph.writedlm(io, ["FEATURES:"]) + MultiScaleTreeGraph.writedlm( + io, reshape(String.(names(data.features)), (1, :)) + ) + MultiScaleTreeGraph._write_table_rows(io, data.features) + + MultiScaleTreeGraph.writedlm(io, [""]) + MultiScaleTreeGraph.writedlm(io, ["MTG:"]) + attributes, column_names = MultiScaleTreeGraph.paste_node_mtg( + data.root, data.features + ) + MultiScaleTreeGraph.writedlm( + io, reshape(column_names, (1, :)), quotes=false + ) + for i in eachindex(attributes["mtg_print"]) + row = Any[attributes[key][i] for key in keys(attributes)] + MultiScaleTreeGraph.writedlm(io, reshape(row, (1, :)), quotes=false) + end + end + return file +end + +function _measure_write_mtg_pair(data; samples::Int=7) + return with_logger(NullLogger()) do + _measure_write_mtg_pair_unlogged(data; samples=samples) + end +end + +function _measure_write_mtg_pair_unlogged(data; samples::Int=7) + streaming_path = tempname() * ".mtg" + materialized_path = tempname() * ".mtg" + try + write_mtg( + streaming_path, data.root, data.classes, nothing, data.features + ) + _write_mtg_materialized(materialized_path, data) + read(streaming_path) == read(materialized_path) || + error("Streaming and materialized writers produced different bytes.") + + GC.gc() + streaming_allocations = @allocated write_mtg( + streaming_path, data.root, data.classes, nothing, data.features + ) + GC.gc() + materialized_allocations = @allocated _write_mtg_materialized( + materialized_path, data + ) + + streaming_times = Vector{Float64}(undef, samples) + materialized_times = Vector{Float64}(undef, samples) + for sample in 1:samples + if isodd(sample) + streaming_times[sample] = @elapsed write_mtg( + streaming_path, data.root, data.classes, nothing, data.features + ) + materialized_times[sample] = @elapsed _write_mtg_materialized( + materialized_path, data + ) + else + materialized_times[sample] = @elapsed _write_mtg_materialized( + materialized_path, data + ) + streaming_times[sample] = @elapsed write_mtg( + streaming_path, data.root, data.classes, nothing, data.features + ) + end + end + + return ( + bytes=filesize(streaming_path), + streaming=( + allocations=streaming_allocations, + seconds=_write_mtg_median(streaming_times), + ), + materialized=( + allocations=materialized_allocations, + seconds=_write_mtg_median(materialized_times), + ), + ) + finally + isfile(streaming_path) && rm(streaming_path; force=true) + isfile(materialized_path) && rm(materialized_path; force=true) + end +end + +function write_mtg_performance_gates(; samples::Int=7) + results = Dict{Int,Any}() + for n_nodes in (1_000, 10_000) + data = _write_mtg_performance_fixture(n_nodes) + result = _measure_write_mtg_pair(data; samples=samples) + + allocation_ratio = result.streaming.allocations / + result.materialized.allocations + time_ratio = result.streaming.seconds / result.materialized.seconds + allocation_ratio <= 0.75 || error( + "write_mtg allocation gate failed at $(n_nodes) nodes: " * + "streaming/materialized = $(allocation_ratio).", + ) + time_ratio <= 1.02 || error( + "write_mtg time gate failed at $(n_nodes) nodes: " * + "streaming/materialized = $(time_ratio).", + ) + results[n_nodes] = merge( + result, + (allocation_ratio=allocation_ratio, time_ratio=time_ratio), + ) + end + + small = results[1_000].streaming + large = results[10_000].streaming + large.allocations <= 15 * small.allocations || error( + "write_mtg allocation scaling is superlinear: " * + "10k/1k = $(large.allocations / small.allocations).", + ) + large.seconds <= 20 * small.seconds || error( + "write_mtg time scaling is superlinear: " * + "10k/1k = $(large.seconds / small.seconds).", + ) + return results +end + +if abspath(PROGRAM_FILE) == @__FILE__ + display(write_mtg_performance_gates()) +end diff --git a/src/write_mtg/write_mtg.jl b/src/write_mtg/write_mtg.jl index 464d2e42..4c0a99cb 100644 --- a/src/write_mtg/write_mtg.jl +++ b/src/write_mtg/write_mtg.jl @@ -90,69 +90,232 @@ function write_mtg(file, mtg, classes, description, features) # MTG section: writedlm(io, [""]) writedlm(io, ["MTG:"]) - mtg_df, mtg_colnames = paste_node_mtg(mtg, features) + layout = _mtg_write_layout(mtg) + feature_columns = _mtg_write_feature_columns(mtg, features) + feature_values = _mtg_write_feature_values(layout, feature_columns) + entity_feature, output_features, mtg_colnames = + _mtg_write_projection(layout, feature_columns) writedlm(io, reshape(mtg_colnames, (1, :)), quotes=false) - for i in eachindex(mtg_df["mtg_print"]) - writedlm(io, reshape([mtg_df[k][i] for k in keys(mtg_df)], (1, :)), quotes=false) - end + _write_mtg_rows( + io, layout, feature_values, entity_feature, output_features + ) end end +@inline function _flush_write_buffer!(io, buffer::IOBuffer) + if position(buffer) > 0 + seekstart(buffer) + write(io, buffer) + truncate(buffer, 0) + end + return nothing +end + function _write_table_rows(io, table) nrows, ncols = size(table) - for i in 1:nrows - row = Any[table[i, j] for j in 1:ncols] - writedlm(io, reshape(row, (1, :))) - end + rows = ((table[i, j] for j in 1:ncols) for i in 1:nrows) + writedlm(io, rows) return nothing end -function paste_node_mtg(mtg, features) +struct _MTGWriteLayout{N} + nodes::Vector{N} + leads::Vector{Int} + parent_refs::BitVector + max_tabs::Int +end - # Get the leading tabulations for each node (i.e. the column of the node) - lead = Int[] - parent_ref = String[] - print_node = String[] - get_node_printing!(mtg, lead, parent_ref, print_node) +struct _MTGWriteFeatureColumns + names::Vector{String} + keys::Vector{Symbol} + is_date::BitVector + plans::Vector{Union{Nothing,ColumnarQueryPlan}} +end - max_tabs = maximum(lead) +function _mtg_write_layout(mtg) + nodes = Vector{typeof(mtg)}() + leads = Int[] + parent_refs = BitVector() + + stack_nodes = Vector{typeof(mtg)}(undef, 1) + stack_leads = Vector{Int}(undef, 1) + stack_refs = BitVector(undef, 1) + stack_nodes[1] = mtg + stack_leads[1] = 0 + stack_refs[1] = false + max_tabs = 0 + + while !isempty(stack_nodes) + node = pop!(stack_nodes) + node_lead = pop!(stack_leads) + node_ref = pop!(stack_refs) + + push!(nodes, node) + push!(leads, node_lead) + push!(parent_refs, node_ref) + max_tabs = max(max_tabs, node_lead) + + child_nodes = children(node) + n_children = length(child_nodes) + @inbounds for i in n_children:-1:1 + changes_column = n_children > 1 && i != n_children + push!(stack_nodes, child_nodes[i]) + push!(stack_leads, changes_column ? node_lead + 1 : node_lead) + push!(stack_refs, !changes_column) + end + end - # Get the attributes for each node: - attributes = OrderedDict{String,Vector{Any}}() + return _MTGWriteLayout(nodes, leads, parent_refs, max_tabs) +end - attributes["mtg_print"] = string.( - # Add the leading tabulations: - repeat.("\t", lead), - # Add the "^" keyword before mtg print in case we refer to the column above: - parent_ref, - # Add the mtg printing (e.g. "/Axis0"): - print_node, - # Add the trailing tabulations: - repeat.("\t", max_tabs .- lead) - ) - - for var in string.(features.NAME) - push!(attributes, var => descendants(mtg, var, self=true)) - end +function _mtg_write_feature_columns(mtg, features) + names_ = String[] + keys_ = Symbol[] + is_date = BitVector() + name_to_column = Dict{String,Int}() - # Build the "ENTITY-CODE" column with necessary "^", leading and trailing tabs - feature_type = Dict{String,String}() @inbounds for i in eachindex(features.NAME) - feature_type[string(features.NAME[i])] = string(features.TYPE[i]) + name = string(features.NAME[i]) + date_feature = string(features.TYPE[i]) == "DD/MM/YY" + column = get(name_to_column, name, 0) + if column == 0 + push!(names_, name) + push!(keys_, Symbol(name)) + push!(is_date, date_feature) + name_to_column[name] = length(names_) + else + # `paste_node_mtg` historically kept the first column position for a + # duplicate feature name, while the last type controlled formatting. + is_date[column] = date_feature + end + end + + plans = Vector{Union{Nothing,ColumnarQueryPlan}}(undef, length(keys_)) + @inbounds for i in eachindex(keys_) + plans[i] = build_columnar_query_plan(mtg, keys_[i]) + end + return _MTGWriteFeatureColumns(names_, keys_, is_date, plans) +end + +function _mtg_write_feature_values( + layout::_MTGWriteLayout, features::_MTGWriteFeatureColumns +) + values = [Vector{Any}(undef, length(layout.nodes)) for _ in eachindex(features.keys)] + + # Column-major lookup matches the physical layout of `ColumnarAttrs` and avoids + # repeating a full tree traversal for every feature. + @inbounds for j in eachindex(features.keys) + column = values[j] + key = features.keys[j] + plan = features.plans[j] + for i in eachindex(layout.nodes) + column[i] = unsafe_getindex(layout.nodes[i], key, plan) + end + end + + # Keep the historical conversion order: all values are collected before date + # formatting is attempted, and `nothing` is represented by an empty field. + @inbounds for j in eachindex(values) + column = values[j] + if features.is_date[j] + for i in eachindex(column) + value = column[i] + column[i] = value === nothing ? "" : format(value, dateformat"d/m/Y") + end + else + replace!(column, nothing => "") + end end + return values +end - for (key, val) in attributes - # If the attribute is a date, write it in the day/month/year format: - if get(feature_type, key, "") == "DD/MM/YY" - replace!(x -> isnothing(x) ? x : format(x, dateformat"d/m/Y"), val) +function _mtg_write_projection( + layout::_MTGWriteLayout, features::_MTGWriteFeatureColumns +) + # `paste_node_mtg` has always used `mtg_print` as its internal topology key. + # A feature with that name therefore replaces the entity-code values instead + # of adding a column; retain that unusual but observable behavior. + entity_feature = findfirst(==("mtg_print"), features.names) + output_features = Int[] + mtg_colnames = String[string("ENTITY-CODE", repeat("\t", layout.max_tabs))] + @inbounds for j in eachindex(features.names) + j == entity_feature && continue + push!(output_features, j) + push!(mtg_colnames, features.names[j]) + end + return entity_feature, output_features, mtg_colnames +end + +@inline function _write_mtg_node_code(io, node) + print(io, String(link(node)), String(symbol(node))) + node_index = index(node) + node_index == -9999 || print(io, node_index) + return nothing +end + +function _write_mtg_rows( + io, + layout::_MTGWriteLayout, + feature_values::Vector{Vector{Any}}, + entity_feature::Union{Nothing,Int}, + output_features::Vector{Int}, +) + buffer = IOBuffer(; sizehint=64 * 1024) + @inbounds for i in eachindex(layout.nodes) + node = layout.nodes[i] + node_lead = layout.leads[i] + + if entity_feature === nothing + for _ in 1:node_lead + print(buffer, '\t') + end + layout.parent_refs[i] && print(buffer, '^') + _write_mtg_node_code(buffer, node) + for _ in 1:(layout.max_tabs - node_lead) + print(buffer, '\t') + end + else + print(buffer, feature_values[entity_feature][i]) end - # Replacing all nothing values by an empty string: - replace!(val, nothing => "") + for j in output_features + print(buffer, '\t') + print(buffer, feature_values[j][i]) + end + print(buffer, '\n') + position(buffer) > 16 * 1024 && _flush_write_buffer!(io, buffer) end - # Renaming first column and adding tabs: + _flush_write_buffer!(io, buffer) + return nothing +end + +function paste_node_mtg(mtg, features) + layout = _mtg_write_layout(mtg) + feature_columns = _mtg_write_feature_columns(mtg, features) + feature_values = _mtg_write_feature_values(layout, feature_columns) + + attributes = OrderedDict{String,Vector{Any}}() + mtg_print = Vector{Any}(undef, length(layout.nodes)) + @inbounds for i in eachindex(layout.nodes) + node_buffer = IOBuffer() + for _ in 1:layout.leads[i] + print(node_buffer, '\t') + end + layout.parent_refs[i] && print(node_buffer, '^') + _write_mtg_node_code(node_buffer, layout.nodes[i]) + for _ in 1:(layout.max_tabs - layout.leads[i]) + print(node_buffer, '\t') + end + mtg_print[i] = String(take!(node_buffer)) + end + attributes["mtg_print"] = mtg_print + + @inbounds for j in eachindex(feature_columns.keys) + attributes[feature_columns.names[j]] = feature_values[j] + end + mtg_colnames = collect(keys(attributes)) - mtg_colnames[1] = string("ENTITY-CODE", repeat("\t", max_tabs)) + mtg_colnames[1] = string("ENTITY-CODE", repeat("\t", layout.max_tabs)) return attributes, mtg_colnames end diff --git a/test/test-write_mtg.jl b/test/test-write_mtg.jl index 41429d8f..53ea7dd0 100644 --- a/test/test-write_mtg.jl +++ b/test/test-write_mtg.jl @@ -89,3 +89,257 @@ mtg = read_mtg(file) @test traverse(mtg, symbol) == traverse(mtg2, symbol) @test traverse(mtg, index) == traverse(mtg2, index) end + +function _legacy_paste_node_mtg_for_test(mtg, features) + lead = Int[] + parent_ref = String[] + print_node = String[] + MultiScaleTreeGraph.get_node_printing!(mtg, lead, parent_ref, print_node) + max_tabs = maximum(lead) + + attributes = MultiScaleTreeGraph.OrderedDict{String,Vector{Any}}() + attributes["mtg_print"] = string.( + repeat.("\t", lead), + parent_ref, + print_node, + repeat.("\t", max_tabs .- lead), + ) + for var in string.(features.NAME) + push!(attributes, var => descendants(mtg, var, self=true)) + end + + feature_type = Dict{String,String}() + @inbounds for i in eachindex(features.NAME) + feature_type[string(features.NAME[i])] = string(features.TYPE[i]) + end + for (key, values_) in attributes + if get(feature_type, key, "") == "DD/MM/YY" + replace!(x -> isnothing(x) ? x : Dates.format(x, dateformat"d/m/Y"), values_) + end + replace!(values_, nothing => "") + end + + mtg_colnames = collect(keys(attributes)) + mtg_colnames[1] = string("ENTITY-CODE", repeat("\t", max_tabs)) + return attributes, mtg_colnames +end + +function _legacy_mtg_section_for_test(mtg, features) + io = IOBuffer() + MultiScaleTreeGraph.writedlm(io, ["MTG:"]) + attributes, column_names = _legacy_paste_node_mtg_for_test(mtg, features) + MultiScaleTreeGraph.writedlm(io, reshape(column_names, (1, :)), quotes=false) + for i in eachindex(attributes["mtg_print"]) + row = Any[attributes[key][i] for key in keys(attributes)] + MultiScaleTreeGraph.writedlm(io, reshape(row, (1, :)), quotes=false) + end + return String(take!(io)) +end + +function _written_mtg_section_for_test(text::String) + section = findfirst("MTG:\n", text) + section === nothing && error("Written file has no MTG section.") + return text[first(section):end] +end + +function _many_feature_writer_fixture(nfeatures::Int) + feature_names = [Symbol("feature_", lpad(i, 2, '0')) for i in 1:nfeatures] + feature_types = [ + (i % 7 == 1 ? "REAL" : + i % 7 == 2 ? "INT" : + i % 7 == 3 ? "BOOLEAN" : + i % 7 == 0 ? "DD/MM/YY" : "STRING") for i in 1:nfeatures + ] + + function node_attributes_for(ordinal::Int) + attrs = Dict{Symbol,Any}() + for i in eachindex(feature_names) + key = feature_names[i] + typ = feature_types[i] + value = if ordinal == 2 && i % 5 == 0 + nothing + elseif typ == "REAL" + ordinal + i / 100 + elseif typ == "INT" + ordinal * i + elseif typ == "BOOLEAN" + isodd(ordinal + i) + elseif typ == "DD/MM/YY" + ordinal == 4 && i % 14 == 0 ? nothing : Date(2026, mod1(i, 12), ordinal) + elseif ordinal == 4 && i % 6 == 0 + missing + elseif i % 7 == 5 + Symbol("state_$(ordinal)_$(i)") + else + "node$(ordinal)-feature$(i)" + end + attrs[key] = value + end + return attrs + end + + root = Node( + 10, + MutableNodeMTG(:/, :Plant, 1, 1), + node_attributes_for(1), + ) + axis = Node( + 41, + root, + MutableNodeMTG(:/, :Axis, 1, 2), + node_attributes_for(2), + ) + Node( + 105, + axis, + MutableNodeMTG(:+, :Leaf, 1, 3), + node_attributes_for(3), + ) + continuation = Node( + 1_000, + axis, + MutableNodeMTG(:<, :Axis, 2, 2), + node_attributes_for(4), + ) + Node( + 10_000, + continuation, + MutableNodeMTG(:<, :Axis, -9999, 2), + node_attributes_for(5), + ) + + features = DataFrame(NAME=feature_names, TYPE=feature_types) + return root, features +end + +@testset "Streaming writer preserves wide columnar MTG bytes" begin + for nfeatures in (28, 40) + mtg, features = _many_feature_writer_fixture(nfeatures) + @test node_attributes(mtg) isa MultiScaleTreeGraph.ColumnarAttrs + @test list_nodes(mtg) == [10, 41, 105, 1_000, 10_000] + + expected_section = _legacy_mtg_section_for_test(mtg, features) + actual = mktemp() do path, io + close(io) + write_mtg(path, mtg, get_classes(mtg), nothing, features) + return read(path, String) + end + @test _written_mtg_section_for_test(actual) == expected_section + + expected_attributes, expected_names = _legacy_paste_node_mtg_for_test(mtg, features) + actual_attributes, actual_names = MultiScaleTreeGraph.paste_node_mtg(mtg, features) + @test actual_names == expected_names + @test isequal(actual_attributes, expected_attributes) + @test occursin("missing", actual) + @test occursin("^ long_text, + :date => Date(2026, 8, 24), + :missing_value => missing, + :nothing_value => nothing, + :symbol_value => :root, + :quoted_value => "root\t\"quoted\"\nvalue", + ), + ) + Node( + 10_000, + root, + MutableNodeMTG(:<, :Leaf, -9999, 2), + Dict( + :long_text => long_text, + :date => Date(2025, 1, 2), + :missing_value => "present", + :nothing_value => nothing, + :symbol_value => :child, + :quoted_value => "child\t\"quoted\"\nvalue", + ), + ) + features = DataFrame( + NAME=[ + :long_text, + :date, + :missing_value, + :date, + :nothing_value, + :symbol_value, + :quoted_value, + ], + TYPE=["STRING", "STRING", "STRING", "DD/MM/YY", "STRING", "STRING", "STRING"], + ) + + expected_section = _legacy_mtg_section_for_test(root, features) + @test ncodeunits(expected_section) > 16 * 1024 + actual = mktemp() do path, io + close(io) + write_mtg(path, root, get_classes(root), nothing, features) + return read(path, String) + end + @test _written_mtg_section_for_test(actual) == expected_section +end + +@testset "Streaming writer preserves mtg_print feature collision" begin + root = Node( + 10, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict(:other => 1, :mtg_print => "root entity override"), + ) + Node( + 10_000, + root, + MutableNodeMTG(:<, :Leaf, -9999, 2), + Dict(:other => 2, :mtg_print => "child entity override"), + ) + features = DataFrame( + NAME=[:other, :mtg_print], + TYPE=["INT", "STRING"], + ) + + expected_section = _legacy_mtg_section_for_test(root, features) + actual = mktemp() do path, io + close(io) + write_mtg(path, root, get_classes(root), nothing, features) + return read(path, String) + end + @test _written_mtg_section_for_test(actual) == expected_section + + expected_attributes, expected_names = _legacy_paste_node_mtg_for_test(root, features) + actual_attributes, actual_names = MultiScaleTreeGraph.paste_node_mtg(root, features) + @test actual_names == expected_names + @test isequal(actual_attributes, expected_attributes) +end + +@testset "Table rows stream without changing DelimitedFiles quoting" begin + table = DataFrame( + first=["plain", "with\ttab", "with\"quote", "with\nline", repeat("long", 6_000)], + second=Any[1, missing, true, :symbol, 42], + ) + expected = IOBuffer() + for i in 1:size(table, 1) + row = Any[table[i, j] for j in 1:size(table, 2)] + MultiScaleTreeGraph.writedlm(expected, reshape(row, (1, :))) + end + actual = IOBuffer() + MultiScaleTreeGraph._write_table_rows(actual, table) + actual_bytes = take!(actual) + expected_bytes = take!(expected) + @test length(actual_bytes) > 16 * 1024 + @test actual_bytes == expected_bytes +end + +@testset "Streaming writer preserves date conversion errors" begin + mtg = Node(10, MutableNodeMTG(:/, :Plant, 1, 1), Dict(:date => missing)) + features = DataFrame(NAME=[:date], TYPE=["DD/MM/YY"]) + mktemp() do path, io + close(io) + @test_throws MethodError write_mtg(path, mtg, get_classes(mtg), nothing, features) + @test endswith(read(path, String), "MTG:\n") + end +end From e7f4ecd2b4653ea4d30f30601ca88e115e4e5566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Mon, 24 Aug 2026 11:03:26 +0200 Subject: [PATCH 02/13] fix: invalidate traversal caches after topology changes --- src/compute_MTG/caching.jl | 5 +- src/compute_MTG/columnarize.jl | 6 +- src/compute_MTG/delete_nodes.jl | 2 + src/compute_MTG/indexing.jl | 28 ++- src/compute_MTG/insert_nodes.jl | 9 +- src/compute_MTG/node_funs.jl | 77 +++++-- src/compute_MTG/prune.jl | 2 + src/types/Attributes.jl | 4 +- src/types/Node.jl | 303 +++++++++++++++++++++++++++- test/test-caching.jl | 346 ++++++++++++++++++++++++++++++++ test/test-descendants.jl | 5 +- test/test-nodes.jl | 12 +- 12 files changed, 765 insertions(+), 34 deletions(-) diff --git a/src/compute_MTG/caching.jl b/src/compute_MTG/caching.jl index b1ac6077..4952681a 100644 --- a/src/compute_MTG/caching.jl +++ b/src/compute_MTG/caching.jl @@ -10,7 +10,7 @@ cache_name("test","var") ``` """ function cache_name(vars...) - "_cache_" * bytes2hex(sha1(join([vars...]))) + "_cache_" * bytes2hex(sha1(join(vars))) end """ @@ -37,6 +37,9 @@ end Cache the nodes of the mtg based on the filters that would be applied to a traversal. This is used automatically when traversing using [`traverse!`](@ref) or [`transform!`](@ref). +Cached traversals are invalidated automatically on the mutated subtree and its +ancestors when nodes are added, removed, or reparented. + # Examples ```julia diff --git a/src/compute_MTG/columnarize.jl b/src/compute_MTG/columnarize.jl index f7182972..2a681ad8 100644 --- a/src/compute_MTG/columnarize.jl +++ b/src/compute_MTG/columnarize.jl @@ -4,11 +4,15 @@ Bind all node attributes to a single `MTGAttributeStore`. """ function columnarize!(mtg::Node) - nodes = traverse(mtg, node -> node, type=typeof(mtg)) + root = get_root(mtg) + nodes = traverse(root, node -> node, type=typeof(root)) isempty(nodes) && return mtg store = MTGAttributeStore() for n in nodes + if _maybe_traversal_cache(n) !== nothing + _register_traversal_cache!(store, n) + end attrs = node_attributes(n) attrs isa ColumnarAttrs || error("columnarize! expects nodes with ColumnarAttrs attributes.") raw = _isbound(attrs) ? Dict{Symbol,Any}(pairs(attrs)) : attrs.staged diff --git a/src/compute_MTG/delete_nodes.jl b/src/compute_MTG/delete_nodes.jl index c4ac0482..a248ae97 100644 --- a/src/compute_MTG/delete_nodes.jl +++ b/src/compute_MTG/delete_nodes.jl @@ -154,10 +154,12 @@ function delete_node!(node::Node{N,A}; child_link_fun=new_child_link) where {N<: parent_children = children(parent_node) idx = _child_index_by_id(parent_children, node_id(node)) idx === nothing || deleteat!(parent_children, idx) + idx === nothing || _mark_structure_mutation!(parent_node) node_return = parent_node end # Clean the old deleted node (isolate it, no parent, no children): + _discard_traversal_cache!(node) attrs = node_attributes(node) attrs isa ColumnarAttrs && remove_columnar_node!(attrs) reparent!(node, nothing) diff --git a/src/compute_MTG/indexing.jl b/src/compute_MTG/indexing.jl index 17146ff4..e32ff119 100644 --- a/src/compute_MTG/indexing.jl +++ b/src/compute_MTG/indexing.jl @@ -2,7 +2,33 @@ Indexing a Node using an integer will index in its children """ Base.getindex(n::Node, i::Integer) = children(n)[i] -Base.setindex!(n::Node, x::Node, i::Integer) = children(n)[i] = x +function _set_child_at!(n::Node{N,A}, x::Node{N,A}, i::Integer) where {N,A} + current_children = children(n) + old_child = current_children[i] + old_child === x && return x + existing_index = _child_index_by_identity(current_children, x) + existing_index === nothing || throw(ArgumentError( + "node $(node_id(x)) is already a child of node $(node_id(n)) at index $existing_index", + )) + parent(x) === nothing || throw(ArgumentError( + "replacement node $(node_id(x)) already has parent $(node_id(parent(x))); detach it before indexed assignment", + )) + _validate_reparent_target(x, n) + _validate_columnar_attach!(n, x) + reparent!(old_child, nothing) + addchild!(n, x) + attached_children = children(n) + attached_index = _child_index_by_identity(attached_children, x)::Int + deleteat!(attached_children, attached_index) + insert!(attached_children, i, x) + _mark_structure_mutation!(n) + return x +end + +Base.setindex!(n::Node{N,A}, x::Node{N,A}, i::Integer) where {N,A} = + _set_child_at!(n, x, i) +Base.setindex!(n::Node{N,A}, x::Node{N,A}, i::Integer) where {N<:AbstractNodeMTG,A<:AbstractDict} = + _set_child_at!(n, x, i) """ Indexing Node attributes from node, e.g. node[:length] or node["length"], diff --git a/src/compute_MTG/insert_nodes.jl b/src/compute_MTG/insert_nodes.jl index f406b311..06963e50 100644 --- a/src/compute_MTG/insert_nodes.jl +++ b/src/compute_MTG/insert_nodes.jl @@ -295,7 +295,7 @@ function insert_parent!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[ Node{N,A}[node], new_node_MTG(node, template), _coerce_insert_attrs(A, copy(attr_fun(node))), - Dict{String,Vector{Node{N,A}}}() + nothing ) _bind_inserted_columnar!(A, node, new_node) @@ -317,7 +317,7 @@ function insert_parent!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[ Node{N,A}[node], new_node_MTG(node, template), _coerce_insert_attrs(A, copy(attr_fun(node))), - Dict{String,Vector{Node{N,A}}}() + nothing ) _bind_inserted_columnar!(A, parent(node), new_node) @@ -358,12 +358,13 @@ function insert_sibling!(node::Node{N,A}, template, attr_fun=node -> A(), maxid= Vector{Node{N,A}}(), new_node_MTG(node, template), _coerce_insert_attrs(A, copy(attr_fun(node))), - Dict{String,Vector{Node{N,A}}}() + nothing ) _bind_inserted_columnar!(A, parent(node), new_node) # Add the new node to the children of the parent node: push!(children(parent(node)), new_node) + _mark_structure_mutation!(parent(node)) return node end @@ -378,7 +379,7 @@ function insert_generation!(node::Node{N,A}, template, attr_fun=node -> A(), max children(node), new_node_MTG(node, template), _coerce_insert_attrs(A, copy(attr_fun(node))), - Dict{String,Vector{Node{N,A}}}() + nothing ) _bind_inserted_columnar!(A, node, new_node) diff --git a/src/compute_MTG/node_funs.jl b/src/compute_MTG/node_funs.jl index 9a686cc3..9eba7d41 100644 --- a/src/compute_MTG/node_funs.jl +++ b/src/compute_MTG/node_funs.jl @@ -84,12 +84,20 @@ end end function _subtree_has_node_id_conflict(node::Node, store::MTGAttributeStore) - nid = node_id(node) - if nid <= length(store.node_bucket) && store.node_bucket[nid] != 0 - return true - end - for ch in children(node) - _subtree_has_node_id_conflict(ch, store) && return true + seen_ids = Set{Int}() + stack = typeof(node)[node] + while !isempty(stack) + current = pop!(stack) + nid = node_id(current) + nid in seen_ids && return true + push!(seen_ids, nid) + if nid <= length(store.node_bucket) && store.node_bucket[nid] != 0 + return true + end + current_children = children(current) + @inbounds for child_index in reverse(eachindex(current_children)) + push!(stack, current_children[child_index]) + end end return false end @@ -97,44 +105,81 @@ end function _merge_columnar_subtree_into_store!(node::Node, target_store::MTGAttributeStore) attrs = node_attributes(node) attrs isa ColumnarAttrs || return false + source_store = _store_for_node_attrs(attrs) snapshot = Dict{Symbol,Any}(pairs(attrs)) _add_node_with_attrs!(target_store, node_id(node), symbol(node), snapshot) + if source_store !== nothing && source_store !== target_store + _remove_node!(source_store, node_id(node)) + end attrs.ref.store = target_store attrs.ref.node_id = node_id(node) empty!(attrs.staged) + if _maybe_traversal_cache(node) !== nothing + source_store === nothing || source_store === target_store || + _unregister_traversal_cache!(source_store, node) + _register_traversal_cache!(target_store, node) + end for ch in children(node) _merge_columnar_subtree_into_store!(ch, target_store) || return false end return true end -function _try_merge_root_subtree_store!(child::Node, parent_store::MTGAttributeStore, child_store::MTGAttributeStore) +function _try_merge_subtree_store!(child::Node, parent_store::MTGAttributeStore, child_store::MTGAttributeStore) parent_store === child_store && return true _subtree_has_node_id_conflict(child, parent_store) && return false return _merge_columnar_subtree_into_store!(child, parent_store) end -@inline function _maybe_recolumnarize_after_attach!(p::Node, child::Node, child_was_root::Bool) - child_was_root || return nothing +function _validate_columnar_attach!( + p::Node, + child::Node, + reserved_ids::Union{Nothing,Set{Int}}=nothing, +) p_store = _columnar_store_or_nothing(p) c_store = _columnar_store_or_nothing(child) if p_store !== nothing && c_store !== nothing && p_store !== c_store - # Fast path: re-bind only the attached subtree into the parent store. - # Fallback to full re-columnarization when IDs collide. - _try_merge_root_subtree_store!(child, p_store, c_store) || columnarize!(get_root(p)) + reserved_ids === nothing && (reserved_ids = Set{Int}()) + subtree_ids = Set{Int}() + stack = typeof(child)[child] + while !isempty(stack) + current = pop!(stack) + nid = node_id(current) + if nid in subtree_ids || + (reserved_ids !== nothing && nid in reserved_ids) || + (nid <= length(p_store.node_bucket) && p_store.node_bucket[nid] != 0) + throw(ArgumentError( + "cannot attach subtree rooted at node $(node_id(child)): node id $nid is duplicated or already present in the destination attribute store", + )) + end + push!(subtree_ids, nid) + current_children = children(current) + @inbounds for child_index in reverse(eachindex(current_children)) + push!(stack, current_children[child_index]) + end + end + union!(reserved_ids, subtree_ids) + end + return reserved_ids +end + +@inline function _maybe_recolumnarize_after_attach!(p::Node, child::Node) + p_store = _columnar_store_or_nothing(p) + c_store = _columnar_store_or_nothing(child) + if p_store !== nothing && c_store !== nothing && p_store !== c_store + _try_merge_subtree_store!(child, p_store, c_store) || error( + "failed to merge the attached subtree into its destination attribute store", + ) end return nothing end function addchild!(p::Node{N,A}, child::Node; force=false) where {N<:AbstractNodeMTG,A} - child_was_root = parent(child) === nothing - - if !child_was_root && parent(child) !== p && force == false + if parent(child) !== nothing && parent(child) !== p && force == false error("The node already has a parent. Hint: use `force=true` if needed.") end reparent!(child, p) - _maybe_recolumnarize_after_attach!(p, child, child_was_root) return child end diff --git a/src/compute_MTG/prune.jl b/src/compute_MTG/prune.jl index 162b2185..9da49cd5 100644 --- a/src/compute_MTG/prune.jl +++ b/src/compute_MTG/prune.jl @@ -26,6 +26,7 @@ function prune!(node) stack = Node[node] while !isempty(stack) n = pop!(stack) + _discard_traversal_cache!(n) attrs = node_attributes(n) attrs isa ColumnarAttrs && remove_columnar_node!(attrs) ch = children(n) @@ -38,6 +39,7 @@ function prune!(node) parent_children = children(parent_node) idx = _child_index_by_id(parent_children, node_id(node)) idx === nothing || deleteat!(parent_children, idx) + idx === nothing || _mark_structure_mutation!(parent_node) end # Delete the links to the parent: diff --git a/src/types/Attributes.jl b/src/types/Attributes.jl index 4689c095..030f21e8 100644 --- a/src/types/Attributes.jl +++ b/src/types/Attributes.jl @@ -12,9 +12,11 @@ mutable struct SubtreeIndexCache tin::Vector{Int} tout::Vector{Int} dfs_order::Vector{Int} + traversal_cache_nodes::Union{Nothing,WeakRef,Vector{WeakRef}} end -SubtreeIndexCache() = SubtreeIndexCache(true, false, :auto, 0, 0, Int[], Int[], Int[]) +SubtreeIndexCache() = + SubtreeIndexCache(true, false, :auto, 0, 0, Int[], Int[], Int[], nothing) mutable struct Column{T} name::Symbol diff --git a/src/types/Node.jl b/src/types/Node.jl index b5fce9c2..5a231a54 100644 --- a/src/types/Node.jl +++ b/src/types/Node.jl @@ -56,6 +56,42 @@ mutable struct Node{N<:AbstractNodeMTG,A} attributes::A "Cache for mtg nodes traversal" traversal_cache::Union{Nothing,Dict{String,Vector{Node{N,A}}}} + + function Node{N,A}( + id, + parent, + children, + MTG, + attributes, + ::Nothing, + ) where {N<:AbstractNodeMTG,A} + new{N,A}(id, parent, children, MTG, attributes, nothing) + end + + function Node{N,A}( + id, + parent, + children, + MTG, + attributes, + traversal_cache, + ) where {N<:AbstractNodeMTG,A} + node = new{N,A}(id, parent, children, MTG, attributes, traversal_cache) + getfield(node, :traversal_cache) === nothing || + _register_existing_traversal_cache!(node) + return node + end +end + +function Node( + id::Int, + parent::Union{Nothing,Node{N,A}}, + children::Vector{Node{N,A}}, + MTG::N, + attributes::A, + traversal_cache::Union{Nothing,Dict{String,Vector{Node{N,A}}}}, +) where {N<:AbstractNodeMTG,A} + Node{N,A}(id, parent, children, MTG, attributes, traversal_cache) end # All deprecated methods (the ones with a node name) : @@ -103,7 +139,8 @@ function Node(id::Int, parent::Node{M,ColumnarAttrs}, MTG::M, attributes::Column node = Node{M,ColumnarAttrs}( id, parent, Vector{Node{M,ColumnarAttrs}}(), MTG, attributes, nothing ) - addchild!(parent, node) + push!(children(parent), node) + _invalidate_traversal_caches!(parent) bind_columnar_child!(node_attributes(parent), attributes, id, getfield(MTG, :symbol)) return node end @@ -203,6 +240,17 @@ function _attach_child!(p::Node, child::Node) return true end +function _validate_reparent_target(node::Node, new_parent::Node) + current = new_parent + while current !== nothing + current === node && throw(ArgumentError( + "cannot reparent node $(node_id(node)) below its own descendant $(node_id(new_parent))", + )) + current = parent(current) + end + return nothing +end + """ reparent!(node::N, p::N) where N<:Node{T,A} @@ -214,13 +262,21 @@ function reparent!(node::N, p::N2) where {N<:Node{T,A},N2<:Union{Nothing,Node{T, old_parent = parent(node) changed = old_parent !== p + if changed && p !== nothing + _validate_reparent_target(node, p) + _validate_columnar_attach!(p, node) + end + changed && _maybe_traversal_cache(node) !== nothing && _discard_traversal_cache!(node) if old_parent !== nothing && changed _detach_child!(old_parent, node) end setfield!(node, :parent, p) attached = p === nothing ? false : _attach_child!(p, node) - (changed || attached) && _mark_structure_mutation!(node) + p === nothing || _maybe_recolumnarize_after_attach!(p, node) + if (changed || attached) && (!attached || _maybe_traversal_cache(node) !== nothing) + _mark_structure_mutation!(node) + end return p end @@ -231,6 +287,32 @@ Set the children of the node, detaching removed children and setting this node a parent of the new children. """ function rechildren!(node::Node{T,A}, chnodes::Vector{Node{T,A}}) where {T,A} + if length(chnodes) <= 8 + for child_index in eachindex(chnodes) + child = chnodes[child_index] + @inbounds for previous_index in firstindex(chnodes):(child_index - 1) + child === chnodes[previous_index] && throw(ArgumentError( + "node $(node_id(child)) cannot occur more than once in the children of node $(node_id(node))", + )) + end + end + else + seen_children = Base.IdSet{Node{T,A}}() + sizehint!(seen_children, length(chnodes)) + for child in chnodes + child in seen_children && throw(ArgumentError( + "node $(node_id(child)) cannot occur more than once in the children of node $(node_id(node))", + )) + push!(seen_children, child) + end + end + incoming_cross_store_ids = nothing + for child in chnodes + _validate_reparent_target(child, node) + incoming_cross_store_ids = + _validate_columnar_attach!(node, child, incoming_cross_store_ids) + end + old_children = children(node) setfield!(node, :children, chnodes) @@ -468,10 +550,216 @@ function _node_store(node::Node) return store end -@inline function _mark_structure_mutation!(node::Node) +function _register_traversal_cache!(store::MTGAttributeStore, node::Node) + registry = store.subtree_index.traversal_cache_nodes + if registry === nothing + store.subtree_index.traversal_cache_nodes = WeakRef(node) + elseif registry isa WeakRef + existing = registry.value + existing === node && return nothing + if existing === nothing + store.subtree_index.traversal_cache_nodes = WeakRef(node) + else + store.subtree_index.traversal_cache_nodes = WeakRef[registry, WeakRef(node)] + end + else + write_index = firstindex(registry) + found = false + @inbounds for read_index in eachindex(registry) + entry = registry[read_index] + existing = entry.value + existing === nothing && continue + found |= existing === node + registry[write_index] = entry + write_index += 1 + end + resize!(registry, write_index - 1) + found || push!(registry, WeakRef(node)) + end + return nothing +end + +function _traversal_cache_registered(store::MTGAttributeStore, node::Node) + registry = store.subtree_index.traversal_cache_nodes + registry === nothing && return false + registry isa WeakRef && return registry.value === node + @inbounds for entry in registry + entry.value === node && return true + end + return false +end + +@inline function _traversal_cache_registry_active!(store::MTGAttributeStore) + registry = store.subtree_index.traversal_cache_nodes + registry === nothing && return false + if registry isa WeakRef + existing = registry.value + (existing === nothing || _maybe_traversal_cache(existing::Node) === nothing) || return true + else + write_index = firstindex(registry) + @inbounds for read_index in eachindex(registry) + entry = registry[read_index] + existing = entry.value + (existing === nothing || _maybe_traversal_cache(existing::Node) === nothing) && continue + registry[write_index] = entry + write_index += 1 + end + resize!(registry, write_index - 1) + if length(registry) == 1 + store.subtree_index.traversal_cache_nodes = only(registry) + return true + end + isempty(registry) || return true + end + store.subtree_index.traversal_cache_nodes = nothing + return false +end + +@inline function _traversal_cache_store(node::Node) attrs = node_attributes(node) - attrs isa ColumnarAttrs || return nothing - store = _store_for_node_attrs(attrs) + store = attrs isa ColumnarAttrs ? _store_for_node_attrs(attrs) : nothing + if store === nothing + parent_node = parent(node) + if parent_node !== nothing + parent_attrs = node_attributes(parent_node) + store = parent_attrs isa ColumnarAttrs ? _store_for_node_attrs(parent_attrs) : nothing + end + end + return store +end + +function _register_existing_traversal_cache!(node::Node) + store = _traversal_cache_store(node) + store === nothing || _register_traversal_cache!(store, node) + return nothing +end + +@inline function _unregister_traversal_cache!(store::MTGAttributeStore, node::Node) + registry = store.subtree_index.traversal_cache_nodes + registry === nothing && return nothing + if registry isa WeakRef + existing = registry.value + (existing === nothing || existing === node) && + (store.subtree_index.traversal_cache_nodes = nothing) + else + write_index = firstindex(registry) + @inbounds for read_index in eachindex(registry) + entry = registry[read_index] + existing = entry.value + (existing === nothing || existing === node) && continue + registry[write_index] = entry + write_index += 1 + end + resize!(registry, write_index - 1) + if isempty(registry) + store.subtree_index.traversal_cache_nodes = nothing + elseif length(registry) == 1 + store.subtree_index.traversal_cache_nodes = only(registry) + end + end + return nothing +end + +@inline function _discard_traversal_cache!(node::Node, store::MTGAttributeStore) + cache = _maybe_traversal_cache(node) + cache === nothing && return nothing + empty!(cache) + setfield!(node, :traversal_cache, nothing) + _unregister_traversal_cache!(store, node) + return nothing +end + +@inline function _discard_traversal_cache!(node::Node) + cache = _maybe_traversal_cache(node) + cache === nothing && return nothing + empty!(cache) + setfield!(node, :traversal_cache, nothing) + store = _traversal_cache_store(node) + store === nothing || _unregister_traversal_cache!(store, node) + return nothing +end + +@inline function _traversal_cache_mutation_store(node::Node) + attrs = node_attributes(node) + store = attrs isa ColumnarAttrs ? _store_for_node_attrs(attrs) : nothing + if store === nothing && attrs isa ColumnarAttrs + parent_node = parent(node) + if parent_node !== nothing + parent_attrs = node_attributes(parent_node) + store = parent_attrs isa ColumnarAttrs ? _store_for_node_attrs(parent_attrs) : nothing + end + end + return store +end + +@noinline function _invalidate_traversal_caches_slow!( + node::Node, + store::Union{Nothing,MTGAttributeStore}, +) + # Traversal caches are local to their starting node. A structural mutation in + # this subtree therefore invalidates the cache on `node` and on every + # ancestor whose traversal can include it. Descendant caches remain valid. + # The common columnar path stays O(1) until a traversal cache has actually + # been requested for that store. + registry = store === nothing ? nothing : store.subtree_index.traversal_cache_nodes + registry_active = store === nothing + registry_entry_count = 0 + if registry isa WeakRef + existing = registry.value + if existing === nothing || _maybe_traversal_cache(existing::Node) === nothing + store.subtree_index.traversal_cache_nodes = nothing + else + registry_active = true + registry_entry_count = 1 + end + elseif registry !== nothing + registry_active = true + registry_entry_count = length(registry) + end + if registry_active + cleared_count = 0 + current = node + while current !== nothing + if store === nothing + _discard_traversal_cache!(current) + else + cache = _maybe_traversal_cache(current) + if cache !== nothing + empty!(cache) + setfield!(current, :traversal_cache, nothing) + cleared_count += 1 + end + end + current = parent(current) + end + if store !== nothing && store.subtree_index.traversal_cache_nodes !== nothing + if cleared_count == registry_entry_count + store.subtree_index.traversal_cache_nodes = nothing + else + _traversal_cache_registry_active!(store) + end + end + end + return nothing +end + +@inline function _invalidate_traversal_caches!( + node::Node, + store::Union{Nothing,MTGAttributeStore}, +) + if store !== nothing && store.subtree_index.traversal_cache_nodes === nothing + return nothing + end + _invalidate_traversal_caches_slow!(node, store) +end + +@inline function _invalidate_traversal_caches!(node::Node) + _invalidate_traversal_caches!(node, _traversal_cache_mutation_store(node)) +end + +@inline function _mark_structure_mutation!(node::Node) + store = _traversal_cache_mutation_store(node) + _invalidate_traversal_caches!(node, store) store === nothing && return nothing _mark_subtree_index_mutation!(store) return nothing @@ -570,6 +858,11 @@ Get the traversal cache of the node if any. @inline _maybe_traversal_cache(node::Node) = getfield(node, :traversal_cache) function node_traversal_cache(node::Node{T,A}) where {T,A} + attrs = node_attributes(node) + if attrs isa ColumnarAttrs + store = _store_for_node_attrs(attrs) + store === nothing || _register_traversal_cache!(store, node) + end cache = getfield(node, :traversal_cache) if cache === nothing cache = Dict{String,Vector{Node{T,A}}}() diff --git a/test/test-caching.jl b/test/test-caching.jl index 0f65cda3..6389b442 100644 --- a/test/test-caching.jl +++ b/test/test-caching.jl @@ -64,3 +64,349 @@ file = joinpath(dirname(dirname(pathof(MultiScaleTreeGraph))), "test", "files", # Check that the nodes of the MTG where not modified: @test [node[:x] for node in AbstractTrees.PreOrderDFS(mtg)] == [1, 2, 3, 4, 5, 6, 7] end + +@testset "structural mutations invalidate traversal caches" begin + mtg = read_mtg(file) + internode = get_node(mtg, 4) + source_encoding = node_mtg(internode) + new_parent = addchild!( + parent(internode), + MutableNodeMTG( + :+, + source_encoding.symbol, + source_encoding.index + 100, + source_encoding.scale, + ), + Dict{Symbol,Any}(), + ) + + cache_nodes!(mtg) + cache_nodes!(internode) + @test length(MultiScaleTreeGraph._node_store(mtg).subtree_index.traversal_cache_nodes) == 2 + added = addchild!(internode, MutableNodeMTG(:+, :Leaf, 2, 3), Dict{Symbol,Any}(:AfterCache => 123)) + + @test MultiScaleTreeGraph._maybe_traversal_cache(mtg) === nothing + @test MultiScaleTreeGraph._maybe_traversal_cache(internode) === nothing + @test MultiScaleTreeGraph._node_store(mtg).subtree_index.traversal_cache_nodes === nothing + @test added in traverse(mtg, node -> node) + @test :AfterCache in get_features(mtg).NAME + + old_parent = internode + cache_nodes!(mtg) + cache_nodes!(old_parent) + cache_nodes!(new_parent) + reparent!(added, new_parent) + + @test MultiScaleTreeGraph._maybe_traversal_cache(mtg) === nothing + @test MultiScaleTreeGraph._maybe_traversal_cache(old_parent) === nothing + @test MultiScaleTreeGraph._maybe_traversal_cache(new_parent) === nothing + @test added in traverse(new_parent, node -> node) + @test !(added in traverse(old_parent, node -> node)) + + cache_nodes!(mtg) + prune!(added) + @test MultiScaleTreeGraph._maybe_traversal_cache(mtg) === nothing + @test !(added in traverse(mtg, node -> node)) + + deleted_mtg = read_mtg(file) + cache_nodes!(deleted_mtg) + delete_node!(get_node(deleted_mtg, 5)) + @test MultiScaleTreeGraph._maybe_traversal_cache(deleted_mtg) === nothing + + sibling_mtg = read_mtg(file) + cache_nodes!(sibling_mtg) + old_length = length(sibling_mtg) + insert_sibling!( + get_node(sibling_mtg, 5), + MutableNodeMTG(:+, :Leaf, 2, 3), + _ -> Dict{Symbol,Any}(:SiblingAfterCache => 456) + ) + @test MultiScaleTreeGraph._maybe_traversal_cache(sibling_mtg) === nothing + @test length(sibling_mtg) == old_length + 1 + @test :SiblingAfterCache in get_features(sibling_mtg).NAME + + pruned_cache_mtg = read_mtg(file) + pruned_root = get_node(pruned_cache_mtg, 4) + cached_descendant = get_node(pruned_cache_mtg, 6) + cache_nodes!(cached_descendant) + @test MultiScaleTreeGraph._node_store(pruned_cache_mtg).subtree_index.traversal_cache_nodes !== nothing + prune!(pruned_root) + @test MultiScaleTreeGraph._maybe_traversal_cache(cached_descendant) === nothing + @test MultiScaleTreeGraph._node_store(pruned_cache_mtg).subtree_index.traversal_cache_nodes === nothing + + cycle_mtg = read_mtg(file) + cycle_ancestor = get_node(cycle_mtg, 4) + cycle_descendant = get_node(cycle_mtg, 6) + cycle_parent_ids = [ + node_id(node) => (parent(node) === nothing ? nothing : node_id(parent(node))) + for node in traverse(cycle_mtg, node -> node) + ] + @test_throws ArgumentError reparent!(cycle_ancestor, cycle_descendant) + @test [ + node_id(node) => (parent(node) === nothing ? nothing : node_id(parent(node))) + for node in traverse(cycle_mtg, node -> node) + ] == cycle_parent_ids + + indexed_mtg = read_mtg(file) + indexed_parent = get_node(indexed_mtg, 4) + replaced = indexed_parent[1] + replacement = Node( + max_id(indexed_mtg) + 1, + MutableNodeMTG(:+, :Leaf, 200, scale(replaced)), + Dict{Symbol,Any}(:IndexedAfterCache => true), + ) + cache_nodes!(indexed_mtg) + indexed_parent[1] = replacement + @test MultiScaleTreeGraph._maybe_traversal_cache(indexed_mtg) === nothing + @test parent(replacement) === indexed_parent + @test parent(replaced) === nothing + @test replacement in traverse(indexed_mtg, node -> node) + @test !(replaced in traverse(indexed_mtg, node -> node)) + + rechildren_mtg = read_mtg(file) + rechildren_parent = get_node(rechildren_mtg, 4) + independent_child = Node( + max_id(rechildren_mtg) + 1, + MutableNodeMTG(:+, :IndependentChild, 1, 3), + Dict{Symbol,Any}(), + ) + rechildren!(rechildren_parent, [independent_child]) + @test MultiScaleTreeGraph._node_store(independent_child) === + MultiScaleTreeGraph._node_store(rechildren_parent) + cache_nodes!(rechildren_parent) + addchild!( + independent_child, + MutableNodeMTG(:+, :CacheChild, 1, 4), + Dict{Symbol,Any}(), + ) + @test MultiScaleTreeGraph._maybe_traversal_cache(rechildren_parent) === nothing + + function assert_insert_cache_invalidation!(inserted) + @test MultiScaleTreeGraph._maybe_traversal_cache(inserted) === nothing + cache_nodes!(inserted) + registry = MultiScaleTreeGraph._node_store(inserted).subtree_index.traversal_cache_nodes + @test registry !== nothing + @test MultiScaleTreeGraph._traversal_cache_registered( + MultiScaleTreeGraph._node_store(inserted), inserted + ) + child = addchild!( + inserted, + MutableNodeMTG(:+, :CacheChild, 1, scale(inserted) + 1), + Dict{Symbol,Any}(), + ) + @test MultiScaleTreeGraph._maybe_traversal_cache(inserted) === nothing + @test child in traverse(inserted, node -> node) + end + + inserted_parent_mtg = read_mtg(file) + parent_target = get_node(inserted_parent_mtg, 4) + insert_parent!( + parent_target, + MutableNodeMTG(:/, :InsertedParent, 1, scale(parent_target)), + ) + assert_insert_cache_invalidation!(parent(parent_target)) + + inserted_root_mtg = read_mtg(file) + insert_parent!( + inserted_root_mtg, + MutableNodeMTG(:/, :InsertedRoot, 1, scale(inserted_root_mtg)), + ) + assert_insert_cache_invalidation!(parent(inserted_root_mtg)) + + inserted_sibling_mtg = read_mtg(file) + sibling_id = max_id(inserted_sibling_mtg) + 1 + insert_sibling!( + get_node(inserted_sibling_mtg, 5), + MutableNodeMTG(:+, :InsertedSibling, 1, 3), + ) + assert_insert_cache_invalidation!(get_node(inserted_sibling_mtg, sibling_id)) + + inserted_generation_mtg = read_mtg(file) + generation_target = get_node(inserted_generation_mtg, 4) + generation_id = max_id(inserted_generation_mtg) + 1 + insert_generation!( + generation_target, + MutableNodeMTG(:/, :InsertedGeneration, 1, scale(generation_target)), + ) + assert_insert_cache_invalidation!(get_node(inserted_generation_mtg, generation_id)) + + raw_parent = Node( + 1, + MutableNodeMTG(:/, :RawRoot, 1, 0), + Dict{Symbol,Any}(), + ) + RawNode = typeof(raw_parent) + raw_attrs = MultiScaleTreeGraph.ColumnarAttrs() + MultiScaleTreeGraph.bind_columnar_child!( + node_attributes(raw_parent), + raw_attrs, + 2, + :RawChild, + ) + raw_cache = Dict{String,Vector{RawNode}}("raw-cache" => RawNode[raw_parent]) + raw_child = RawNode( + 2, + raw_parent, + RawNode[], + MutableNodeMTG(:+, :RawChild, 1, 1), + raw_attrs, + raw_cache, + ) + raw_registry = MultiScaleTreeGraph._node_store(raw_parent).subtree_index.traversal_cache_nodes + @test raw_registry !== nothing + @test MultiScaleTreeGraph._traversal_cache_registered( + MultiScaleTreeGraph._node_store(raw_parent), raw_child + ) + addchild!(raw_parent, raw_child) + @test MultiScaleTreeGraph._maybe_traversal_cache(raw_child) === nothing + @test MultiScaleTreeGraph._node_store(raw_parent).subtree_index.traversal_cache_nodes === nothing + converted_raw = RawNode( + Int32(3), + nothing, + RawNode[], + MutableNodeMTG(:/, :ConvertedRaw, 1, 0), + MultiScaleTreeGraph.ColumnarAttrs(), + Dict(), + ) + @test node_id(converted_raw) == 3 + @test getfield(converted_raw, :traversal_cache) isa Dict{String,Vector{RawNode}} + + nonroot_columnar_mtg = read_mtg(file) + nonroot_columnar_branch = get_node(nonroot_columnar_mtg, 4) + cache_nodes!(nonroot_columnar_mtg) + columnarize!(nonroot_columnar_branch) + @test MultiScaleTreeGraph._node_store(nonroot_columnar_branch) === + MultiScaleTreeGraph._node_store(nonroot_columnar_mtg) + addchild!( + nonroot_columnar_branch, + max_id(nonroot_columnar_mtg) + 1, + MutableNodeMTG(:+, :ColumnarizedChild, 1, scale(nonroot_columnar_branch) + 1), + Dict{Symbol,Any}(:NonrootColumnarAfterCache => true), + ) + @test MultiScaleTreeGraph._maybe_traversal_cache(nonroot_columnar_mtg) === nothing + @test :NonrootColumnarAfterCache in get_features(nonroot_columnar_mtg).NAME + + source_tree = Node( + 100, + MutableNodeMTG(:/, :SourceRoot, 1, 0), + Dict{Symbol,Any}(), + ) + source_parent = addchild!( + source_tree, + 101, + MutableNodeMTG(:+, :SourceParent, 1, 1), + Dict{Symbol,Any}(), + ) + moved_subtree = addchild!( + source_parent, + 102, + MutableNodeMTG(:+, :MovedSubtree, 1, 2), + Dict{Symbol,Any}(:MovedValue => 102), + ) + moved_descendant = addchild!( + moved_subtree, + 103, + MutableNodeMTG(:+, :MovedDescendant, 1, 3), + Dict{Symbol,Any}(:MovedDescendantValue => 103), + ) + target_tree = Node( + 1, + MutableNodeMTG(:/, :TargetRoot, 1, 0), + Dict{Symbol,Any}(), + ) + target_parent = addchild!( + target_tree, + 2, + MutableNodeMTG(:+, :TargetParent, 1, 1), + Dict{Symbol,Any}(), + ) + source_store = MultiScaleTreeGraph._node_store(source_tree) + target_store = MultiScaleTreeGraph._node_store(target_tree) + cache_nodes!(source_tree) + cache_nodes!(moved_subtree) + cache_nodes!(moved_descendant) + cache_nodes!(target_tree) + reparent!(moved_subtree, target_parent) + @test MultiScaleTreeGraph._maybe_traversal_cache(source_tree) === nothing + @test MultiScaleTreeGraph._maybe_traversal_cache(moved_subtree) === nothing + @test MultiScaleTreeGraph._maybe_traversal_cache(moved_descendant) !== nothing + @test MultiScaleTreeGraph._maybe_traversal_cache(target_tree) === nothing + @test parent(moved_subtree) === target_parent + @test !(moved_subtree in traverse(source_tree, identity, type=typeof(source_tree))) + @test moved_subtree in traverse(target_tree, identity, type=typeof(target_tree)) + @test MultiScaleTreeGraph._node_store(moved_subtree) === target_store + @test MultiScaleTreeGraph._node_store(moved_descendant) === target_store + @test source_store.node_bucket[node_id(moved_subtree)] == 0 + @test source_store.node_bucket[node_id(moved_descendant)] == 0 + @test source_store.subtree_index.traversal_cache_nodes === nothing + cache_nodes!(target_tree) + addchild!( + moved_descendant, + 104, + MutableNodeMTG(:+, :MovedChild, 1, 4), + Dict{Symbol,Any}(:CrossStoreAfterCache => true), + ) + @test MultiScaleTreeGraph._maybe_traversal_cache(target_tree) === nothing + @test MultiScaleTreeGraph._maybe_traversal_cache(moved_descendant) === nothing + @test :CrossStoreAfterCache in get_features(target_tree).NAME + + conflict_tree = Node( + 1, + MutableNodeMTG(:/, :ConflictRoot, 1, 0), + Dict{Symbol,Any}(), + ) + conflict_old_child = addchild!( + conflict_tree, + 2, + MutableNodeMTG(:+, :ConflictChild, 1, 1), + Dict{Symbol,Any}(), + ) + conflicting_replacement = Node( + 2, + MutableNodeMTG(:/, :ReplacementRoot, 1, 0), + Dict{Symbol,Any}(), + ) + conflict_children_before = copy(children(conflict_tree)) + conflict_store_before = MultiScaleTreeGraph._node_store(conflict_tree) + replacement_store_before = MultiScaleTreeGraph._node_store(conflicting_replacement) + @test_throws ArgumentError conflict_tree[1] = conflicting_replacement + @test children(conflict_tree) == conflict_children_before + @test parent(conflict_old_child) === conflict_tree + @test parent(conflicting_replacement) === nothing + @test MultiScaleTreeGraph._node_store(conflict_tree) === conflict_store_before + @test MultiScaleTreeGraph._node_store(conflicting_replacement) === replacement_store_before + + rechildren_target = Node( + 1, + MutableNodeMTG(:/, :RechildrenTarget, 1, 0), + Dict{Symbol,Any}(), + ) + rechildren_old = addchild!( + rechildren_target, + 2, + MutableNodeMTG(:+, :RechildrenOld, 1, 1), + Dict{Symbol,Any}(), + ) + incoming_a = Node( + 100, + MutableNodeMTG(:/, :IncomingA, 1, 0), + Dict{Symbol,Any}(), + ) + incoming_b = Node( + 100, + MutableNodeMTG(:/, :IncomingB, 1, 0), + Dict{Symbol,Any}(), + ) + rechildren_before = copy(children(rechildren_target)) + target_store_before = MultiScaleTreeGraph._node_store(rechildren_target) + incoming_a_store_before = MultiScaleTreeGraph._node_store(incoming_a) + incoming_b_store_before = MultiScaleTreeGraph._node_store(incoming_b) + @test_throws ArgumentError rechildren!(rechildren_target, [incoming_a, incoming_b]) + @test children(rechildren_target) == rechildren_before + @test parent(rechildren_old) === rechildren_target + @test parent(incoming_a) === nothing + @test parent(incoming_b) === nothing + @test MultiScaleTreeGraph._node_store(rechildren_target) === target_store_before + @test MultiScaleTreeGraph._node_store(incoming_a) === incoming_a_store_before + @test MultiScaleTreeGraph._node_store(incoming_b) === incoming_b_store_before +end diff --git a/test/test-descendants.jl b/test/test-descendants.jl index 013b7051..26176720 100644 --- a/test/test-descendants.jl +++ b/test/test-descendants.jl @@ -70,8 +70,9 @@ end @testset "descendants clear error on mixed columnar stores" begin mtg_a = read_mtg("files/simple_plant.mtg") mtg_b = read_mtg("files/simple_plant.mtg") - # Bypass addchild! on purpose to build an incoherent tree (mixed stores). - reparent!(mtg_b, mtg_a) + # Bypass the public topology mutators on purpose to build an incoherent tree. + push!(children(mtg_a), mtg_b) + setfield!(mtg_b, :parent, mtg_a) err = try descendants(mtg_a, :Width) diff --git a/test/test-nodes.jl b/test/test-nodes.jl index 7b4bf427..1c87fde8 100644 --- a/test/test-nodes.jl +++ b/test/test-nodes.jl @@ -161,11 +161,17 @@ end VERSION >= v"1.7" && @test_throws "The parent node has an MTG encoding of type `MutableNodeMTG`, but the MTG encoding you provide is of type `NodeMTG`, please make sure they are the same." Node(mtg, NodeMTG(:/, :Branch, 1, 2)) end -@testset "addchild! re-columnarizes when attaching a root subtree" begin +@testset "addchild! rejects colliding root-subtree ids transactionally" begin mtg_a = read_mtg(file) mtg_b = read_mtg(file) - addchild!(mtg_a, mtg_b; force=true) - @test_nowarn descendants(mtg_a, :Width) + children_before = copy(children(mtg_a)) + store_a_before = MultiScaleTreeGraph._node_store(mtg_a) + store_b_before = MultiScaleTreeGraph._node_store(mtg_b) + @test_throws ArgumentError addchild!(mtg_a, mtg_b; force=true) + @test children(mtg_a) == children_before + @test parent(mtg_b) === nothing + @test MultiScaleTreeGraph._node_store(mtg_a) === store_a_before + @test MultiScaleTreeGraph._node_store(mtg_b) === store_b_before end @testset "new child node attributes are in the columnar store" begin From 51bf756a4e1a23c540897a2243eb1b9244f497d3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Mon, 24 Aug 2026 15:46:28 +0200 Subject: [PATCH 03/13] perf: streamline columnar MTG I/O --- src/compute_MTG/summary.jl | 159 +++++++++-- src/read_MTG/parse_mtg.jl | 122 ++++++--- src/read_MTG/utils-string.jl | 20 +- src/types/Attributes.jl | 9 +- src/write_mtg/write_mtg.jl | 509 +++++++++++++++++++++++++++++++++-- test/test-columnar.jl | 58 ++++ test/test-read_mtg.jl | 166 ++++++++++++ test/test-summary.jl | 184 +++++++++++++ test/test-write_mtg.jl | 429 +++++++++++++++++++++++++++++ 9 files changed, 1569 insertions(+), 87 deletions(-) diff --git a/src/compute_MTG/summary.jl b/src/compute_MTG/summary.jl index ecc83fe2..1f7e189f 100644 --- a/src/compute_MTG/summary.jl +++ b/src/compute_MTG/summary.jl @@ -35,13 +35,21 @@ function get_description(mtg) return nothing end -""" - get_features(mtg) +@inline function _mtg_feature_type(value) + T = typeof(value) + if T <: AbstractFloat + return "REAL" + elseif T <: Bool + return "BOOLEAN" + elseif T <: Integer + return "INT" + elseif T <: Date + return "DD/MM/YY" + end + return "STRING" +end -Compute the mtg features section based on its attributes. Usefull after having computed new attributes -in the mtg. -""" -function get_features(mtg) +function _get_features_legacy(mtg) names_ = Symbol[] types_ = String[] seen = Set{Tuple{Symbol,String}}() @@ -53,18 +61,7 @@ function get_features(mtg) continue end - typ = - if T <: AbstractFloat - "REAL" - elseif T <: Bool - "BOOLEAN" - elseif T <: Integer - "INT" - elseif T <: Date - "DD/MM/YY" - else - "STRING" - end + typ = _mtg_feature_type(value) row = (Symbol(name), typ) if !(row in seen) @@ -78,6 +75,132 @@ function get_features(mtg) ColumnTable(Symbol[:NAME, :TYPE], AbstractVector[names_, types_]) end +function _record_column_features!( + first_positions::Dict{Tuple{Symbol,String},Tuple{Int,Int}}, + positions::Dict{Int,Int}, + store::MTGAttributeStore, + bid::Int, + bucket::SymbolBucket, + col_idx::Int, + column::Column{T}, +) where {T} + @inbounds for row in eachindex(bucket.row_to_node) + nodeid = bucket.row_to_node[row] + position = get(positions, nodeid, 0) + position == 0 && continue + + # A duplicated or stale reverse mapping could otherwise expose a value + # that `pairs(node_attributes(node))` would never visit. + nodeid <= length(store.node_bucket) || return false + nodeid <= length(store.node_row) || return false + store.node_bucket[nodeid] == bid || return false + store.node_row[nodeid] == row || return false + get(bucket.node_to_row, nodeid, 0) == row || return false + isassigned(column.data, row) || return false + + value = column.data[row] + value_type = typeof(value) + ((value_type <: AbstractVector) || (value_type <: Nothing)) && continue + feature = (column.name, _mtg_feature_type(value)) + candidate = (position, col_idx) + previous = get(first_positions, feature, nothing) + if previous === nothing || candidate < previous + first_positions[feature] = candidate + end + end + return true +end + +function _get_features_columnar(mtg) + ordered_nodes = Vector{typeof(mtg)}() + traverse!(mtg) do node + push!(ordered_nodes, node) + end + + if isempty(ordered_nodes) + return ColumnTable( + Symbol[:NAME, :TYPE], AbstractVector[Symbol[], String[]] + ) + end + + first_attrs = node_attributes(first(ordered_nodes)) + first_attrs isa ColumnarAttrs && _isbound(first_attrs) || return nothing + store = first_attrs.ref.store::MTGAttributeStore + positions = Dict{Int,Int}() + sizehint!(positions, length(ordered_nodes)) + used_buckets = falses(length(store.buckets)) + + # The traversal sequence is authoritative: `traverse!` may use a public, + # user-supplied no-filter cache. Validate every reference before touching the + # store directly; malformed or mixed stores retain the legacy behavior. + @inbounds for position in eachindex(ordered_nodes) + node = ordered_nodes[position] + attrs = node_attributes(node) + attrs isa ColumnarAttrs && _isbound(attrs) || return nothing + attrs.ref.store === store || return nothing + + nodeid = node_id(node) + nodeid > 0 || return nothing + attrs.ref.node_id == nodeid || return nothing + nodeid <= length(store.node_bucket) || return nothing + nodeid <= length(store.node_row) || return nothing + haskey(positions, nodeid) && return nothing + + bid = store.node_bucket[nodeid] + row = store.node_row[nodeid] + 1 <= bid <= length(store.buckets) || return nothing + bucket = store.buckets[bid] + 1 <= row <= length(bucket.row_to_node) || return nothing + bucket.row_to_node[row] == nodeid || return nothing + get(bucket.node_to_row, nodeid, 0) == row || return nothing + positions[nodeid] = position + used_buckets[bid] = true + end + + first_positions = Dict{Tuple{Symbol,String},Tuple{Int,Int}}() + for (bid, bucket) in pairs(store.buckets) + used_buckets[bid] || continue + nrows = length(bucket.row_to_node) + length(bucket.columns) == length(bucket.col_types) || return nothing + @inbounds for col_idx in eachindex(bucket.columns) + column = _column(bucket, col_idx) + column isa Column || return nothing + get(bucket.col_index, column.name, 0) == col_idx || return nothing + length(column.data) == nrows || return nothing + column.name in (:description, :symbols, :scales) && continue + _record_column_features!( + first_positions, + positions, + store, + bid, + bucket, + col_idx, + column, + ) || return nothing + end + end + + features = collect(keys(first_positions)) + sort!(features; by=feature -> first_positions[feature]) + names_ = Vector{Symbol}(undef, length(features)) + types_ = Vector{String}(undef, length(features)) + @inbounds for i in eachindex(features) + names_[i], types_[i] = features[i] + end + return ColumnTable(Symbol[:NAME, :TYPE], AbstractVector[names_, types_]) +end + +""" + get_features(mtg) + +Compute the mtg features section based on its attributes. Usefull after having computed new attributes +in the mtg. +""" +function get_features(mtg) + features = _get_features_columnar(mtg) + return features === nothing ? _get_features_legacy(mtg) : features +end + """ scales(mtg) diff --git a/src/read_MTG/parse_mtg.jl b/src/read_MTG/parse_mtg.jl index 8e89bcf2..513867da 100644 --- a/src/read_MTG/parse_mtg.jl +++ b/src/read_MTG/parse_mtg.jl @@ -126,96 +126,129 @@ function parse_MTG_node(l) (link, symbol, index) end +@noinline function _mtg_node_data_text(node_data, diagnostic_start::Integer) + return join(view(node_data, diagnostic_start:lastindex(node_data)), "\t") +end -""" - -Parse MTG node attributes names, values and type - -# Arguments -- `node_data::String`: A splitted mtg node data (attributes) -- `features::ColumnTable`: The parsed features table -- `attr_column_start::Integer`: The index of the column of the first attribute -- `line::Integer`: The current line of the mtg file -- `force::Bool`: force data reading even if errors are met during conversion ? - -# Return - -A list of attributes - -""" -function parse_MTG_node_attr(node_data, features, feature_names, attr_column_start, line; force=false) - +function _parse_MTG_node_attr_fields( + node_data, + features, + feature_names, + attr_column_start::Integer, + diagnostic_start::Integer, + line; + force=false, +) if length(node_data) < attr_column_start return init_empty_attr() end - node_data_attr = node_data[attr_column_start:end] + node_data_attr_count = length(node_data) - attr_column_start + 1 - if length(node_data_attr) > size(features)[1] + if node_data_attr_count > size(features)[1] error("Found more columns for features in MTG than declared in the FEATURE section", - ". Please check line ", line, " of the MTG:\n", join(node_data, "\t")) + ". Please check line ", line, " of the MTG:\n", + _mtg_node_data_text(node_data, diagnostic_start)) end node_attr = Dict{Symbol,Any}() - sizehint!(node_attr, length(node_data_attr)) + sizehint!(node_attr, node_data_attr_count) node_type = features.TYPE - # node_data_attr is always read in order so names and types correspond to values in features - for i in eachindex(node_data_attr) + # Attribute fields are always read in order so names and types correspond to + # values in FEATURES. Keep the intermediate Dict and ColumnarAttrs copy: its + # resulting physical column order is observable through `pairs` and writing. + for i in 1:node_data_attr_count + field_index = attr_column_start + i - 1 + field = node_data[field_index] feature_name = feature_names[i] - if node_data_attr[i] == "" || node_data_attr[i] == "NA" + if field == "" || field == "NA" continue end if node_type[i] == "INT" try - node_attr[feature_name] = parse(Int, node_data_attr[i]) + node_attr[feature_name] = parse(Int, field) catch e if !force error("Found issue in the MTG when converting column $(features[i,1]) ", - "with value $(node_data_attr[i]) into Integer.", - " Please check line ", line, " of the MTG:\n", join(node_data, "\t")) + "with value $(field) into Integer.", + " Please check line ", line, " of the MTG:\n", + _mtg_node_data_text(node_data, diagnostic_start)) end end elseif node_type[i] == "BOOLEAN" try - node_attr[feature_name] = parse(Bool, node_data_attr[i]) + node_attr[feature_name] = parse(Bool, field) catch e if !force error("Found issue in the MTG when converting column $(features[i,1]) ", - "with value $(node_data_attr[i]) into Boolean.", - " Please check line ", line, " of the MTG:\n", join(node_data, "\t")) + "with value $(field) into Boolean.", + " Please check line ", line, " of the MTG:\n", + _mtg_node_data_text(node_data, diagnostic_start)) end end elseif node_type[i] == "DD/MM/YY" try - node_attr[feature_name] = Date(node_data_attr[i], dateformat"d/m/y") + node_attr[feature_name] = Date(field, dateformat"d/m/y") catch e if !force error("Found issue in the MTG when converting column $(features[i,1]) ", - "with value $(node_data_attr[i]) into a date with format 'day/month/year'.", - " Please check line ", line, " of the MTG:\n", join(node_data, "\t")) + "with value $(field) into a date with format 'day/month/year'.", + " Please check line ", line, " of the MTG:\n", + _mtg_node_data_text(node_data, diagnostic_start)) end end elseif node_type[i] == "REAL" || (node_type[i] == "ALPHA" && feature_name in (:Width, :Length)) try - node_attr[feature_name] = parse(Float64, node_data_attr[i]) + node_attr[feature_name] = parse(Float64, field) catch e if !force error("Found issue in the MTG when converting column $(features[i,1]) ", - "with value $(node_data_attr[i]) into Floating point number.", - " Please check line ", line, " of the MTG:\n", join(node_data, "\t")) + "with value $(field) into Floating point number.", + " Please check line ", line, " of the MTG:\n", + _mtg_node_data_text(node_data, diagnostic_start)) end end else - node_attr[feature_name] = node_data_attr[i] + node_attr[feature_name] = field end end ColumnarAttrs(node_attr) end +""" + +Parse MTG node attributes names, values and type + +# Arguments +- `node_data::String`: A splitted mtg node data (attributes) +- `features::ColumnTable`: The parsed features table +- `attr_column_start::Integer`: The index of the column of the first attribute +- `line::Integer`: The current line of the mtg file +- `force::Bool`: force data reading even if errors are met during conversion ? + +# Return + +A list of attributes + +""" +function parse_MTG_node_attr( + node_data, features, feature_names, attr_column_start, line; force=false +) + return _parse_MTG_node_attr_fields( + node_data, + features, + feature_names, + attr_column_start, + firstindex(node_data), + line; + force=force, + ) +end + init_empty_attr() = ColumnarAttrs() @@ -229,7 +262,6 @@ function parse_line_to_node!(tree_dict, l, line, attr_column_start, last_node_co splitted_MTG = split(l[1], "\t") node_column = findfirst(x -> length(x) > 0, splitted_MTG) - node_data = splitted_MTG[node_column:end] if attr_column_start < node_column error( @@ -243,13 +275,19 @@ function parse_line_to_node!(tree_dict, l, line, attr_column_start, last_node_co ) end - node_attr_column_start = attr_column_start - node_column + 1 - node = split_MTG_elements(node_data[1]) + node = split_MTG_elements(splitted_MTG[node_column]) node, shared = expand_node!(node, 1) # Get node attributes: if features !== nothing - node_attr = parse_MTG_node_attr(node_data, features, feature_names, node_attr_column_start, line) + node_attr = _parse_MTG_node_attr_fields( + splitted_MTG, + features, + feature_names, + attr_column_start, + node_column, + line, + ) else # if there are no attribute in the MTG, we create an empty attribute: node_attr = init_empty_attr() diff --git a/src/read_MTG/utils-string.jl b/src/read_MTG/utils-string.jl index 6e6d450d..31cab7ab 100644 --- a/src/read_MTG/utils-string.jl +++ b/src/read_MTG/utils-string.jl @@ -1,3 +1,10 @@ +const _MTG_SECTION_REGEX = r"(CODE|CLASSES|DESCRIPTION|FEATURES|MTG)[[:blank:]]*:" +const _MTG_CODE_SECTION_REGEX = r"CODE[[:blank:]]*:" +const _MTG_CLASSES_SECTION_REGEX = r"CLASSES[[:blank:]]*:" +const _MTG_DESCRIPTION_SECTION_REGEX = r"DESCRIPTION[[:blank:]]*:" +const _MTG_FEATURES_SECTION_REGEX = r"FEATURES[[:blank:]]*:" +const _MTG_MTG_SECTION_REGEX = r"MTG[[:blank:]]*:" + """ issection(string) @@ -10,10 +17,7 @@ Is a string part of an MTG section ? Returns `true` if it does, `false` otherwis issection("CODE :") ``` """ -function issection(string) - sections = ("CODE", "CLASSES", "DESCRIPTION", "FEATURES", "MTG") - occursin(Regex("($(join(sections, "|")))[[:blank:]]*:"), string) -end +issection(string) = occursin(_MTG_SECTION_REGEX, string) """ issection(string,section) @@ -31,6 +35,14 @@ issection("CODE :", "CODE") ``` """ function issection(string, section) + if section isa AbstractString + section == "CODE" && return occursin(_MTG_CODE_SECTION_REGEX, string) + section == "CLASSES" && return occursin(_MTG_CLASSES_SECTION_REGEX, string) + section == "DESCRIPTION" && + return occursin(_MTG_DESCRIPTION_SECTION_REGEX, string) + section == "FEATURES" && return occursin(_MTG_FEATURES_SECTION_REGEX, string) + section == "MTG" && return occursin(_MTG_MTG_SECTION_REGEX, string) + end occursin(Regex("$section[[:blank:]]*:"), string) end diff --git a/src/types/Attributes.jl b/src/types/Attributes.jl index 030f21e8..a1f0fb32 100644 --- a/src/types/Attributes.jl +++ b/src/types/Attributes.jl @@ -679,11 +679,12 @@ function Base.iterate(attrs::ColumnarAttrs, state=nothing) if !_isbound(attrs) return state === nothing ? iterate(attrs.staged) : iterate(attrs.staged, state) end - k = keys(attrs) + store, bid, row = _bound_store_bid_row(attrs.ref) + bucket = store.buckets[bid] i = state === nothing ? 1 : state - i > length(k) && return nothing - key = k[i] - return (key => get(attrs, key, nothing), i + 1) + i > length(bucket.columns) && return nothing + column = _column(bucket, i) + return (column.name => column.data[row], i + 1) end function Base.pop!(attrs::ColumnarAttrs, key, default=nothing) diff --git a/src/write_mtg/write_mtg.jl b/src/write_mtg/write_mtg.jl index 4c0a99cb..0c4b10e4 100644 --- a/src/write_mtg/write_mtg.jl +++ b/src/write_mtg/write_mtg.jl @@ -11,6 +11,8 @@ Write an mtg file to disk. - `classes`: the classes section - `description`: the description section - `features`: the features section +- `feature_overrides`: optional mapping from declared feature names (`Symbol`) to + vectors aligned with MTG preorder; only serialized values are replaced # Note @@ -29,22 +31,34 @@ write_mtg("test.mtg",mtg) function write_mtg(file, mtg; kwargs...) kwargs = (; kwargs...) - if !haskey(kwargs, :classes) - classes = get_classes(mtg) - end - - if !haskey(kwargs, :description) - description = nothing - end - - if !haskey(kwargs, :features) - features = get_features(mtg) + classes = haskey(kwargs, :classes) ? kwargs.classes : get_classes(mtg) + description = haskey(kwargs, :description) ? kwargs.description : nothing + features = haskey(kwargs, :features) ? kwargs.features : get_features(mtg) + feature_overrides = haskey(kwargs, :feature_overrides) ? + kwargs.feature_overrides : nothing + + if feature_overrides === nothing + write_mtg(file, mtg, classes, description, features) + else + write_mtg( + file, + mtg, + classes, + description, + features; + feature_overrides=feature_overrides, + ) end - - write_mtg(file, mtg, classes, description, features) end -function write_mtg(file, mtg, classes, description, features) +function write_mtg( + file, + mtg, + classes, + description, + features; + feature_overrides=nothing, +) @info "Writing mtg to $file" open(file, "w") do io # Code section: @@ -92,13 +106,65 @@ function write_mtg(file, mtg, classes, description, features) writedlm(io, ["MTG:"]) layout = _mtg_write_layout(mtg) feature_columns = _mtg_write_feature_columns(mtg, features) - feature_values = _mtg_write_feature_values(layout, feature_columns) - entity_feature, output_features, mtg_colnames = - _mtg_write_projection(layout, feature_columns) - writedlm(io, reshape(mtg_colnames, (1, :)), quotes=false) - _write_mtg_rows( - io, layout, feature_values, entity_feature, output_features + prepared_overrides = _mtg_write_feature_overrides( + layout, + feature_columns, + feature_overrides, ) + columnar_context = _mtg_columnar_write_context(layout, feature_columns) + if columnar_context === nothing + feature_values = prepared_overrides === nothing ? + _mtg_write_feature_values(layout, feature_columns) : + _mtg_write_feature_values( + layout, + feature_columns, + prepared_overrides, + ) + entity_feature, output_features, mtg_colnames = + _mtg_write_projection(layout, feature_columns) + writedlm(io, reshape(mtg_colnames, (1, :)), quotes=false) + _write_mtg_rows( + io, layout, feature_values, entity_feature, output_features + ) + else + # Date conversion remains before the ENTITY-CODE header so conversion + # failures preserve the historical partial-file boundary. + date_values = prepared_overrides === nothing ? + _mtg_write_columnar_date_values( + feature_columns, + columnar_context, + ) : + _mtg_write_columnar_date_values( + feature_columns, + columnar_context, + prepared_overrides, + ) + entity_feature, output_features, mtg_colnames = + _mtg_write_projection(layout, feature_columns) + writedlm(io, reshape(mtg_colnames, (1, :)), quotes=false) + if prepared_overrides === nothing + _write_mtg_columnar_rows( + io, + layout, + feature_columns, + columnar_context, + date_values, + entity_feature, + output_features, + ) + else + _write_mtg_columnar_rows( + io, + layout, + feature_columns, + columnar_context, + date_values, + entity_feature, + output_features, + prepared_overrides, + ) + end + end end end @@ -132,6 +198,70 @@ struct _MTGWriteFeatureColumns plans::Vector{Union{Nothing,ColumnarQueryPlan}} end +struct _MTGWriteFeatureOverrides + columns::Vector{Any} +end + +@inline _mtg_write_feature_overrides( + ::_MTGWriteLayout, + ::_MTGWriteFeatureColumns, + ::Nothing, +) = nothing + +function _mtg_write_feature_overrides( + layout::_MTGWriteLayout, + features::_MTGWriteFeatureColumns, + feature_overrides, +) + (feature_overrides isa AbstractDict || feature_overrides isa NamedTuple) || + throw(ArgumentError( + "feature_overrides must map Symbol feature names to AbstractVector values", + )) + isempty(feature_overrides) && return nothing + + feature_positions = Dict{Symbol,Int}( + key => index for (index, key) in pairs(features.keys) + ) + columns = Vector{Any}(undef, length(features.keys)) + fill!(columns, nothing) + for (key, values) in pairs(feature_overrides) + key isa Symbol || throw(ArgumentError( + "feature_overrides keys must be Symbols; got $(repr(key))", + )) + values isa AbstractVector || throw(ArgumentError( + "feature_overrides[$(repr(key))] must be an AbstractVector; got $(typeof(values))", + )) + feature_index = get(feature_positions, key, 0) + iszero(feature_index) && throw(ArgumentError( + "feature_overrides contains undeclared MTG feature $(repr(key))", + )) + length(values) == length(layout.nodes) || throw(ArgumentError( + "feature_overrides[$(repr(key))] has length $(length(values)); " * + "expected $(length(layout.nodes)) values aligned with MTG preorder", + )) + axes(values, 1) == Base.OneTo(length(layout.nodes)) || throw(ArgumentError( + "feature_overrides[$(repr(key))] must use one-based axes aligned " * + "with MTG preorder; got $(axes(values, 1))", + )) + columns[feature_index] = values + end + return _MTGWriteFeatureOverrides(columns) +end + +struct _MTGColumnarWriteContext + store::MTGAttributeStore + bucket_ids::Vector{Int} + rows::Vector{Int} +end + +function _mtg_validate_column_rows(column::Column{T}, rows::Vector{Int}) where {T} + @inbounds for row in rows + row <= length(column.data) || return false + isassigned(column.data, row) || return false + end + return true +end + function _mtg_write_layout(mtg) nodes = Vector{typeof(mtg)}() leads = Int[] @@ -197,6 +327,149 @@ function _mtg_write_feature_columns(mtg, features) return _MTGWriteFeatureColumns(names_, keys_, is_date, plans) end +function _mtg_columnar_write_context( + layout::_MTGWriteLayout, features::_MTGWriteFeatureColumns +) + isempty(layout.nodes) && return nothing + isempty(features.keys) && return nothing + nfeatures = length(features.keys) + length(features.names) == nfeatures || return nothing + length(features.is_date) == nfeatures || return nothing + length(features.plans) == nfeatures || return nothing + + first_attrs = node_attributes(first(layout.nodes)) + first_attrs isa ColumnarAttrs && _isbound(first_attrs) || return nothing + store = first_attrs.ref.store::MTGAttributeStore + nbuckets = length(store.buckets) + + @inbounds for j in eachindex(features.keys) + plan = features.plans[j] + plan isa ColumnarQueryPlan || return nothing + plan.store === store || return nothing + plan.key == features.keys[j] || return nothing + length(plan.col_idx_by_bucket) == nbuckets || return nothing + end + + bucket_ids = Vector{Int}(undef, length(layout.nodes)) + rows = Vector{Int}(undef, length(layout.nodes)) + rows_by_bucket = [Int[] for _ in 1:nbuckets] + @inbounds for i in eachindex(layout.nodes) + node = layout.nodes[i] + attrs = node_attributes(node) + attrs isa ColumnarAttrs && _isbound(attrs) || return nothing + attrs.ref.store === store || return nothing + + nodeid = node_id(node) + nodeid > 0 || return nothing + attrs.ref.node_id == nodeid || return nothing + nodeid <= length(store.node_bucket) || return nothing + nodeid <= length(store.node_row) || return nothing + bid = store.node_bucket[nodeid] + row = store.node_row[nodeid] + 1 <= bid <= nbuckets || return nothing + bucket = store.buckets[bid] + 1 <= row <= length(bucket.row_to_node) || return nothing + bucket.row_to_node[row] == nodeid || return nothing + get(bucket.node_to_row, nodeid, 0) == row || return nothing + + bucket_ids[i] = bid + rows[i] = row + push!(rows_by_bucket[bid], row) + end + + @inbounds for bid in eachindex(rows_by_bucket) + bucket_rows = rows_by_bucket[bid] + isempty(bucket_rows) && continue + bucket = store.buckets[bid] + length(bucket.columns) == length(bucket.col_types) || return nothing + + for j in eachindex(features.keys) + plan = features.plans[j]::ColumnarQueryPlan + col_idx = plan.col_idx_by_bucket[bid] + get(bucket.col_index, features.keys[j], 0) == col_idx || return nothing + col_idx == 0 && continue + 1 <= col_idx <= length(bucket.columns) || return nothing + column = _column(bucket, col_idx) + column isa Column || return nothing + column.name == features.keys[j] || return nothing + _mtg_validate_column_rows(column, bucket_rows) || return nothing + end + end + + return _MTGColumnarWriteContext(store, bucket_ids, rows) +end + +@inline function _mtg_column_value(column::Column{T}, row::Int) where {T} + return @inbounds column.data[row] +end + +@inline function _mtg_override_value(values::AbstractVector{T}, row::Int) where {T} + return @inbounds values[row] +end + +@inline function _mtg_columnar_feature_value( + features::_MTGWriteFeatureColumns, + context::_MTGColumnarWriteContext, + feature_index::Int, + node_index::Int, +) + plan = features.plans[feature_index]::ColumnarQueryPlan + bid = @inbounds context.bucket_ids[node_index] + col_idx = @inbounds plan.col_idx_by_bucket[bid] + col_idx == 0 && return nothing + row = @inbounds context.rows[node_index] + column = @inbounds context.store.buckets[bid].columns[col_idx] + return _mtg_column_value(column, row) +end + +function _mtg_write_columnar_date_values( + features::_MTGWriteFeatureColumns, context::_MTGColumnarWriteContext +) + values = Vector{Union{Nothing,Vector{String}}}(undef, length(features.keys)) + fill!(values, nothing) + + # Match the legacy conversion order exactly: feature-major, then node-major. + @inbounds for j in eachindex(features.keys) + features.is_date[j] || continue + column = Vector{String}(undef, length(context.rows)) + for i in eachindex(context.rows) + value = _mtg_columnar_feature_value(features, context, j, i) + column[i] = value === nothing ? "" : format(value, dateformat"d/m/Y") + end + values[j] = column + end + return values +end + +function _mtg_write_columnar_date_values( + features::_MTGWriteFeatureColumns, + context::_MTGColumnarWriteContext, + overrides::_MTGWriteFeatureOverrides, +) + values = Vector{Union{Nothing,Vector{String}}}(undef, length(features.keys)) + fill!(values, nothing) + + # Match the legacy conversion order exactly: feature-major, then node-major. + @inbounds for j in eachindex(features.keys) + features.is_date[j] || continue + override = overrides.columns[j] + column = Vector{String}(undef, length(context.rows)) + if override === nothing + for i in eachindex(context.rows) + value = _mtg_columnar_feature_value(features, context, j, i) + column[i] = value === nothing ? "" : format(value, dateformat"d/m/Y") + end + else + for i in eachindex(context.rows) + value = _mtg_override_value(override, i) + column[i] = value === nothing ? "" : format(value, dateformat"d/m/Y") + end + end + values[j] = column + end + return values +end + function _mtg_write_feature_values( layout::_MTGWriteLayout, features::_MTGWriteFeatureColumns ) @@ -229,6 +502,46 @@ function _mtg_write_feature_values( return values end +function _mtg_write_feature_values( + layout::_MTGWriteLayout, + features::_MTGWriteFeatureColumns, + overrides::_MTGWriteFeatureOverrides, +) + values = [Vector{Any}(undef, length(layout.nodes)) for _ in eachindex(features.keys)] + + # Preserve the fallback lookup and error order for every feature before + # replacing selected values with vectors aligned to this exact layout. + @inbounds for j in eachindex(features.keys) + column = values[j] + key = features.keys[j] + plan = features.plans[j] + for i in eachindex(layout.nodes) + column[i] = unsafe_getindex(layout.nodes[i], key, plan) + end + override = overrides.columns[j] + if override !== nothing + for i in eachindex(layout.nodes) + column[i] = _mtg_override_value(override, i) + end + end + end + + # Keep the historical conversion order: all values are collected before date + # formatting is attempted, and `nothing` is represented by an empty field. + @inbounds for j in eachindex(values) + column = values[j] + if features.is_date[j] + for i in eachindex(column) + value = column[i] + column[i] = value === nothing ? "" : format(value, dateformat"d/m/Y") + end + else + replace!(column, nothing => "") + end + end + return values +end + function _mtg_write_projection( layout::_MTGWriteLayout, features::_MTGWriteFeatureColumns ) @@ -289,6 +602,164 @@ function _write_mtg_rows( return nothing end +@inline function _write_mtg_column_value(io, column::Column{T}, row::Int) where {T} + value = @inbounds column.data[row] + value === nothing || print(io, value) + return nothing +end + +@inline function _write_mtg_columnar_feature( + io, + features::_MTGWriteFeatureColumns, + context::_MTGColumnarWriteContext, + date_values::Vector{Union{Nothing,Vector{String}}}, + feature_index::Int, + node_index::Int, +) + if features.is_date[feature_index] + column = date_values[feature_index]::Vector{String} + print(io, column[node_index]) + return nothing + end + + plan = features.plans[feature_index]::ColumnarQueryPlan + bid = @inbounds context.bucket_ids[node_index] + col_idx = @inbounds plan.col_idx_by_bucket[bid] + col_idx == 0 && return nothing + row = @inbounds context.rows[node_index] + column = @inbounds context.store.buckets[bid].columns[col_idx] + _write_mtg_column_value(io, column, row) + return nothing +end + +@inline function _write_mtg_columnar_feature( + io, + features::_MTGWriteFeatureColumns, + context::_MTGColumnarWriteContext, + date_values::Vector{Union{Nothing,Vector{String}}}, + feature_index::Int, + node_index::Int, + overrides::_MTGWriteFeatureOverrides, +) + if features.is_date[feature_index] + column = date_values[feature_index]::Vector{String} + print(io, column[node_index]) + return nothing + end + + override = @inbounds overrides.columns[feature_index] + if override === nothing + plan = features.plans[feature_index]::ColumnarQueryPlan + bid = @inbounds context.bucket_ids[node_index] + col_idx = @inbounds plan.col_idx_by_bucket[bid] + col_idx == 0 && return nothing + row = @inbounds context.rows[node_index] + column = @inbounds context.store.buckets[bid].columns[col_idx] + _write_mtg_column_value(io, column, row) + else + value = _mtg_override_value(override, node_index) + value === nothing || print(io, value) + end + return nothing +end + +function _write_mtg_columnar_rows( + io, + layout::_MTGWriteLayout, + features::_MTGWriteFeatureColumns, + context::_MTGColumnarWriteContext, + date_values::Vector{Union{Nothing,Vector{String}}}, + entity_feature::Union{Nothing,Int}, + output_features::Vector{Int}, +) + buffer = IOBuffer(; sizehint=16 * 1024) + @inbounds for i in eachindex(layout.nodes) + node = layout.nodes[i] + node_lead = layout.leads[i] + + if entity_feature === nothing + for _ in 1:node_lead + print(buffer, '\t') + end + layout.parent_refs[i] && print(buffer, '^') + _write_mtg_node_code(buffer, node) + for _ in 1:(layout.max_tabs - node_lead) + print(buffer, '\t') + end + else + _write_mtg_columnar_feature( + buffer, features, context, date_values, entity_feature, i + ) + end + + for j in output_features + print(buffer, '\t') + _write_mtg_columnar_feature( + buffer, features, context, date_values, j, i + ) + end + print(buffer, '\n') + position(buffer) > 16 * 1024 && _flush_write_buffer!(io, buffer) + end + _flush_write_buffer!(io, buffer) + return nothing +end + +function _write_mtg_columnar_rows( + io, + layout::_MTGWriteLayout, + features::_MTGWriteFeatureColumns, + context::_MTGColumnarWriteContext, + date_values::Vector{Union{Nothing,Vector{String}}}, + entity_feature::Union{Nothing,Int}, + output_features::Vector{Int}, + overrides::_MTGWriteFeatureOverrides, +) + buffer = IOBuffer(; sizehint=16 * 1024) + @inbounds for i in eachindex(layout.nodes) + node = layout.nodes[i] + node_lead = layout.leads[i] + + if entity_feature === nothing + for _ in 1:node_lead + print(buffer, '\t') + end + layout.parent_refs[i] && print(buffer, '^') + _write_mtg_node_code(buffer, node) + for _ in 1:(layout.max_tabs - node_lead) + print(buffer, '\t') + end + else + _write_mtg_columnar_feature( + buffer, + features, + context, + date_values, + entity_feature, + i, + overrides, + ) + end + + for j in output_features + print(buffer, '\t') + _write_mtg_columnar_feature( + buffer, + features, + context, + date_values, + j, + i, + overrides, + ) + end + print(buffer, '\n') + position(buffer) > 16 * 1024 && _flush_write_buffer!(io, buffer) + end + _flush_write_buffer!(io, buffer) + return nothing +end + function paste_node_mtg(mtg, features) layout = _mtg_write_layout(mtg) feature_columns = _mtg_write_feature_columns(mtg, features) diff --git a/test/test-columnar.jl b/test/test-columnar.jl index 2f8ac23e..91aa97ab 100644 --- a/test/test-columnar.jl +++ b/test/test-columnar.jl @@ -5,6 +5,64 @@ mtg = read_mtg(file) @test node_attributes(mtg) isa MultiScaleTreeGraph.ColumnarAttrs +@testset "ColumnarAttrs iteration" begin + root = MultiScaleTreeGraph.Node( + MultiScaleTreeGraph.NodeMTG(:/, :Plant, 1, 1), + (root_value=1,), + ) + first_leaf = MultiScaleTreeGraph.Node( + root, + MultiScaleTreeGraph.NodeMTG(:/, :Leaf, 1, 2), + (first_only=11, shared="first"), + ) + second_leaf = MultiScaleTreeGraph.Node( + root, + MultiScaleTreeGraph.NodeMTG(:+, :Leaf, 2, 2), + (second_only=22, shared="second"), + ) + + first_attrs = node_attributes(first_leaf) + second_attrs = node_attributes(second_leaf) + first_pairs = collect(pairs(first_attrs)) + second_pairs = collect(pairs(second_attrs)) + + @test first.(first_pairs) == collect(keys(first_attrs)) + @test last.(first_pairs) == [ + get(first_attrs, key, nothing) for key in keys(first_attrs) + ] + @test first.(second_pairs) == collect(keys(second_attrs)) + @test last.(second_pairs) == [ + get(second_attrs, key, nothing) for key in keys(second_attrs) + ] + @test first_attrs[:first_only] == 11 + @test first_attrs[:second_only] === nothing + @test second_attrs[:first_only] === nothing + @test second_attrs[:second_only] == 22 + + second_attrs[:shared] = 2 + @test Dict(pairs(first_attrs))[:shared] == "first" + @test Dict(pairs(second_attrs))[:shared] == 2 + + add_column!(root, :Leaf, :temperature, Float64, default=20.0) + @test last(collect(keys(first_attrs))) == :temperature + @test last(collect(pairs(first_attrs))) == (:temperature => 20.0) + + rename_column!(root, :Leaf, :temperature, :renamed_temperature) + @test :temperature ∉ keys(first_attrs) + @test last(collect(keys(first_attrs))) == :renamed_temperature + @test last(collect(pairs(first_attrs))) == (:renamed_temperature => 20.0) + + drop_column!(root, :Leaf, :renamed_temperature) + @test :renamed_temperature ∉ keys(first_attrs) + @test first.(collect(pairs(first_attrs))) == collect(keys(first_attrs)) + + unbound = MultiScaleTreeGraph.ColumnarAttrs( + Dict{Symbol,Any}(:unbound_first => 1, :unbound_second => "two"), + ) + @test collect(pairs(unbound)) == collect(pairs(unbound.staged)) + @test first.(collect(pairs(unbound))) == collect(keys(unbound)) +end + leaf = traverse(mtg, node -> node, symbol=:Leaf, type=typeof(mtg))[1] leaf_width = attribute(leaf, :Width, default=nothing) diff --git a/test/test-read_mtg.jl b/test/test-read_mtg.jl index 2640cf68..a4261ac7 100644 --- a/test/test-read_mtg.jl +++ b/test/test-read_mtg.jl @@ -83,3 +83,169 @@ end @test all(name -> !isdefined(MultiScaleTreeGraph, name), parser_state_names) end + +function _legacy_parse_attrs_for_test( + node_data, features, feature_names, attr_column_start +) + length(node_data) < attr_column_start && + return MultiScaleTreeGraph.ColumnarAttrs() + node_data_attr = node_data[attr_column_start:end] + node_attr = Dict{Symbol,Any}() + sizehint!(node_attr, length(node_data_attr)) + for i in eachindex(node_data_attr) + field = node_data_attr[i] + (field == "" || field == "NA") && continue + feature_name = feature_names[i] + typ = features.TYPE[i] + if typ == "INT" + node_attr[feature_name] = parse(Int, field) + elseif typ == "BOOLEAN" + node_attr[feature_name] = parse(Bool, field) + elseif typ == "DD/MM/YY" + node_attr[feature_name] = Date(field, dateformat"d/m/y") + elseif typ == "REAL" || + (typ == "ALPHA" && feature_name in (:Width, :Length)) + node_attr[feature_name] = parse(Float64, field) + else + node_attr[feature_name] = field + end + end + return MultiScaleTreeGraph.ColumnarAttrs(node_attr) +end + +@testset "section regex fast paths preserve public matching" begin + @test MultiScaleTreeGraph.issection("prefix CODE : suffix") + @test MultiScaleTreeGraph.issection("NOTCODE:") + @test MultiScaleTreeGraph.issection("prefix MTG : suffix", "MTG") + @test MultiScaleTreeGraph.issection("prefix MTG : suffix", SubString("MTG", 1, 3)) + @test MultiScaleTreeGraph.issection("prefix MTG:", :MTG) + @test MultiScaleTreeGraph.issection("prefix MTG:", "M.G") + @test !MultiScaleTreeGraph.issection("prefix code:") + @test_throws ErrorException MultiScaleTreeGraph.issection("prefix MTG:", "(") +end + +@testset "attribute parser preserves values, types, and Dict copy order" begin + features = MultiScaleTreeGraph.ColumnTable( + Symbol[:NAME, :TYPE], + AbstractVector[ + [:count, :alive, :when, :measure, :label, :Width, :skipped], + ["INT", "BOOLEAN", "DD/MM/YY", "REAL", "STRING", "ALPHA", "STRING"], + ], + ) + feature_names = Symbol.(features.NAME) + node_data = split( + "/Plant1\t7\ttrue\t24/08/2026\t1.5\tlabel-value\t2.25\tNA", + "\t", + ) + expected = _legacy_parse_attrs_for_test( + node_data, features, feature_names, 2 + ) + actual = MultiScaleTreeGraph.parse_MTG_node_attr( + node_data, features, feature_names, 2, [17] + ) + @test collect(pairs(actual)) == collect(pairs(expected)) + @test actual[:count] === 7 + @test actual[:alive] === true + @test actual[:when] == Date(2026, 8, 24) + @test actual[:measure] === 1.5 + @test actual[:Width] === 2.25 + @test typeof(actual[:label]) == typeof(expected[:label]) + @test !haskey(actual, :skipped) + + nwide = 40 + wide_names = [Symbol("feature_$i") for i in 1:nwide] + wide_types = [ + (i % 5 == 1 ? "INT" : + i % 5 == 2 ? "REAL" : + i % 5 == 3 ? "BOOLEAN" : + i % 5 == 4 ? "DD/MM/YY" : "STRING") for i in 1:nwide + ] + wide_values = [ + (typ == "INT" ? string(i) : + typ == "REAL" ? string(i / 10) : + typ == "BOOLEAN" ? string(isodd(i)) : + typ == "DD/MM/YY" ? "24/08/2026" : "value-$i") for + (i, typ) in enumerate(wide_types) + ] + wide_features = MultiScaleTreeGraph.ColumnTable( + Symbol[:NAME, :TYPE], AbstractVector[wide_names, wide_types] + ) + wide_data = vcat("/Plant1", wide_values) + wide_expected = _legacy_parse_attrs_for_test( + wide_data, wide_features, wide_names, 2 + ) + wide_actual = MultiScaleTreeGraph.parse_MTG_node_attr( + wide_data, wide_features, wide_names, 2, [18] + ) + @test collect(pairs(wide_actual)) == collect(pairs(wide_expected)) +end + +@testset "attribute parser diagnostics and row atomicity" begin + features = MultiScaleTreeGraph.ColumnTable( + Symbol[:NAME, :TYPE], + AbstractVector[[:count, :alive], ["INT", "BOOLEAN"]], + ) + feature_names = Symbol.(features.NAME) + fields = split("\t/Plant1\tbad\ttrue", "\t") + + conversion_error = try + MultiScaleTreeGraph._parse_MTG_node_attr_fields( + fields, features, feature_names, 3, 2, [17] + ) + nothing + catch error_ + error_ + end + @test conversion_error isa ErrorException + @test conversion_error.msg == + "Found issue in the MTG when converting column count with value bad into Integer." * + " Please check line [17] of the MTG:\n/Plant1\tbad\ttrue" + + forced = MultiScaleTreeGraph._parse_MTG_node_attr_fields( + fields, features, feature_names, 3, 2, [17]; force=true + ) + @test !haskey(forced, :count) + @test forced[:alive] === true + + too_many = try + MultiScaleTreeGraph.parse_MTG_node_attr( + ["/Plant1", "1", ""], + MultiScaleTreeGraph.ColumnTable( + Symbol[:NAME, :TYPE], AbstractVector[[:count], ["INT"]] + ), + [:count], + 2, + [18], + ) + nothing + catch error_ + error_ + end + @test too_many isa ErrorException + @test endswith(too_many.msg, "/Plant1\t1\t") + + classes = MultiScaleTreeGraph.ColumnTable( + Symbol[:SYMBOL, :SCALE], AbstractVector[["Plant"], [1]] + ) + tree_dict = Dict{Int,Node}() + line = [17] + l = ["\t/Plant1\tbad\ttrue"] + last_node_column = zeros(Integer, 2) + last_node_column[1] = 1 + next_node_id = [1] + @test_throws ErrorException MultiScaleTreeGraph.parse_line_to_node!( + tree_dict, + l, + line, + 3, + last_node_column, + next_node_id, + MutableNodeMTG, + features, + feature_names, + classes, + ) + @test isempty(tree_dict) + @test next_node_id == [1] + @test last_node_column == [1, 0] +end diff --git a/test/test-summary.jl b/test/test-summary.jl index 445af2d3..060530e6 100644 --- a/test/test-summary.jl +++ b/test/test-summary.jl @@ -1,5 +1,40 @@ mtg = read_mtg("files/simple_plant.mtg"); +function _legacy_get_features_for_test(mtg) + names_ = Symbol[] + types_ = String[] + seen = Set{Tuple{Symbol,String}}() + traverse!(mtg) do node + for (name, value) in pairs(node_attributes(node)) + T = typeof(value) + if T <: AbstractVector || T <: Nothing || + name in (:description, :symbols, :scales) + continue + end + typ = if T <: AbstractFloat + "REAL" + elseif T <: Bool + "BOOLEAN" + elseif T <: Integer + "INT" + elseif T <: Date + "DD/MM/YY" + else + "STRING" + end + feature = (Symbol(name), typ) + if !(feature in seen) + push!(seen, feature) + push!(names_, feature[1]) + push!(types_, feature[2]) + end + end + end + return MultiScaleTreeGraph.ColumnTable( + Symbol[:NAME, :TYPE], AbstractVector[names_, types_] + ) +end + @testset "getting scales" begin @test scales(mtg) == [0, 1, 2, 3] @test symbols(mtg) == components(mtg) == [:Scene, :Individual, :Axis, :Internode, :Leaf] @@ -33,6 +68,155 @@ end @test features.TYPE == ["REAL", "REAL", "REAL", "DD/MM/YY", "BOOLEAN"] end +@testset "columnar feature discovery preserves legacy order and fallback" begin + root = Node( + 10, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}( + :root_real => 1.5, + :root_bool => true, + :root_missing => missing, + :ignored_nothing => nothing, + :ignored_vector => [1, 2], + :description => "metadata", + ), + ) + first_leaf = Node( + 41, + root, + MutableNodeMTG(:/, :Leaf, 1, 2), + Dict{Symbol,Any}(:mixed_type => "text", :first_only => 1), + ) + second_leaf = Node( + 105, + root, + MutableNodeMTG(:+, :Leaf, 2, 2), + Dict{Symbol,Any}( + :mixed_type => Date(2026, 8, 24), + :second_only => DateTime(2026, 8, 24, 12), + ), + ) + + @test MultiScaleTreeGraph._get_features_columnar(root) == + _legacy_get_features_for_test(root) + @test get_features(root) == _legacy_get_features_for_test(root) + @test get_features(first_leaf) == _legacy_get_features_for_test(first_leaf) + features = get_features(root) + @test (:root_missing, "STRING") in collect(zip(features.NAME, features.TYPE)) + @test (:root_bool, "BOOLEAN") in collect(zip(features.NAME, features.TYPE)) + @test (:second_only, "STRING") in collect(zip(features.NAME, features.TYPE)) + @test !(:ignored_nothing in features.NAME) + @test !(:ignored_vector in features.NAME) + @test !(:description in features.NAME) + + # Changing a botanical symbol does not migrate its physical attribute bucket. + symbol!(first_leaf, :RenamedLeaf) + @test MultiScaleTreeGraph._get_features_columnar(root) == + _legacy_get_features_for_test(root) + + # The public traversal cache is authoritative, including its node order. + no_filter_cache = MultiScaleTreeGraph.cache_name( + nothing, nothing, nothing, true, nothing + ) + MultiScaleTreeGraph.node_traversal_cache(root)[no_filter_cache] = + typeof(root)[second_leaf, root, first_leaf] + @test MultiScaleTreeGraph._get_features_columnar(root) == + _legacy_get_features_for_test(root) + @test get_features(root) == _legacy_get_features_for_test(root) + + # Removing a row swaps the bucket's last physical row into its place. Feature + # order must nevertheless remain traversal order, not storage-row order. + swap_root = Node( + 1, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}(), + ) + removed = Node( + 2, + swap_root, + MutableNodeMTG(:+, :Leaf, 1, 2), + Dict{Symbol,Any}(:mixed => 1), + ) + string_leaf = Node( + 3, + swap_root, + MutableNodeMTG(:+, :Leaf, 2, 2), + Dict{Symbol,Any}(:mixed => "second"), + ) + date_leaf = Node( + 4, + swap_root, + MutableNodeMTG(:+, :Leaf, 3, 2), + Dict{Symbol,Any}(:mixed => Date(2026, 8, 24)), + ) + delete_node!(removed) + @test children(swap_root) == [string_leaf, date_leaf] + @test get_features(swap_root) == _legacy_get_features_for_test(swap_root) + swap_features = get_features(swap_root) + mixed_rows = [ + (name, typ) for (name, typ) in zip(swap_features.NAME, swap_features.TYPE) + if name == :mixed + ] + @test mixed_rows == [(:mixed, "STRING"), (:mixed, "DD/MM/YY")] + + # Directly malformed mixed stores retain the old per-node traversal behavior. + mixed_root = Node( + 1, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}(:root_value => 1), + ) + external = Node( + 20, + MutableNodeMTG(:/, :External, 1, 1), + Dict{Symbol,Any}(:external_value => 2), + ) + push!(children(mixed_root), external) + @test MultiScaleTreeGraph._get_features_columnar(mixed_root) === nothing + @test get_features(mixed_root) == _legacy_get_features_for_test(mixed_root) + + # Raw unbound attributes also use the original staged-dictionary path. + raw_attrs = MultiScaleTreeGraph.ColumnarAttrs( + Dict{Symbol,Any}(:staged_value => 3) + ) + RawNode = typeof(mixed_root) + raw = RawNode( + 30, + nothing, + RawNode[], + MutableNodeMTG(:/, :Raw, 1, 1), + raw_attrs, + nothing, + ) + @test MultiScaleTreeGraph._get_features_columnar(raw) === nothing + @test get_features(raw) == _legacy_get_features_for_test(raw) + + # A missing physical cell must reject the typed fast path before it is read. + corrupt_root = Node( + 1, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}(:corrupt_value => "root"), + ) + corrupt_child = Node( + 2, + corrupt_root, + MutableNodeMTG(:<, :Plant, 2, 1), + Dict{Symbol,Any}(:corrupt_value => "child"), + ) + corrupt_attrs = node_attributes(corrupt_child) + corrupt_store = corrupt_attrs.ref.store + corrupt_bid = corrupt_store.node_bucket[node_id(corrupt_child)] + corrupt_row = corrupt_store.node_row[node_id(corrupt_child)] + corrupt_bucket = corrupt_store.buckets[corrupt_bid] + corrupt_column = corrupt_bucket.columns[ + corrupt_bucket.col_index[:corrupt_value] + ] + resize!(corrupt_column.data, corrupt_row - 1) + resize!(corrupt_column.data, corrupt_row) + @test !isassigned(corrupt_column.data, corrupt_row) + @test MultiScaleTreeGraph._get_features_columnar(corrupt_root) === nothing + @test_throws UndefRefError get_features(corrupt_root) +end + @testset "get attributes/names" begin @test sort!(get_attributes(mtg)) == sort!(names(mtg)) == [:Length, :Width, :XEuler, :dateDeath, :description, :isAlive, :scales, :symbols] end diff --git a/test/test-write_mtg.jl b/test/test-write_mtg.jl index 53ea7dd0..9aa44105 100644 --- a/test/test-write_mtg.jl +++ b/test/test-write_mtg.jl @@ -1,3 +1,11 @@ +struct _ShiftedWriterVector{T} <: AbstractVector{T} + values::Vector{T} +end + +Base.size(vector::_ShiftedWriterVector) = size(vector.values) +Base.axes(vector::_ShiftedWriterVector) = (0:(length(vector.values) - 1),) +Base.getindex(vector::_ShiftedWriterVector, index::Int) = vector.values[index + 1] + file = joinpath(dirname(dirname(pathof(MultiScaleTreeGraph))), "test", "files", "simple_plant.mtg") mtg = read_mtg(file) @@ -217,6 +225,11 @@ end mtg, features = _many_feature_writer_fixture(nfeatures) @test node_attributes(mtg) isa MultiScaleTreeGraph.ColumnarAttrs @test list_nodes(mtg) == [10, 41, 105, 1_000, 10_000] + layout = MultiScaleTreeGraph._mtg_write_layout(mtg) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns(mtg, features) + @test MultiScaleTreeGraph._mtg_columnar_write_context( + layout, feature_columns + ) isa MultiScaleTreeGraph._MTGColumnarWriteContext expected_section = _legacy_mtg_section_for_test(mtg, features) actual = mktemp() do path, io @@ -283,6 +296,218 @@ end return read(path, String) end @test _written_mtg_section_for_test(actual) == expected_section + + projected = mktemp() do path, io + close(io) + write_mtg( + path, + root, + get_classes(root), + nothing, + features; + feature_overrides=Dict(:long_text => fill(long_text, 2)), + ) + return read(path, String) + end + @test _written_mtg_section_for_test(projected) == expected_section +end + +@testset "Feature overrides are aligned, typed, and source preserving" begin + root = Node( + 10, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}( + :reference => 10, + :date => Date(2026, 8, 24), + :other => "root", + :mtg_print => "original root", + ), + ) + Node( + 1_000, + root, + MutableNodeMTG(:<, :Leaf, -9999, 2), + Dict{Symbol,Any}( + :reference => 1_000, + :date => Date(2025, 1, 2), + :other => "child", + :mtg_print => "original child", + ), + ) + features = DataFrame( + NAME=[:reference, :date, :other, :mtg_print], + TYPE=["INT", "DD/MM/YY", "STRING", "STRING"], + ) + overrides = Dict{Symbol,AbstractVector}( + :reference => Union{Nothing,Int}[2, 1], + :date => Union{Nothing,Date}[Date(2024, 12, 31), nothing], + :mtg_print => ["projected root", "projected child"], + ) + source_attributes = [Dict(pairs(node_attributes(node))) for node in traverse(root, identity)] + + expected = deepcopy(root) + expected_nodes = traverse(expected, identity) + for (i, node) in pairs(expected_nodes) + node[:reference] = overrides[:reference][i] + node[:date] = overrides[:date][i] + node[:mtg_print] = overrides[:mtg_print][i] + end + expected_section = _legacy_mtg_section_for_test(expected, features) + + actual = mktemp() do path, io + close(io) + write_mtg( + path, + root, + get_classes(root), + nothing, + features; + feature_overrides=overrides, + ) + return read(path, String) + end + @test _written_mtg_section_for_test(actual) == expected_section + @test occursin("31/12/2024", actual) + @test !occursin("24/8/2026", actual) + @test [Dict(pairs(node_attributes(node))) for node in traverse(root, identity)] == + source_attributes + + high_level = mktemp() do path, io + close(io) + write_mtg( + path, + root; + classes=get_classes(root), + description=nothing, + features=features, + feature_overrides=overrides, + ) + return read(path, String) + end + @test high_level == actual + + default = mktemp() do path, io + close(io) + write_mtg(path, root, get_classes(root), nothing, features) + return read(path, String) + end + empty_override = mktemp() do path, io + close(io) + write_mtg( + path, + root, + get_classes(root), + nothing, + features; + feature_overrides=Dict{Symbol,Vector{Int}}(), + ) + return read(path, String) + end + @test empty_override == default + + invalid_overrides = ( + (Dict(:undeclared => [1, 2]), "undeclared MTG feature :undeclared"), + (Dict(:reference => [1]), "expected 2 values aligned with MTG preorder"), + (Dict("reference" => [1, 2]), "keys must be Symbols"), + (Dict(:reference => 1), "must be an AbstractVector"), + ( + Dict(:reference => _ShiftedWriterVector([1, 2])), + "must use one-based axes", + ), + ) + for (invalid, expected_message) in invalid_overrides + mktemp() do path, io + close(io) + error = try + write_mtg( + path, + root, + get_classes(root), + nothing, + features; + feature_overrides=invalid, + ) + nothing + catch caught + caught + end + @test error isa ArgumentError + @test occursin(expected_message, sprint(showerror, error)) + @test endswith(read(path, String), "MTG:\n") + end + end +end + +@testset "Feature overrides preserve the fallback lookup boundary" begin + encoding = MutableNodeMTG(:/, :Plant, 1, 1) + attributes = MultiScaleTreeGraph.ColumnarAttrs(Dict(:reference => 10, :other => "root")) + NodeType = Node{typeof(encoding),typeof(attributes)} + root = NodeType( + 10, + nothing, + NodeType[], + encoding, + attributes, + nothing, + ) + features = DataFrame(NAME=[:reference, :other], TYPE=["INT", "STRING"]) + layout = MultiScaleTreeGraph._mtg_write_layout(root) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns(root, features) + @test MultiScaleTreeGraph._mtg_columnar_write_context( + layout, + feature_columns, + ) === nothing + + expected = deepcopy(root) + expected[:reference] = 1 + expected_section = _legacy_mtg_section_for_test(expected, features) + actual = mktemp() do path, io + close(io) + write_mtg( + path, + root, + get_classes(root), + nothing, + features; + feature_overrides=Dict(:reference => [1]), + ) + return read(path, String) + end + @test _written_mtg_section_for_test(actual) == expected_section + @test root[:reference] == 10 +end + +@testset "Duplicate feature last STRING type overrides earlier date type" begin + root = Node( + 10, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}(:date => Date(2026, 8, 24)), + ) + Node( + 20, + root, + MutableNodeMTG(:<, :Leaf, 1, 2), + Dict{Symbol,Any}(:date => Date(2025, 1, 2)), + ) + features = DataFrame( + NAME=[:date, :date], TYPE=["DD/MM/YY", "STRING"] + ) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns(root, features) + @test feature_columns.names == ["date"] + @test collect(feature_columns.is_date) == [false] + @test MultiScaleTreeGraph._mtg_columnar_write_context( + MultiScaleTreeGraph._mtg_write_layout(root), feature_columns + ) isa MultiScaleTreeGraph._MTGColumnarWriteContext + + expected_section = _legacy_mtg_section_for_test(root, features) + actual = mktemp() do path, io + close(io) + write_mtg(path, root, get_classes(root), nothing, features) + return read(path, String) + end + @test _written_mtg_section_for_test(actual) == expected_section + @test occursin("2026-08-24", actual) + @test !occursin("24/08/2026", actual) end @testset "Streaming writer preserves mtg_print feature collision" begin @@ -316,6 +541,179 @@ end @test isequal(actual_attributes, expected_attributes) end +@testset "Typed column writer preserves scalar and container cells" begin + root = Node( + 10, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}( + :vector_value => [1, 2], + :missing_value => missing, + :nothing_value => nothing, + :date_value => Date(2026, 8, 24), + :plain_date => Date(2026, 8, 24), + :text_value => "root", + ), + ) + branch = Node( + 1_000, + root, + MutableNodeMTG(:+, :Branch, 1, 2), + Dict{Symbol,Any}( + :vector_value => [3, 4, 5], + :missing_value => "present", + :date_value => nothing, + :plain_date => Date(2025, 1, 2), + :text_value => "branch", + ), + ) + Node( + 10_000, + branch, + MutableNodeMTG(:<, :Leaf, 1, 3), + Dict{Symbol,Any}( + :vector_value => String["a", "b"], + :missing_value => missing, + :nothing_value => nothing, + :date_value => Date(2024, 12, 31), + :plain_date => Date(2024, 12, 31), + ), + ) + features = DataFrame( + NAME=[ + :vector_value, + :missing_value, + :nothing_value, + :absent_value, + :absent_date, + :date_value, + :plain_date, + :text_value, + ], + TYPE=[ + "STRING", + "STRING", + "STRING", + "STRING", + "DD/MM/YY", + "DD/MM/YY", + "STRING", + "STRING", + ], + ) + + for subtree in (root, branch) + layout = MultiScaleTreeGraph._mtg_write_layout(subtree) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns( + subtree, features + ) + @test MultiScaleTreeGraph._mtg_columnar_write_context( + layout, feature_columns + ) isa MultiScaleTreeGraph._MTGColumnarWriteContext + + expected_section = _legacy_mtg_section_for_test(subtree, features) + actual = mktemp() do path, io + close(io) + write_mtg(path, subtree, get_classes(subtree), nothing, features) + return read(path, String) + end + @test _written_mtg_section_for_test(actual) == expected_section + @test occursin("missing", actual) + @test occursin("[1, 2]", actual) || subtree === branch + @test occursin("24/8/2026", actual) || subtree === branch + @test occursin("2026-08-24", actual) || subtree === branch + end +end + +@testset "Typed writer falls back on mixed columnar stores" begin + root = Node( + 1, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}(:value => 1), + ) + external = Node( + 20, + MutableNodeMTG(:/, :External, 1, 1), + Dict{Symbol,Any}(:value => 2), + ) + push!(children(root), external) + features = DataFrame(NAME=[:value], TYPE=["INT"]) + layout = MultiScaleTreeGraph._mtg_write_layout(root) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns(root, features) + @test MultiScaleTreeGraph._mtg_columnar_write_context( + layout, feature_columns + ) === nothing + + mktemp() do path, io + close(io) + @test_throws ArgumentError write_mtg( + path, root, get_classes(root), nothing, features + ) + @test endswith(read(path, String), "MTG:\n") + end + mktemp() do path, io + close(io) + @test_throws ArgumentError write_mtg( + path, + root, + get_classes(root), + nothing, + features; + feature_overrides=Dict(:value => [10, 20]), + ) + @test endswith(read(path, String), "MTG:\n") + end +end + +@testset "Typed writer validates every bucket row before streaming" begin + root = Node( + 1, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}(:value => "root"), + ) + child = Node( + 2, + root, + MutableNodeMTG(:<, :Plant, 2, 1), + Dict{Symbol,Any}(:value => "child"), + ) + attrs = node_attributes(child) + store = attrs.ref.store + bid = store.node_bucket[node_id(child)] + row = store.node_row[node_id(child)] + bucket = store.buckets[bid] + column = bucket.columns[bucket.col_index[:value]] + resize!(column.data, row - 1) + resize!(column.data, row) + @test !isassigned(column.data, row) + + features = DataFrame(NAME=[:value], TYPE=["STRING"]) + layout = MultiScaleTreeGraph._mtg_write_layout(root) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns(root, features) + @test MultiScaleTreeGraph._mtg_columnar_write_context( + layout, feature_columns + ) === nothing + + mktemp() do path, io + close(io) + @test_throws UndefRefError write_mtg( + path, root, get_classes(root), nothing, features + ) + @test endswith(read(path, String), "MTG:\n") + end + mktemp() do path, io + close(io) + @test_throws UndefRefError write_mtg( + path, + root, + get_classes(root), + nothing, + features; + feature_overrides=Dict(:value => ["projected root", "projected child"]), + ) + @test endswith(read(path, String), "MTG:\n") + end +end + @testset "Table rows stream without changing DelimitedFiles quoting" begin table = DataFrame( first=["plain", "with\ttab", "with\"quote", "with\nline", repeat("long", 6_000)], @@ -337,9 +735,40 @@ end @testset "Streaming writer preserves date conversion errors" begin mtg = Node(10, MutableNodeMTG(:/, :Plant, 1, 1), Dict(:date => missing)) features = DataFrame(NAME=[:date], TYPE=["DD/MM/YY"]) + layout = MultiScaleTreeGraph._mtg_write_layout(mtg) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns(mtg, features) + @test MultiScaleTreeGraph._mtg_columnar_write_context( + layout, feature_columns + ) isa MultiScaleTreeGraph._MTGColumnarWriteContext mktemp() do path, io close(io) @test_throws MethodError write_mtg(path, mtg, get_classes(mtg), nothing, features) @test endswith(read(path, String), "MTG:\n") end end + +@testset "Duplicate date mtg_print fails before ENTITY-CODE" begin + mtg = Node( + 10, + MutableNodeMTG(:/, :Plant, 1, 1), + Dict{Symbol,Any}(:mtg_print => missing, :other => 1), + ) + features = DataFrame( + NAME=[:mtg_print, :other, :mtg_print], + TYPE=["STRING", "INT", "DD/MM/YY"], + ) + feature_columns = MultiScaleTreeGraph._mtg_write_feature_columns(mtg, features) + @test feature_columns.names == ["mtg_print", "other"] + @test collect(feature_columns.is_date) == [true, false] + @test MultiScaleTreeGraph._mtg_columnar_write_context( + MultiScaleTreeGraph._mtg_write_layout(mtg), feature_columns + ) isa MultiScaleTreeGraph._MTGColumnarWriteContext + + mktemp() do path, io + close(io) + @test_throws MethodError write_mtg( + path, mtg, get_classes(mtg), nothing, features + ) + @test endswith(read(path, String), "MTG:\n") + end +end From fd4edb5287649b91684b63377a89a36a6675925d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Mon, 24 Aug 2026 20:40:46 +0200 Subject: [PATCH 04/13] fix: preserve source IDs during columnar subtree merge --- src/compute_MTG/node_funs.jl | 3 +- test/test-caching.jl | 55 ++++++++++++++++++++++++++++++++++++ 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/compute_MTG/node_funs.jl b/src/compute_MTG/node_funs.jl index 9eba7d41..7d276986 100644 --- a/src/compute_MTG/node_funs.jl +++ b/src/compute_MTG/node_funs.jl @@ -106,10 +106,11 @@ function _merge_columnar_subtree_into_store!(node::Node, target_store::MTGAttrib attrs = node_attributes(node) attrs isa ColumnarAttrs || return false source_store = _store_for_node_attrs(attrs) + source_node_id = attrs.ref.node_id snapshot = Dict{Symbol,Any}(pairs(attrs)) _add_node_with_attrs!(target_store, node_id(node), symbol(node), snapshot) if source_store !== nothing && source_store !== target_store - _remove_node!(source_store, node_id(node)) + _remove_node!(source_store, source_node_id) end attrs.ref.store = target_store attrs.ref.node_id = node_id(node) diff --git a/test/test-caching.jl b/test/test-caching.jl index 6389b442..0c65430b 100644 --- a/test/test-caching.jl +++ b/test/test-caching.jl @@ -409,4 +409,59 @@ end @test MultiScaleTreeGraph._node_store(rechildren_target) === target_store_before @test MultiScaleTreeGraph._node_store(incoming_a) === incoming_a_store_before @test MultiScaleTreeGraph._node_store(incoming_b) === incoming_b_store_before + + relabelled_source = Node( + 20, + MutableNodeMTG(:/, :RelabelledSource, 1, 0), + Dict{Symbol,Any}(:SourceValue => 20), + ) + relabelled_child = addchild!( + relabelled_source, + 1, + MutableNodeMTG(:+, :RelabelledChild, 1, 1), + Dict{Symbol,Any}(:ChildValue => 1), + ) + relabelled_descendant = addchild!( + relabelled_child, + 2, + MutableNodeMTG(:+, :RelabelledDescendant, 1, 2), + Dict{Symbol,Any}(:DescendantValue => 2), + ) + relabelled_source_store = MultiScaleTreeGraph._node_store(relabelled_source) + setfield!(relabelled_source, :id, 1) + setfield!(relabelled_child, :id, 2) + setfield!(relabelled_descendant, :id, 3) + + relabelled_target = Node( + 50, + MutableNodeMTG(:/, :RelabelledTarget, 1, 0), + Dict{Symbol,Any}(), + ) + relabelled_target_parent = addchild!( + relabelled_target, + 51, + MutableNodeMTG(:+, :RelabelledTargetParent, 1, 1), + Dict{Symbol,Any}(), + ) + relabelled_target_store = MultiScaleTreeGraph._node_store(relabelled_target) + + addchild!(relabelled_target_parent, relabelled_source) + + @test parent(relabelled_source) === relabelled_target_parent + @test [node_id(node) for node in traverse(relabelled_source, identity)] == [1, 2, 3] + @test relabelled_source[:SourceValue] == 20 + @test relabelled_child[:ChildValue] == 1 + @test relabelled_descendant[:DescendantValue] == 2 + @test all( + node -> MultiScaleTreeGraph._node_store(node) === relabelled_target_store, + traverse(relabelled_source, identity), + ) + @test [ + MultiScaleTreeGraph.node_attributes(node).ref.node_id + for node in traverse(relabelled_source, identity) + ] == [1, 2, 3] + @test all( + old_id -> relabelled_source_store.node_bucket[old_id] == 0, + (20, 1, 2), + ) end From 3dc6aa66d277a84e218011d0433426f130cafaec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Wed, 26 Aug 2026 09:27:59 +0200 Subject: [PATCH 05/13] fix: make columnar attribute mutation row-local --- docs/src/tutorials/1.manipulate_node.md | 25 +++ .../tutorials/7.performance_considerations.md | 26 ++- src/compute_MTG/append_attributes.jl | 15 +- src/compute_MTG/caching.jl | 19 +- src/compute_MTG/descendants.jl | 31 +++- src/compute_MTG/indexing.jl | 1 + src/compute_MTG/select.jl | 21 ++- src/compute_MTG/summary.jl | 4 + src/conversion/Tables.jl | 32 +++- src/types/Attributes.jl | 162 ++++++++++++++---- src/types/Node.jl | 19 +- src/write_mtg/write_mtg.jl | 3 + test/test-columnar.jl | 90 +++++++++- 13 files changed, 389 insertions(+), 59 deletions(-) diff --git a/docs/src/tutorials/1.manipulate_node.md b/docs/src/tutorials/1.manipulate_node.md index 1239bb67..fa2c09df 100644 --- a/docs/src/tutorials/1.manipulate_node.md +++ b/docs/src/tutorials/1.manipulate_node.md @@ -165,6 +165,19 @@ leaf[:Width] = 1.0 leaf[:Width] ``` +To remove an attribute from one node, use `pop!` (and keep the old value) or `delete!`: + +```@example usepkg +leaf[:temporary] = 3.0 +old_value = pop!(leaf, :temporary) +@assert old_value == 3.0 +@assert !haskey(leaf, :temporary) +``` + +`empty!(node_attributes(leaf))` removes every attribute from that node only. To remove a +column from every node with the same MTG symbol, use the explicit schema operation +`drop_column!(mtg, symbol(leaf), :attribute_name)`. + You can also use the explicit API: ```@example usepkg @@ -173,4 +186,16 @@ attribute!(leaf, :new_var, 1.0) attributes(leaf, format=:dict) ``` +### Attribute names and node fields + +Dot access on a `Node` is reserved for attributes: `node.Width` is equivalent to looking up +the `:Width` attribute. Structural state is accessed with `node_id(node)`, `parent(node)`, +`children(node)`, `node_mtg(node)`, and `node_attributes(node)`. Therefore an attribute named +`:id`, `:parent`, or `:children` does not replace structural state; it only occupies that dot +name. Use the accessor functions to avoid ambiguity. + +`copy(node)` is intentionally unsupported because a shallow copy would alias both topology +and the shared columnar store. Use `deepcopy(get_root(node))` for an independent tree, or +`attributes(node; format=:dict)` for an independent snapshot of one node's attributes. + For more examples on querying values in a subtree (descendants, ancestors, filters), continue to [Traversal, descendants, ancestors and filters](@ref). diff --git a/docs/src/tutorials/7.performance_considerations.md b/docs/src/tutorials/7.performance_considerations.md index 0c0868e6..f6ecabd5 100644 --- a/docs/src/tutorials/7.performance_considerations.md +++ b/docs/src/tutorials/7.performance_considerations.md @@ -31,6 +31,30 @@ The explicit attribute API is: `read_mtg` always uses the typed columnar backend. If you build nodes manually, you can still pass `Dict`/`NamedTuple` values, and they are converted automatically to columnar attributes. +The columnar store keeps one typed payload vector per attribute and symbol, plus a compact +presence mask. Consequently, dictionary-style mutations remain local to the selected node: + +```julia +old_value = pop!(node, :temporary_value) +delete!(node, :another_value) +empty!(node_attributes(node)) +``` + +These operations do not remove values from other nodes that share the same physical column. +An attribute whose value is explicitly `nothing` is still present; use `haskey(node, key)` to +distinguish that case from an absent attribute. + +Schema operations are deliberately separate: + +- `add_column!(mtg, :Leaf, :temperature, Float64; default=20.0)` adds a defaulted + attribute to every current and future `:Leaf` row; +- `drop_column!(mtg, :Leaf, :temperature)` removes the complete column from every + `:Leaf`; and +- `rename_column!(mtg, :Leaf, :temperature, :leaf_temperature)` renames that shared + column. + +Use a schema operation only when the intended scope is all nodes of a symbol. + ### MTG encoding The MTG encoding is the type used to store the MTG information about the node, *i.e.* the `scale`, `index`, `symbol` and `link`. @@ -83,7 +107,7 @@ descendants!(nodes, mtg, self=true) This pattern is especially useful when the same request is executed many times (e.g. across millions of leaves). -`type=` for `descendants`/`ancestors` is deprecated because return eltypes are inferred automatically from typed columns. +`type=` for `descendants`/`ancestors` is deprecated because return eltypes are inferred automatically from typed columns. It is scheduled for removal in MultiScaleTreeGraph 0.17; maintained code should omit it now. ### Hybrid descendants backend (growth-safe) diff --git a/src/compute_MTG/append_attributes.jl b/src/compute_MTG/append_attributes.jl index 1201ebe4..9f54e183 100755 --- a/src/compute_MTG/append_attributes.jl +++ b/src/compute_MTG/append_attributes.jl @@ -48,13 +48,20 @@ function Base.pop!(node::Node{M,T}, key) where {M<:AbstractNodeMTG,T<:NamedTuple end function Base.pop!(node::Node{<:AbstractNodeMTG,<:AbstractDict}, key) - poped_value = pop!(node_attributes(node), key, nothing) - - return poped_value + return pop!(node_attributes(node), key) end function Base.pop!(node::Node{<:AbstractNodeMTG,ColumnarAttrs}, key) - pop!(node_attributes(node), key, nothing) + return pop!(node_attributes(node), key) +end + +function Base.pop!(node::Node{<:AbstractNodeMTG,<:AbstractDict}, key, default) + return pop!(node_attributes(node), key, default) +end + +function Base.delete!(node::Node{<:AbstractNodeMTG,<:AbstractDict}, key) + delete!(node_attributes(node), key) + return node end # Renaming attributes: diff --git a/src/compute_MTG/caching.jl b/src/compute_MTG/caching.jl index 4952681a..ffbdb07e 100644 --- a/src/compute_MTG/caching.jl +++ b/src/compute_MTG/caching.jl @@ -20,10 +20,21 @@ Clean the cached variables in the mtg, usually added from [`descendants!`](@ref) """ function clean_cache!(mtg) cached_vars = find_cached_vars(mtg) - traverse!( - mtg, - node -> [pop!(node, attr) for attr in cached_vars] - ) + store = _columnar_store(get_root(mtg)) + if store === nothing + traverse!(mtg) do node + for attr in cached_vars + pop!(node, attr, nothing) + end + end + else + for bucket in store.buckets + for attr in cached_vars + _drop_column_internal!(bucket, attr) + end + end + end + return nothing end function find_cached_vars(node) diff --git a/src/compute_MTG/descendants.jl b/src/compute_MTG/descendants.jl index ade9523b..c7b720ec 100644 --- a/src/compute_MTG/descendants.jl +++ b/src/compute_MTG/descendants.jl @@ -1,7 +1,8 @@ @inline function _maybe_depwarn_traversal_type_kw(fname::Symbol, type) type === Any && return nothing Base.depwarn( - "Keyword argument `type` in `$fname` is deprecated and will be removed in a future release. " * + "Keyword argument `type` in `$fname` is deprecated and will be removed in " * + "MultiScaleTreeGraph 0.17. " * "Return types are inferred automatically from the columnar attribute store; remove `type` " * "and use `ignore_nothing=true` when you want `nothing` values filtered out.", fname, @@ -46,7 +47,13 @@ end @inbounds for i in left:right nid = idx.dfs_order[i] store.node_bucket[nid] == bid || continue - v = col.data[store.node_row[nid]] + row = store.node_row[nid] + if !_row_has_value(col, row) + ignore_nothing && continue + push!(out, nothing) + continue + end + v = col.data[row] ignore_nothing && v === nothing && continue push!(out, v) end @@ -120,7 +127,13 @@ function _collect_descendant_values_indexed!( end row = store.node_row[nid] - v = store.buckets[bid].columns[col_idx].data[row] + column = store.buckets[bid].columns[col_idx] + if !_row_has_value(column, row) + ignore_nothing && continue + push!(out, nothing) + continue + end + v = column.data[row] ignore_nothing && v === nothing && continue push!(out, v) end @@ -188,9 +201,15 @@ function _collect_descendant_multi_values_indexed!( row_vals[j] = nothing row_has_nothing = true else - v = store.buckets[bid].columns[col_idx].data[row] - row_vals[j] = v - row_has_nothing |= v === nothing + column = store.buckets[bid].columns[col_idx] + if _row_has_value(column, row) + v = column.data[row] + row_vals[j] = v + row_has_nothing |= v === nothing + else + row_vals[j] = nothing + row_has_nothing = true + end end end ignore_nothing && row_has_nothing && continue diff --git a/src/compute_MTG/indexing.jl b/src/compute_MTG/indexing.jl index e32ff119..96b58cef 100644 --- a/src/compute_MTG/indexing.jl +++ b/src/compute_MTG/indexing.jl @@ -126,6 +126,7 @@ end col_idx == 0 && return nothing row = store.node_row[nodeid] col = store.buckets[bid].columns[col_idx] + _row_has_value(col, row) || return nothing return col.data[row] end diff --git a/src/compute_MTG/select.jl b/src/compute_MTG/select.jl index 096bb8cd..d41a3cb4 100644 --- a/src/compute_MTG/select.jl +++ b/src/compute_MTG/select.jl @@ -70,15 +70,26 @@ function select!( ignore_nothing=ignore_nothing ) - # Remove all un-selected attributes from the MTG: - traverse!( - mtg, - node -> begin + # Selection is intentionally schema-wide. On a columnar MTG, say so directly + # instead of relying on a node-local mutation to remove a shared column as a + # side effect. + store = _columnar_store(get_root(mtg)) + if store === nothing + traverse!(mtg) do node for attr in keys(node_attributes(node)) attr in keep_var || pop!(node, attr) end end - ) + else + for bucket in store.buckets + to_drop = Symbol[ + column.name for column in bucket.columns if !(column.name in keep_var) + ] + for attr in to_drop + _drop_column_internal!(bucket, attr) + end + end + end end diff --git a/src/compute_MTG/summary.jl b/src/compute_MTG/summary.jl index 1f7e189f..4716ca7c 100644 --- a/src/compute_MTG/summary.jl +++ b/src/compute_MTG/summary.jl @@ -97,6 +97,8 @@ function _record_column_features!( store.node_row[nodeid] == row || return false get(bucket.node_to_row, nodeid, 0) == row || return false isassigned(column.data, row) || return false + row <= length(column.present) || return false + _row_has_value(column, row) || continue value = column.data[row] value_type = typeof(value) @@ -167,6 +169,8 @@ function _get_features_columnar(mtg) column isa Column || return nothing get(bucket.col_index, column.name, 0) == col_idx || return nothing length(column.data) == nrows || return nothing + length(column.present) == nrows || return nothing + count(identity, column.present) == column.n_present || return nothing column.name in (:description, :symbols, :scales) && continue _record_column_features!( first_positions, diff --git a/src/conversion/Tables.jl b/src/conversion/Tables.jl index b72bcbc6..a1649cc8 100644 --- a/src/conversion/Tables.jl +++ b/src/conversion/Tables.jl @@ -3,17 +3,32 @@ struct MTGAttrColumnView{N,T} <: AbstractVector{T} key::Symbol end +struct ColumnarTableView{T,C<:Column} <: AbstractVector{T} + column::C +end + const _TABLE_META_COLUMNS = (:description, :symbols, :scales) Base.IndexStyle(::Type{<:MTGAttrColumnView}) = IndexLinear() Base.size(col::MTGAttrColumnView) = (length(col.nodes),) Base.length(col::MTGAttrColumnView) = length(col.nodes) +Base.IndexStyle(::Type{<:ColumnarTableView}) = IndexLinear() +Base.size(col::ColumnarTableView) = (length(col.column.data),) +Base.length(col::ColumnarTableView) = length(col.column.data) + @inline function Base.getindex(col::MTGAttrColumnView{N,T}, i::Int) where {N,T} v = attribute(col.nodes[i], col.key, nothing) return v === nothing ? (missing::T) : (v::T) end +@inline function Base.getindex(col::ColumnarTableView{T}, i::Int) where {T} + column = col.column + _row_has_value(column, i) || return missing::T + value = @inbounds column.data[i] + return value === nothing ? (missing::T) : (value::T) +end + @inline _to_table_key(key::Symbol) = key @inline _to_table_key(key) = Symbol(key) @@ -41,6 +56,8 @@ end end has_any = true + column = bucket.columns[col_idx] + column.n_present == length(column.present) || (has_missing = true) col_T = bucket.col_types[col_idx] col_T_no_nothing = _remove_nothing_type(col_T) if col_T_no_nothing === Union{} @@ -57,6 +74,14 @@ end has_missing ? Union{Missing,T} : T end + +@inline function _table_column_type(column::Column) + T = _remove_nothing_type(eltype(column.data)) + has_missing = column.n_present != length(column.present) || T !== eltype(column.data) + T === Union{} && return Missing + return has_missing ? Union{Missing,T} : T +end + function _collect_attr_names_from_store(store::MTGAttributeStore) out = Symbol[] seen = Set{Symbol}() @@ -174,7 +199,8 @@ function symbol_table(mtg::Node, symbol, vars=nothing) if vars_ === nothing for col in bucket.columns push!(names_, col.name) - push!(cols_, col.data) + T = _table_column_type(col) + push!(cols_, ColumnarTableView{T,typeof(col)}(col)) end else for key in vars_ @@ -183,7 +209,9 @@ function symbol_table(mtg::Node, symbol, vars=nothing) if col_idx == 0 push!(cols_, fill(missing, length(bucket.row_to_node))) else - push!(cols_, bucket.columns[col_idx].data) + col = bucket.columns[col_idx] + T = _table_column_type(col) + push!(cols_, ColumnarTableView{T,typeof(col)}(col)) end end end diff --git a/src/types/Attributes.jl b/src/types/Attributes.jl index a1f0fb32..fbbec524 100644 --- a/src/types/Attributes.jl +++ b/src/types/Attributes.jl @@ -21,9 +21,15 @@ SubtreeIndexCache() = mutable struct Column{T} name::Symbol data::Vector{T} + present::BitVector + n_present::Int default::T + default_present::Bool end +Column{T}(name::Symbol, data::Vector{T}, default::T) where {T} = + Column{T}(name, data, trues(length(data)), length(data), default, true) + mutable struct SymbolBucket symbol::Symbol row_to_node::Vector{Int} @@ -247,7 +253,14 @@ function _widen_column!(bucket::SymbolBucket, col_idx::Int, ::Type{NewT}) where new_data[i] = old_data[i] end new_default = convert(NewT, old_col.default) - bucket.columns[col_idx] = Column{NewT}(old_col.name, new_data, new_default) + bucket.columns[col_idx] = Column{NewT}( + old_col.name, + new_data, + copy(old_col.present), + old_col.n_present, + new_default, + old_col.default_present, + ) bucket.col_types[col_idx] = NewT return bucket.columns[col_idx] end @@ -262,10 +275,18 @@ function _ensure_column_type!(bucket::SymbolBucket, col_idx::Int, value) return _widen_column!(bucket, col_idx, NewT) end -function _add_column_internal!(bucket::SymbolBucket, key::Symbol, ::Type{T}, default::T) where {T} +function _add_column_internal!( + bucket::SymbolBucket, + key::Symbol, + ::Type{T}, + default::T, + default_present::Bool=true, +) where {T} haskey(bucket.col_index, key) && error("Column $(key) already exists for symbol $(bucket.symbol).") nrows = length(bucket.row_to_node) - col = Column{T}(key, fill(default, nrows), default) + present = default_present ? trues(nrows) : falses(nrows) + n_present = default_present ? nrows : 0 + col = Column{T}(key, fill(default, nrows), present, n_present, default, default_present) push!(bucket.columns, col) push!(bucket.col_types, T) bucket.col_index[key] = length(bucket.columns) @@ -274,14 +295,35 @@ end function _add_nullable_column_internal!(bucket::SymbolBucket, key::Symbol, ::Type{T}) where {T} TT = Union{Nothing,T} - _add_column_internal!(bucket, key, TT, nothing) + _add_column_internal!(bucket, key, TT, nothing, false) +end + +@inline _row_has_value(column::Column, row::Int) = @inbounds column.present[row] + +@inline function _mark_row_present!(column::Column, row::Int) + @inbounds if !column.present[row] + column.present[row] = true + column.n_present += 1 + end + return nothing +end + +@inline function _mark_row_absent!(column::Column, row::Int) + @inbounds begin + column.data[row] = column.default + if column.present[row] + column.present[row] = false + column.n_present -= 1 + end + end + return nothing end function _set_value!(bucket::SymbolBucket, row::Int, key::Symbol, value) col_idx = get(bucket.col_index, key, 0) if col_idx == 0 if value === nothing - _add_nullable_column_internal!(bucket, key, Any) + _add_nullable_column_internal!(bucket, key, Nothing) else _add_nullable_column_internal!(bucket, key, typeof(value)) end @@ -290,13 +332,16 @@ function _set_value!(bucket::SymbolBucket, row::Int, key::Symbol, value) col = _ensure_column_type!(bucket, col_idx, value) col.data[row] = value + _mark_row_present!(col, row) return value end function _get_value(bucket::SymbolBucket, row::Int, key::Symbol, default=nothing) col_idx = get(bucket.col_index, key, 0) col_idx == 0 && return default - _column(bucket, col_idx).data[row] + column = _column(bucket, col_idx) + _row_has_value(column, row) || return default + column.data[row] end function _bucket_row(ref::NodeAttrRef) @@ -332,6 +377,8 @@ function _add_node_with_attrs!(store::MTGAttributeStore, node_id::Int, symbol::S @inbounds for i in eachindex(bucket.columns) col = bucket.columns[i] push!(col.data, col.default) + push!(col.present, col.default_present) + col.default_present && (col.n_present += 1) end store.node_bucket[node_id] = bid @@ -365,10 +412,14 @@ function _remove_node!(store::MTGAttributeStore, node_id::Int) @inbounds for i in eachindex(bucket.columns) col = bucket.columns[i] + removed_present = col.present[row] if row != last_row col.data[row] = col.data[last_row] + col.present[row] = col.present[last_row] end pop!(col.data) + pop!(col.present) + removed_present && (col.n_present -= 1) end if moved_node != 0 @@ -476,6 +527,8 @@ function infer_columnar_attr_type(node, key::Symbol, symbol_filter, ignore_nothi continue end has_any = true + column = _column(bucket, col_idx) + column.n_present == length(column.present) || (missing_in_some = true) col_T = bucket.col_types[col_idx] T = T === Union{} ? col_T : typejoin(T, col_T) end @@ -551,19 +604,29 @@ function Base.length(attrs::ColumnarAttrs) if !_isbound(attrs) return length(attrs.staged) end - store, bid, _ = _bound_store_bid_row(attrs.ref) - return length(store.buckets[bid].columns) + store, bid, row = _bound_store_bid_row(attrs.ref) + bucket = store.buckets[bid] + count = 0 + @inbounds for column in bucket.columns + count += column.present[row] + end + return count end function Base.keys(attrs::ColumnarAttrs) if !_isbound(attrs) return collect(keys(attrs.staged)) end - store, bid, _ = _bound_store_bid_row(attrs.ref) + store, bid, row = _bound_store_bid_row(attrs.ref) bucket = store.buckets[bid] - out = Vector{Symbol}(undef, length(bucket.columns)) + out = Vector{Symbol}(undef, length(attrs)) + j = 1 @inbounds for i in eachindex(bucket.columns) - out[i] = _column(bucket, i).name + column = _column(bucket, i) + if column.present[row] + out[j] = column.name + j += 1 + end end out end @@ -572,8 +635,11 @@ function Base.haskey(attrs::ColumnarAttrs, key::Symbol) if !_isbound(attrs) return haskey(attrs.staged, key) end - store, bid, _ = _bound_store_bid_row(attrs.ref) - haskey(store.buckets[bid].col_index, key) + store, bid, row = _bound_store_bid_row(attrs.ref) + bucket = store.buckets[bid] + col_idx = get(bucket.col_index, key, 0) + col_idx == 0 && return false + return _row_has_value(_column(bucket, col_idx), row) end Base.haskey(attrs::ColumnarAttrs, key) = haskey(attrs, _normalize_attr_key(key)) @@ -585,7 +651,9 @@ function Base.getindex(attrs::ColumnarAttrs, key::Symbol) bucket = store.buckets[bid] col_idx = get(bucket.col_index, key, 0) col_idx == 0 && throw(KeyError(key)) - return _column(bucket, col_idx).data[row] + column = _column(bucket, col_idx) + _row_has_value(column, row) || throw(KeyError(key)) + return column.data[row] end Base.getindex(attrs::ColumnarAttrs, key) = getindex(attrs, _normalize_attr_key(key)) @@ -608,9 +676,11 @@ end bucket = store.buckets[bid] col_idx = get(bucket.col_index, key, 0) col_idx == 0 && return default + column = _column(bucket, col_idx) + _row_has_value(column, row) || return default if _column_matches_exact_type(bucket, col_idx, T) - col = bucket.columns[col_idx]::Column{T} + col = column::Column{T} return @inbounds col.data[row] elseif _column_matches_nullable_type(bucket, col_idx, T) col = bucket.columns[col_idx]::Column{Union{Nothing,T}} @@ -630,9 +700,11 @@ function Base.get(attrs::ColumnarAttrs, key::Symbol, default::T) where {T} bucket = store.buckets[bid] col_idx = get(bucket.col_index, key, 0) col_idx == 0 && return default + column = _column(bucket, col_idx) + _row_has_value(column, row) || return default if _column_matches_exact_type(bucket, col_idx, T) - col = bucket.columns[col_idx]::Column{T} + col = column::Column{T} return @inbounds col.data[row] elseif _column_matches_nullable_type(bucket, col_idx, T) col = bucket.columns[col_idx]::Column{Union{Nothing,T}} @@ -653,9 +725,11 @@ Base.get(attrs::ColumnarAttrs, key, default) = get(attrs, _normalize_attr_key(ke if _column_matches_exact_type(bucket, col_idx, T) col = bucket.columns[col_idx]::Column{T} @inbounds col.data[row] = value + _mark_row_present!(col, row) elseif _column_matches_nullable_type(bucket, col_idx, T) col = bucket.columns[col_idx]::Column{Union{Nothing,T}} @inbounds col.data[row] = value + _mark_row_present!(col, row) else _set_value!(bucket, row, key, value) end @@ -682,24 +756,48 @@ function Base.iterate(attrs::ColumnarAttrs, state=nothing) store, bid, row = _bound_store_bid_row(attrs.ref) bucket = store.buckets[bid] i = state === nothing ? 1 : state - i > length(bucket.columns) && return nothing - column = _column(bucket, i) - return (column.name => column.data[row], i + 1) + @inbounds while i <= length(bucket.columns) + column = _column(bucket, i) + if column.present[row] + return (column.name => column.data[row], i + 1) + end + i += 1 + end + return nothing +end + +@inline function _pop_bound!(attrs::ColumnarAttrs, key::Symbol, default, throw_missing::Bool) + store, bid, row = _bound_store_bid_row(attrs.ref) + bucket = store.buckets[bid] + col_idx = get(bucket.col_index, key, 0) + if col_idx == 0 + throw_missing && throw(KeyError(key)) + return default + end + column = _column(bucket, col_idx) + if !_row_has_value(column, row) + throw_missing && throw(KeyError(key)) + return default + end + old = column.data[row] + _mark_row_absent!(column, row) + return old end -function Base.pop!(attrs::ColumnarAttrs, key, default=nothing) +function Base.pop!(attrs::ColumnarAttrs, key) key_ = _normalize_attr_key(key) if !_isbound(attrs) - return pop!(attrs.staged, key_, default) + return pop!(attrs.staged, key_) end + return _pop_bound!(attrs, key_, nothing, true) +end - store, bid, row = _bound_store_bid_row(attrs.ref) - bucket = store.buckets[bid] - col_idx = get(bucket.col_index, key_, 0) - col_idx == 0 && return default - old = _column(bucket, col_idx).data[row] - _drop_column_internal!(bucket, key_) - return old +function Base.pop!(attrs::ColumnarAttrs, key, default) + key_ = _normalize_attr_key(key) + if !_isbound(attrs) + return pop!(attrs.staged, key_, default) + end + return _pop_bound!(attrs, key_, default, false) end function Base.delete!(attrs::ColumnarAttrs, key) @@ -712,11 +810,11 @@ function Base.empty!(attrs::ColumnarAttrs) empty!(attrs.staged) return attrs end - store, bid, _ = _bound_store_bid_row(attrs.ref) + store, bid, row = _bound_store_bid_row(attrs.ref) bucket = store.buckets[bid] - empty!(bucket.col_index) - empty!(bucket.columns) - empty!(bucket.col_types) + @inbounds for column in bucket.columns + _row_has_value(column, row) && _mark_row_absent!(column, row) + end return attrs end diff --git a/src/types/Node.jl b/src/types/Node.jl index 5a231a54..ace7deed 100644 --- a/src/types/Node.jl +++ b/src/types/Node.jl @@ -94,7 +94,7 @@ function Node( Node{N,A}(id, parent, children, MTG, attributes, traversal_cache) end -# All deprecated methods (the ones with a node name) : +# Deprecated named-node constructors are retained through 0.16.x and removed in 0.17. @deprecate Node(name::String, id::Int, parent::Union{Nothing,Node{N,A}}, children::Nothing, MTG::N, attributes::A, traversal_cache::Dict{String,Vector{Node{N,A}}}) where {N<:AbstractNodeMTG,A} Node(id, parent, children, MTG, attributes, traversal_cache) @deprecate Node(name::String, id::Int, MTG::M, attributes::T) where {M<:AbstractNodeMTG,T<:MutableNamedTuple} Node(id, MTG, attributes) @deprecate Node(name::String, id::Int, MTG::M, attributes::T) where {M<:AbstractNodeMTG,T<:NamedTuple} Node(id, MTG, attributes) @@ -176,8 +176,21 @@ function Node(parent::Node{N,A}, MTG::T) where {N<:AbstractNodeMTG,A,T<:Abstract Node(new_id(get_root(parent)), parent, MTG, A()) end -# Copying a node returns the node: -Base.copy(node::Node) = node +""" + copy(node::Node) + +Shallow copying an MTG node is unsupported because sharing its parent, children, and +columnar attribute store would violate tree identity and mutation invariants. Use +`deepcopy(get_root(node))` for an independent tree, or [`attributes`](@ref) to copy only +one node's attribute values. +""" +function Base.copy(::Node) + throw(ArgumentError( + "copy(::Node) is unsupported because a shallow node copy would alias topology " * + "and columnar storage; use deepcopy(get_root(node)) for an independent tree or " * + "attributes(node; format=:dict) for an attribute snapshot", + )) +end ## AbstractTrees compatibility: diff --git a/src/write_mtg/write_mtg.jl b/src/write_mtg/write_mtg.jl index 0c4b10e4..e9022e4a 100644 --- a/src/write_mtg/write_mtg.jl +++ b/src/write_mtg/write_mtg.jl @@ -257,6 +257,7 @@ end function _mtg_validate_column_rows(column::Column{T}, rows::Vector{Int}) where {T} @inbounds for row in rows row <= length(column.data) || return false + row <= length(column.present) || return false isassigned(column.data, row) || return false end return true @@ -400,6 +401,7 @@ function _mtg_columnar_write_context( end @inline function _mtg_column_value(column::Column{T}, row::Int) where {T} + _row_has_value(column, row) || return nothing return @inbounds column.data[row] end @@ -603,6 +605,7 @@ function _write_mtg_rows( end @inline function _write_mtg_column_value(io, column::Column{T}, row::Int) where {T} + _row_has_value(column, row) || return nothing value = @inbounds column.data[row] value === nothing || print(io, value) return nothing diff --git a/test/test-columnar.jl b/test/test-columnar.jl index 91aa97ab..201fbd16 100644 --- a/test/test-columnar.jl +++ b/test/test-columnar.jl @@ -35,8 +35,12 @@ mtg = read_mtg(file) get(second_attrs, key, nothing) for key in keys(second_attrs) ] @test first_attrs[:first_only] == 11 - @test first_attrs[:second_only] === nothing - @test second_attrs[:first_only] === nothing + @test !haskey(first_attrs, :second_only) + @test !haskey(second_attrs, :first_only) + @test get(first_attrs, :second_only, nothing) === nothing + @test get(second_attrs, :first_only, nothing) === nothing + @test_throws KeyError first_attrs[:second_only] + @test_throws KeyError second_attrs[:first_only] @test second_attrs[:second_only] == 22 second_attrs[:shared] = 2 @@ -63,6 +67,88 @@ mtg = read_mtg(file) @test first.(collect(pairs(unbound))) == collect(keys(unbound)) end +@testset "ColumnarAttrs mutations are row-local" begin + root = Node(NodeMTG(:/, :Plant, 1, 1)) + first_leaf = Node(root, NodeMTG(:/, :Leaf, 1, 2), (x=10, shared="first")) + second_leaf = Node(root, NodeMTG(:+, :Leaf, 2, 2), (x=20, shared="second")) + + store = MultiScaleTreeGraph._node_store(root) + leaf_bucket = store.buckets[store.symbol_to_bucket[:Leaf]] + x_column = leaf_bucket.columns[leaf_bucket.col_index[:x]] + @test eltype(x_column.data) == Union{Nothing,Int} + @test eltype(x_column.data) !== Any + + @test pop!(first_leaf, :x) == 10 + @test !haskey(first_leaf, :x) + @test attribute(first_leaf, :x, :absent) === :absent + @test first_leaf[:x] === nothing + @test_throws KeyError node_attributes(first_leaf)[:x] + @test_throws KeyError pop!(first_leaf, :x) + @test pop!(first_leaf, :x, :absent) === :absent + @test haskey(second_leaf, :x) + @test second_leaf[:x] == 20 + + first_leaf[:nullable] = nothing + @test haskey(first_leaf, :nullable) + @test first_leaf[:nullable] === nothing + @test !haskey(second_leaf, :nullable) + + @test delete!(second_leaf, :shared) === second_leaf + @test !haskey(second_leaf, :shared) + @test first_leaf[:shared] == "first" + + empty!(node_attributes(first_leaf)) + @test isempty(node_attributes(first_leaf)) + @test haskey(second_leaf, :x) + @test second_leaf[:x] == 20 + + drop_column!(root, :Leaf, :x) + @test !haskey(second_leaf, :x) + @test !haskey(leaf_bucket.col_index, :x) +end + +@testset "Sparse columnar attributes survive growth, merge, and MTG I/O" begin + root = Node(NodeMTG(:/, :Plant, 1, 1)) + first_leaf = Node(root, NodeMTG(:/, :Leaf, 1, 2), (local_value=1,)) + second_leaf = Node(root, NodeMTG(:+, :Leaf, 2, 2)) + @test !haskey(second_leaf, :local_value) + + third_leaf = Node(root, NodeMTG(:+, :Leaf, 3, 2)) + @test !haskey(third_leaf, :local_value) + append!(third_leaf, (local_value=3, appended="yes")) + @test third_leaf[:local_value] == 3 + @test !haskey(first_leaf, :appended) + + add_column!(root, :Leaf, :temperature, Float64, default=20.0) + fourth_leaf = Node(root, NodeMTG(:+, :Leaf, 4, 2)) + @test fourth_leaf[:temperature] == 20.0 + @test first_leaf[:temperature] == 20.0 + + detached = Node(10, NodeMTG(:+, :Leaf, 10, 2), (detached_only=10,)) + addchild!(root, detached) + @test detached[:detached_only] == 10 + @test !haskey(first_leaf, :detached_only) + @test first_leaf[:local_value] == 1 + + pop!(first_leaf, :local_value) + roundtrip = mktemp() do path, io + write_mtg(path, root) + read_mtg(path) + end + @test !haskey(get_node(roundtrip, 2), :local_value) + @test get_node(roundtrip, 4)[:local_value] == 3 + roundtrip_leaves = descendants(roundtrip; symbol=:Leaf) + @test last(roundtrip_leaves)[:detached_only] == 10 +end + +@testset "Node copy rejects topology aliasing" begin + root = Node(NodeMTG(:/, :Plant, 1, 1)) + @test_throws ArgumentError copy(root) + copied = deepcopy(root) + @test copied !== root + @test node_id(copied) == node_id(root) +end + leaf = traverse(mtg, node -> node, symbol=:Leaf, type=typeof(mtg))[1] leaf_width = attribute(leaf, :Width, default=nothing) From 7c3bc54528fd15a0b96920c98909f4ec49cb4810 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Wed, 26 Aug 2026 09:49:20 +0200 Subject: [PATCH 06/13] refactor: isolate MTG compatibility shims --- docs/make.jl | 1 + docs/src/compatibility.md | 41 ++++++++++++++++++++ src/MultiScaleTreeGraph.jl | 8 +--- src/compute_MTG/insert_nodes.jl | 11 +++++- src/types/Node.jl | 2 +- test/runtests.jl | 4 ++ test/test-ancestors.jl | 5 --- test/test-compatibility.jl | 67 +++++++++++++++++++++++++++++++++ test/test-descendants.jl | 5 --- 9 files changed, 125 insertions(+), 19 deletions(-) create mode 100644 docs/src/compatibility.md create mode 100644 test/test-compatibility.jl diff --git a/docs/make.jl b/docs/make.jl index ad78eb1d..1a4f292a 100644 --- a/docs/make.jl +++ b/docs/make.jl @@ -32,6 +32,7 @@ makedocs(; "Add and Remove Nodes" => "tutorials/6.add_remove_nodes.md", "Performance Considerations" => "tutorials/7.performance_considerations.md", ], + "Compatibility" => "compatibility.md", "API" => "api.md", ] ) diff --git a/docs/src/compatibility.md b/docs/src/compatibility.md new file mode 100644 index 00000000..385b2e08 --- /dev/null +++ b/docs/src/compatibility.md @@ -0,0 +1,41 @@ +# Compatibility and deprecations + +MultiScaleTreeGraph converts historical MTG files into the current in-memory model, but +maintained package code, examples, and tutorials should use the current Julia API. A +deprecated Julia entry point is a migration aid, not a second permanent API. + +## Scheduled removals + +The following deprecated entry points are retained through the 0.16 release series and are +scheduled for removal in MultiScaleTreeGraph 0.17: + +| Deprecated entry point | Historical role | Category | Current replacement | Retained evidence and warning | Exit | +|:--|:--|:--|:--|:--|:--| +| `Node(name, id, ...)` constructors | Public API from when a separate string name identified each node | Public API deprecation | `Node(id, ...)`; a node's MTG symbol carries its identity | `test/test-compatibility.jl`; Julia deprecation warning | Remove in 0.17 | +| `insert_node!(node, template, maxid)` | Public predecessor of the explicit insertion operations | Public API deprecation | Normally `insert_parent!(node, template)`. To preserve a caller-managed ID counter, use `insert_parent!(node, template, _ -> typeof(node_attributes(node))(), maxid)` | `test/test-compatibility.jl`; Julia deprecation warning | Remove in 0.17 | +| `type=` in `descendants` and `ancestors` | Caller-selected result storage before attribute result types were inferred | Public API deprecation | Omit `type=`; typed columns infer the result type. Use `ignore_nothing=true` to filter absent values | `test/test-compatibility.jl`; Julia deprecation warning | Remove in 0.17 | + +An inventory of MultiScaleTreeGraph and its maintained ecosystem packages found no package, +tutorial, or documentation caller of the named constructors or `insert_node!`. The only +maintained uses of `type=` are the explicit compatibility checks. Ordinary tests use the +current inferred-result API. A deprecated call found in maintained downstream code should be +migrated, not used to justify extending this window. + +!!! compat "Base function exports" + MultiScaleTreeGraph defines methods of Base functions such as `show`, `length`, + `iterate`, `append!`, `names`, and `==`, but no longer re-exports those names. They + remain available normally because Base owns them; callers do not need to qualify or + import them from MultiScaleTreeGraph. + +## Compatibility that remains intentional + +| Boundary | Producer | Category | Normalization | Test/behavior | Exit | +|:--|:--|:--|:--|:--|:--| +| String forms of MTG symbols and links | User code and historical files | Public input normalization | Convert once to `Symbol` | Node and MTG reader tests; no warning | Retain while these public input forms are documented | +| Historical MTG text files | External MTG datasets | External format | `read_mtg` parses into the canonical node and columnar representation | Reader and writer round-trip tests; no warning | Indefinite external-format boundary | + +These input formats do not require ordinary runtime code to maintain a second writable +representation. + +If a deprecated Julia call still appears in maintained code, migrate that caller instead of +adding another compatibility branch. diff --git a/src/MultiScaleTreeGraph.jl b/src/MultiScaleTreeGraph.jl index d9b63b2b..a6660f0a 100644 --- a/src/MultiScaleTreeGraph.jl +++ b/src/MultiScaleTreeGraph.jl @@ -62,13 +62,8 @@ export traverse export transform!, transform, select!, select export get_root export nextsibling, prevsibling, lastsibling -export print -export show -export length export MetaGraph -export iterate export siblings -export append! export @mutate_node! export @mutate_mtg! export is_filtered @@ -84,10 +79,9 @@ export Node export AbstractNodeMTG export NodeMTG export MutableNodeMTG -export (==) export check_filters export get_features, get_attributes -export names, scales, symbols, components +export scales, symbols, components export node_id, node_mtg, node_attributes export symbol, scale, index, link export symbol!, scale!, index!, link! diff --git a/src/compute_MTG/insert_nodes.jl b/src/compute_MTG/insert_nodes.jl index 06963e50..f427edba 100644 --- a/src/compute_MTG/insert_nodes.jl +++ b/src/compute_MTG/insert_nodes.jl @@ -394,4 +394,13 @@ function insert_generation!(node::Node{N,A}, template, attr_fun=node -> A(), max return node end -@deprecate insert_node!(node, template, maxid) insert_parent!(node, template, maxid) +# Retained through 0.16.x and removed in 0.17. +function insert_node!(node::Node{N,A}, template, maxid) where {N<:AbstractNodeMTG,A} + Base.depwarn( + "`insert_node!(node, template, maxid)` is deprecated; use " * + "`insert_parent!(node, template, _ -> typeof(node_attributes(node))(), maxid)` " * + "instead. The compatibility alias will be removed in MultiScaleTreeGraph 0.17.", + :insert_node!, + ) + return insert_parent!(node, template, _ -> A(), maxid) +end diff --git a/src/types/Node.jl b/src/types/Node.jl index ace7deed..0c004ae3 100644 --- a/src/types/Node.jl +++ b/src/types/Node.jl @@ -95,7 +95,7 @@ function Node( end # Deprecated named-node constructors are retained through 0.16.x and removed in 0.17. -@deprecate Node(name::String, id::Int, parent::Union{Nothing,Node{N,A}}, children::Nothing, MTG::N, attributes::A, traversal_cache::Dict{String,Vector{Node{N,A}}}) where {N<:AbstractNodeMTG,A} Node(id, parent, children, MTG, attributes, traversal_cache) +@deprecate Node(name::String, id::Int, parent::Union{Nothing,Node{N,A}}, children::Nothing, MTG::N, attributes::A, traversal_cache::Dict{String,Vector{Node{N,A}}}) where {N<:AbstractNodeMTG,A} Node(id, parent, Node{N,A}[], MTG, attributes, traversal_cache) @deprecate Node(name::String, id::Int, MTG::M, attributes::T) where {M<:AbstractNodeMTG,T<:MutableNamedTuple} Node(id, MTG, attributes) @deprecate Node(name::String, id::Int, MTG::M, attributes::T) where {M<:AbstractNodeMTG,T<:NamedTuple} Node(id, MTG, attributes) @deprecate Node(name::String, id::Int, parent::Node, MTG::M, attributes::A) where {M<:AbstractNodeMTG,A} Node(id, parent, MTG, attributes) diff --git a/test/runtests.jl b/test/runtests.jl index 63506c41..cb224749 100644 --- a/test/runtests.jl +++ b/test/runtests.jl @@ -4,6 +4,10 @@ using Dates import DataFrames: DataFrame, names, nrow using Graphs, AbstractTrees +@testset "compatibility boundaries" begin + include("test-compatibility.jl") +end + @testset "read_mtg" begin include("test-read_mtg.jl") end diff --git a/test/test-ancestors.jl b/test/test-ancestors.jl index 57bcc474..b9a13692 100644 --- a/test/test-ancestors.jl +++ b/test/test-ancestors.jl @@ -5,11 +5,6 @@ # Using a leaf node from the mtg: leaf_node = get_node(mtg, 5) - if Base.JLOptions().depwarn == 0 - @test ancestors(leaf_node, :Width; type=Union{Nothing,Float64}) == reverse(width_all[1:4]) - else - @test_logs (:warn, r"Keyword argument `type` in `ancestors` is deprecated") ancestors(leaf_node, :Width; type=Union{Nothing,Float64}) - end @test ancestors(leaf_node, :Width) == reverse(width_all[1:4]) d = ancestors(leaf_node, :Width, scale=3) diff --git a/test/test-compatibility.jl b/test/test-compatibility.jl new file mode 100644 index 00000000..80733411 --- /dev/null +++ b/test/test-compatibility.jl @@ -0,0 +1,67 @@ +@testset "deprecated Julia entry points" begin + legacy_node_call = () -> Node("legacy name", 7, NodeMTG(:/, :Plant, 1, 1), (x=1,)) + legacy_node = if Base.JLOptions().depwarn == 0 + legacy_node_call() + else + @test_logs (:warn, r"Node\(name::String.*is deprecated") legacy_node_call() + end + @test node_id(legacy_node) == 7 + @test symbol(legacy_node) == :Plant + @test legacy_node[:x] == 1 + + raw_attributes = Dict{Symbol,Any}(:x => 2) + raw_cache = Dict{String,Vector{Node{NodeMTG,typeof(raw_attributes)}}}() + legacy_raw_call = () -> Node( + "legacy name", + 8, + nothing, + nothing, + NodeMTG(:/, :Plant, 1, 1), + raw_attributes, + raw_cache, + ) + legacy_raw = if Base.JLOptions().depwarn == 0 + legacy_raw_call() + else + @test_logs (:warn, r"Node\(name::String.*is deprecated") legacy_raw_call() + end + @test node_id(legacy_raw) == 8 + @test isempty(children(legacy_raw)) + @test legacy_raw[:x] == 2 + + mtg = read_mtg("files/simple_plant.mtg") + target = mtg[1][1] + template = MutableNodeMTG(:/, :Shoot, 0, 1) + previous_max = max_id(mtg) + maxid = [previous_max] + legacy_insert_call = () -> MultiScaleTreeGraph.insert_node!(target, template, maxid) + returned = if Base.JLOptions().depwarn == 0 + legacy_insert_call() + else + @test_logs (:warn, r"insert_node!.*is deprecated") legacy_insert_call() + end + @test returned === target + @test maxid == [previous_max + 1] + @test parent(target) !== nothing + @test node_id(parent(target)) == previous_max + 1 + + type_mtg = read_mtg("files/simple_plant.mtg") + width_all = [nothing, nothing, 0.02, 0.1, 0.02, 0.1] + leaf_node = get_node(type_mtg, 5) + legacy_descendants_call = () -> descendants(type_mtg, :Width; type=Union{Nothing,Float64}) + legacy_ancestors_call = () -> ancestors(leaf_node, :Width; type=Union{Nothing,Float64}) + legacy_descendants_result, legacy_ancestors_result = if Base.JLOptions().depwarn == 0 + (legacy_descendants_call(), legacy_ancestors_call()) + else + descendants_result = @test_logs (:warn, r"Keyword argument `type` in `descendants` is deprecated") legacy_descendants_call() + ancestors_result = @test_logs (:warn, r"Keyword argument `type` in `ancestors` is deprecated") legacy_ancestors_call() + (descendants_result, ancestors_result) + end + @test legacy_descendants_result == width_all + @test legacy_ancestors_result == reverse([nothing, nothing, nothing, 0.02]) +end + +@testset "Base names are extended, not re-exported" begin + removed_exports = (:print, :show, :length, :iterate, :append!, :names, Symbol("==")) + @test all(name -> name ∉ Base.names(MultiScaleTreeGraph), removed_exports) +end diff --git a/test/test-descendants.jl b/test/test-descendants.jl index 26176720..d4533e88 100644 --- a/test/test-descendants.jl +++ b/test/test-descendants.jl @@ -1,11 +1,6 @@ @testset "descendants" begin mtg = read_mtg("files/simple_plant.mtg") width_all = [nothing, nothing, 0.02, 0.1, 0.02, 0.1] - if Base.JLOptions().depwarn == 0 - @test descendants(mtg, :Width; type=Union{Nothing,Float64}) == width_all - else - @test_logs (:warn, r"Keyword argument `type` in `descendants` is deprecated") descendants(mtg, :Width; type=Union{Nothing,Float64}) - end @test descendants(mtg, :Width) == width_all d = descendants(mtg, :Width, scale=1) From b445fa75a9b571fc204fa367ba4583e81416930b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Wed, 26 Aug 2026 21:29:16 +0200 Subject: [PATCH 07/13] fix: clarify MTG compatibility migrations --- docs/src/compatibility.md | 48 +++++++++-- .../tutorials/7.performance_considerations.md | 6 +- src/compute_MTG/ancestors.jl | 8 +- src/compute_MTG/descendants.jl | 37 ++++++-- test/test-compatibility.jl | 85 ++++++++++++++++++- 5 files changed, 164 insertions(+), 20 deletions(-) diff --git a/docs/src/compatibility.md b/docs/src/compatibility.md index 385b2e08..6ac84b39 100644 --- a/docs/src/compatibility.md +++ b/docs/src/compatibility.md @@ -13,13 +13,38 @@ scheduled for removal in MultiScaleTreeGraph 0.17: |:--|:--|:--|:--|:--|:--| | `Node(name, id, ...)` constructors | Public API from when a separate string name identified each node | Public API deprecation | `Node(id, ...)`; a node's MTG symbol carries its identity | `test/test-compatibility.jl`; Julia deprecation warning | Remove in 0.17 | | `insert_node!(node, template, maxid)` | Public predecessor of the explicit insertion operations | Public API deprecation | Normally `insert_parent!(node, template)`. To preserve a caller-managed ID counter, use `insert_parent!(node, template, _ -> typeof(node_attributes(node))(), maxid)` | `test/test-compatibility.jl`; Julia deprecation warning | Remove in 0.17 | -| `type=` in `descendants` and `ancestors` | Caller-selected result storage before attribute result types were inferred | Public API deprecation | Omit `type=`; typed columns infer the result type. Use `ignore_nothing=true` to filter absent values | `test/test-compatibility.jl`; Julia deprecation warning | Remove in 0.17 | +| `type=` in attribute-value overloads of `descendants`, `descendants!`, `ancestors`, and `ancestors!` | Caller-selected result storage before attribute result types were inferred | Public API deprecation | Omit `type=`. Single-key non-mutating calls infer when possible; use an explicit typed buffer when an exact element type is required. Use `ignore_nothing=true` to filter absent values | `test/test-compatibility.jl`; Julia deprecation warning | Remove in 0.17 | An inventory of MultiScaleTreeGraph and its maintained ecosystem packages found no package, tutorial, or documentation caller of the named constructors or `insert_node!`. The only -maintained uses of `type=` are the explicit compatibility checks. Ordinary tests use the -current inferred-result API. A deprecated call found in maintained downstream code should be -migrated, not used to justify extending this window. +maintained uses of `type=` on `descendants` or `ancestors` are the explicit compatibility +checks. Ordinary tests use the current inferred-result API; `traverse(...; type=...)` remains +a separate supported keyword. A deprecated call found in maintained downstream code should +be migrated, not used to justify extending this window. + +### Migration details + +- The named-node family includes root constructors for `NamedTuple` and + `MutableNamedTuple`, attached-node constructors for those and other attribute + backends, plus the low-level + parent/children/cache form. For the ordinary constructors, remove the leading string name + and keep the ID, MTG metadata, parent (when present), and attributes. Code using the + low-level form should use the current constructor shape and pass an actual children vector + instead of the historical `nothing` placeholder. +- `insert_node!` historically inserted a parent. Replace it with `insert_parent!`; retain the + explicit attribute factory and `maxid` argument only when the caller really owns that ID + counter. +- For single-key `descendants` and `ancestors` calls, remove `type=`. Columnar attributes + provide an inferred result type; without enough type information, a result may use a + broader element type. Multi-key calls retain their heterogeneous result representation. + When an exact element type is required, encode it in the reusable output buffer, for + example `values = Union{Nothing,Float64}[]` followed by + `descendants!(values, node, :Width)`. The dict-backed cache-producing + `descendants!(node, key)` form can also drop `type=`; callers that require a typed result + should use the explicit-buffer form. + +This removal does not apply to `traverse(...; type=...)`, where `type` remains part of the +current public API. !!! compat "Base function exports" MultiScaleTreeGraph defines methods of Base functions such as `show`, `length`, @@ -32,10 +57,23 @@ migrated, not used to justify extending this window. | Boundary | Producer | Category | Normalization | Test/behavior | Exit | |:--|:--|:--|:--|:--|:--| | String forms of MTG symbols and links | User code and historical files | Public input normalization | Convert once to `Symbol` | Node and MTG reader tests; no warning | Retain while these public input forms are documented | -| Historical MTG text files | External MTG datasets | External format | `read_mtg` parses into the canonical node and columnar representation | Reader and writer round-trip tests; no warning | Indefinite external-format boundary | +| Historical MTG text files | External MTG datasets | External format | `read_mtg` parses into the canonical node and columnar representation | `test/test-read_mtg.jl` and `test/test-write_mtg.jl`; no warning | Indefinite external-format boundary | These input formats do not require ordinary runtime code to maintain a second writable representation. +### Private traversal fallback + +The public contract is the result of `get_features`. The private +`_get_features_legacy` helper is an internal traversal fallback used when the optimized +columnar path cannot safely handle a backing store. Its name describes the implementation +path, not a legacy MTG file format. It is neither exported nor versioned as a compatibility +API and may be changed or removed without a deprecation cycle, provided `get_features` +retains its public behavior. The equivalence checks in `test/test-summary.jl` protect that +behavior; they do not make the helper public. + +Support for external MTG text files is independent of this helper and remains the permanent +reader/writer boundary described above. + If a deprecated Julia call still appears in maintained code, migrate that caller instead of adding another compatibility branch. diff --git a/docs/src/tutorials/7.performance_considerations.md b/docs/src/tutorials/7.performance_considerations.md index f6ecabd5..67976769 100644 --- a/docs/src/tutorials/7.performance_considerations.md +++ b/docs/src/tutorials/7.performance_considerations.md @@ -107,7 +107,11 @@ descendants!(nodes, mtg, self=true) This pattern is especially useful when the same request is executed many times (e.g. across millions of leaves). -`type=` for `descendants`/`ancestors` is deprecated because return eltypes are inferred automatically from typed columns. It is scheduled for removal in MultiScaleTreeGraph 0.17; maintained code should omit it now. +`type=` for attribute-value calls to `descendants`, `descendants!`, `ancestors`, and +`ancestors!` is deprecated and scheduled for removal in MultiScaleTreeGraph 0.17. Single-key +non-mutating calls infer their return eltype from typed columns; explicit-buffer in-place +calls take it from the reusable output buffer. Maintained code should omit the keyword now. +This deprecation does not apply to `traverse(...; type=...)`. ### Hybrid descendants backend (growth-safe) diff --git a/src/compute_MTG/ancestors.jl b/src/compute_MTG/ancestors.jl index d102d890..bce7b53e 100644 --- a/src/compute_MTG/ancestors.jl +++ b/src/compute_MTG/ancestors.jl @@ -71,7 +71,9 @@ is filtered out (`false`). `recursivity_level = 2`. If a negative value is provided (the default), the function returns all valid values from the node to the root. - `ignore_nothing = false`: filter-out the nodes with `nothing` values for the given `key` -- `type::Union{Union,DataType}`: Deprecated. Return types are inferred automatically. +- `type::Union{Union,DataType}`: Deprecated and scheduled for removal in 0.17. Omit it + for `ancestors`; single-key allocating calls infer their result type. For + `ancestors!(out, node, key)`, choose the element type of `out`. # Examples @@ -114,7 +116,7 @@ function ancestors( symbol = normalize_symbol_filter(symbol) link = normalize_link_filter(link) - _maybe_depwarn_traversal_type_kw(:ancestors, type) + _maybe_depwarn_traversal_type_kw(:ancestors, type, :single_allocating) key_ = Symbol(key) # Check the filters once, and then compute the ancestors recursively using `ancestors_` @@ -277,7 +279,7 @@ function ancestors!( symbol = normalize_symbol_filter(symbol) link = normalize_link_filter(link) - _maybe_depwarn_traversal_type_kw(:ancestors!, type) + _maybe_depwarn_traversal_type_kw(:ancestors!, type, :explicit_buffer) key_ = Symbol(key) check_filters(node, scale=scale, symbol=symbol, link=link) filter_fun_ = filter_fun_nothing(filter_fun, ignore_nothing, key_) diff --git a/src/compute_MTG/descendants.jl b/src/compute_MTG/descendants.jl index c7b720ec..3ef441e8 100644 --- a/src/compute_MTG/descendants.jl +++ b/src/compute_MTG/descendants.jl @@ -1,10 +1,24 @@ -@inline function _maybe_depwarn_traversal_type_kw(fname::Symbol, type) +@inline function _maybe_depwarn_traversal_type_kw(fname::Symbol, type, result_storage::Symbol) type === Any && return nothing + replacement = if result_storage === :single_allocating + "Remove `type`; single-key allocating calls infer their result type when possible. " * + "Use an explicit typed output buffer when an exact element type is required." + elseif result_storage === :explicit_buffer + "Remove `type`; the result element type is taken from the reusable output buffer." + elseif result_storage === :multi_allocating + "Remove `type`; multi-key calls retain their heterogeneous result representation." + elseif result_storage === :cache + "The cache-producing form does not infer a typed result. Remove `type`; callers " * + "that require a typed result should use `descendants!(out, node, key)` and choose " * + "the element type of `out`." + else + error("Unknown traversal result storage mode: $result_storage") + end Base.depwarn( "Keyword argument `type` in `$fname` is deprecated and will be removed in " * "MultiScaleTreeGraph 0.17. " * - "Return types are inferred automatically from the columnar attribute store; remove `type` " * - "and use `ignore_nothing=true` when you want `nothing` values filtered out.", + replacement * + " Use `ignore_nothing=true` when you want `nothing` values filtered out.", fname, ) return nothing @@ -356,7 +370,7 @@ function descendants( type::Union{Union,DataType}=Any) symbol = normalize_symbol_filter(symbol) link = normalize_link_filter(link) - _maybe_depwarn_traversal_type_kw(:descendants, type) + _maybe_depwarn_traversal_type_kw(:descendants, type, :single_allocating) # Check the filters once, and then compute the descendants recursively using `descendants_` check_filters(node, scale=scale, symbol=symbol, link=link) @@ -411,7 +425,7 @@ function descendants( ) symbol = normalize_symbol_filter(symbol) link = normalize_link_filter(link) - _maybe_depwarn_traversal_type_kw(:descendants, type) + _maybe_depwarn_traversal_type_kw(:descendants, type, :multi_allocating) check_filters(node, scale=scale, symbol=symbol, link=link) keys = _normalize_descendant_keys(key) @@ -524,7 +538,7 @@ function descendants!( ) symbol = normalize_symbol_filter(symbol) link = normalize_link_filter(link) - _maybe_depwarn_traversal_type_kw(:descendants!, type) + _maybe_depwarn_traversal_type_kw(:descendants!, type, :explicit_buffer) check_filters(node, scale=scale, symbol=symbol, link=link) key_ = Symbol(key) key_plan = build_columnar_query_plan(node, key_) @@ -573,7 +587,7 @@ function descendants!( ) symbol = normalize_symbol_filter(symbol) link = normalize_link_filter(link) - _maybe_depwarn_traversal_type_kw(:descendants!, type) + _maybe_depwarn_traversal_type_kw(:descendants!, type, :explicit_buffer) check_filters(node, scale=scale, symbol=symbol, link=link) keys = _normalize_descendant_keys(key) @@ -679,7 +693,7 @@ function descendants!( type::Union{Union,DataType}=Any) where {N,A<:AbstractDict} symbol = normalize_symbol_filter(symbol) link = normalize_link_filter(link) - _maybe_depwarn_traversal_type_kw(:descendants!, type) + _maybe_depwarn_traversal_type_kw(:descendants!, type, :cache) # Check the filters once, and then compute the descendants recursively using `descendants_` check_filters(node, scale=scale, symbol=symbol, link=link) @@ -779,7 +793,12 @@ is filtered out (`false`). grand-children: `recursivity_level = 2`. If `Inf` (the default) or a negative value is provided, there is no recursion limitation. - `ignore_nothing = false`: filter-out the nodes with `nothing` values for the given `key` -- `type::Union{Union,DataType}`: Deprecated. Return types are inferred automatically. +- `type::Union{Union,DataType}`: Deprecated and scheduled for removal in 0.17. Omit it + for allocating calls; single-key calls infer from the columnar attribute store and + multi-key calls retain a heterogeneous representation. For + `descendants!(out, node, key)`, choose the element type of `out`. The cache-producing + `descendants!(node, key)` form should be replaced by the explicit-buffer form when a + typed result is required. # Tips diff --git a/test/test-compatibility.jl b/test/test-compatibility.jl index 80733411..cf7058e3 100644 --- a/test/test-compatibility.jl +++ b/test/test-compatibility.jl @@ -29,6 +29,18 @@ @test isempty(children(legacy_raw)) @test legacy_raw[:x] == 2 + raw_child_attributes = Dict{Symbol,Any}(:x => 3) + raw_child_cache = Dict{String,Vector{Node{NodeMTG,typeof(raw_child_attributes)}}}() + raw_child = Node( + 9, + legacy_raw, + Node{NodeMTG,typeof(raw_child_attributes)}[], + NodeMTG(:+, :Leaf, 1, 2), + raw_child_attributes, + raw_child_cache, + ) + push!(children(legacy_raw), raw_child) + mtg = read_mtg("files/simple_plant.mtg") target = mtg[1][1] template = MutableNodeMTG(:/, :Shoot, 0, 1) @@ -50,15 +62,84 @@ leaf_node = get_node(type_mtg, 5) legacy_descendants_call = () -> descendants(type_mtg, :Width; type=Union{Nothing,Float64}) legacy_ancestors_call = () -> ancestors(leaf_node, :Width; type=Union{Nothing,Float64}) + descendants_buffer = Union{Nothing,Float64}[] + ancestors_buffer = Union{Nothing,Float64}[] + # The deprecated keyword deliberately disagrees with the buffers: these overloads + # have always taken their element type from `out`. + legacy_descendants_bang_call = () -> descendants!( + descendants_buffer, type_mtg, :Width; type=String + ) + legacy_ancestors_bang_call = () -> ancestors!( + ancestors_buffer, leaf_node, :Width; type=String + ) + expected_multi = descendants(type_mtg, (:Width, :Length)) + legacy_multi_call = () -> descendants( + type_mtg, (:Width, :Length); type=Union{Nothing,Float64} + ) + multi_buffer = Any[] + legacy_multi_bang_call = () -> descendants!( + multi_buffer, type_mtg, (:Width, :Length); type=Union{Nothing,Float64} + ) + legacy_cached_call = () -> descendants!( + legacy_raw, :x; type=Union{Nothing,Int} + ) legacy_descendants_result, legacy_ancestors_result = if Base.JLOptions().depwarn == 0 (legacy_descendants_call(), legacy_ancestors_call()) else - descendants_result = @test_logs (:warn, r"Keyword argument `type` in `descendants` is deprecated") legacy_descendants_call() - ancestors_result = @test_logs (:warn, r"Keyword argument `type` in `ancestors` is deprecated") legacy_ancestors_call() + descendants_result = @test_logs( + (:warn, r"`type` in `descendants` is deprecated.*single-key allocating calls infer their result type"), + legacy_descendants_call(), + ) + ancestors_result = @test_logs( + (:warn, r"`type` in `ancestors` is deprecated.*single-key allocating calls infer their result type"), + legacy_ancestors_call(), + ) (descendants_result, ancestors_result) end @test legacy_descendants_result == width_all @test legacy_ancestors_result == reverse([nothing, nothing, nothing, 0.02]) + + legacy_descendants_bang_result, legacy_ancestors_bang_result = if Base.JLOptions().depwarn == 0 + (legacy_descendants_bang_call(), legacy_ancestors_bang_call()) + else + descendants_result = @test_logs( + (:warn, r"`type` in `descendants!` is deprecated.*element type is taken from the reusable output buffer"), + legacy_descendants_bang_call(), + ) + ancestors_result = @test_logs( + (:warn, r"`type` in `ancestors!` is deprecated.*element type is taken from the reusable output buffer"), + legacy_ancestors_bang_call(), + ) + (descendants_result, ancestors_result) + end + @test legacy_descendants_bang_result === descendants_buffer + @test legacy_ancestors_bang_result === ancestors_buffer + @test descendants_buffer == width_all + @test ancestors_buffer == reverse([nothing, nothing, nothing, 0.02]) + + legacy_multi_result, legacy_multi_bang_result, legacy_cached_result = + if Base.JLOptions().depwarn == 0 + (legacy_multi_call(), legacy_multi_bang_call(), legacy_cached_call()) + else + multi_result = @test_logs( + (:warn, r"`type` in `descendants` is deprecated.*multi-key calls retain their heterogeneous result representation"), + legacy_multi_call(), + ) + multi_bang_result = @test_logs( + (:warn, r"`type` in `descendants!` is deprecated.*element type is taken from the reusable output buffer"), + legacy_multi_bang_call(), + ) + cached_result = @test_logs( + (:warn, r"`type` in `descendants!` is deprecated.*cache-producing form does not infer a typed result"), + legacy_cached_call(), + ) + (multi_result, multi_bang_result, cached_result) + end + @test legacy_multi_result == expected_multi + @test legacy_multi_bang_result === multi_buffer + @test multi_buffer == expected_multi + @test legacy_cached_result == Union{Nothing,Int}[3] + @test eltype(legacy_cached_result) == Union{Nothing,Int} end @testset "Base names are extended, not re-exported" begin From 4ae23b5611e5336d34ea692566896ac943fcb171 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Thu, 27 Aug 2026 03:38:39 +0200 Subject: [PATCH 08/13] perf: benchmark row mutation and topology growth --- benchmark/Project.toml | 4 +- benchmark/README.md | 20 + benchmark/benchmarks.jl | 4 + .../test-row-mutation-topology-benchmark.jl | 346 ++++++++++++++++++ benchmark/test/runtests.jl | 225 ++++++++++++ 5 files changed, 598 insertions(+), 1 deletion(-) create mode 100644 benchmark/test-row-mutation-topology-benchmark.jl create mode 100644 benchmark/test/runtests.jl diff --git a/benchmark/Project.toml b/benchmark/Project.toml index cc73667f..e75f8402 100644 --- a/benchmark/Project.toml +++ b/benchmark/Project.toml @@ -1,7 +1,9 @@ [deps] BenchmarkTools = "6e4b80f9-dd63-53aa-95a3-0cdb28fa8baf" +Logging = "56ddb016-857b-54e1-b83d-db4d58db5568" MultiScaleTreeGraph = "dd4a991b-8a45-4075-bede-262ee62d5583" Tables = "bd369af6-aec1-5ad0-b16a-f7cc5008161c" +Test = "8dfed614-e22c-5e08-85e1-65c5234f0b40" [sources] -MultiScaleTreeGraph = {path = ".."} \ No newline at end of file +MultiScaleTreeGraph = {path = ".."} diff --git a/benchmark/README.md b/benchmark/README.md index 17430ee7..58e91026 100644 --- a/benchmark/README.md +++ b/benchmark/README.md @@ -14,6 +14,26 @@ Run the byte-parity, allocation, and timing gates for the streaming MTG writer: julia --project=benchmark benchmark/write_mtg_streaming.jl ``` +Run the focused correctness gate for A1 row-local mutation and topology fixtures: + +```bash +julia --project=benchmark benchmark/test/runtests.jl +``` + +The `a1_row_mutation_topology` benchmark group uses deterministic fixtures with +32, 256, and 1,024 leaf rows plus one root. It measures cold explicit-ID +construction, one hot explicit-ID append, and `write_mtg` for dense and sparse +attributes. Row-local `pop!`, `empty!`, and sparse-toggle workloads are registered +only when a public behavior probe confirms that the loaded MultiScaleTreeGraph +revision supports row-local absence. Historical schema-wide revisions keep their +semantic oracle but are not used as a timing baseline for those different +operations. + +The `preexisting_auto_id` subgroup deliberately exercises repeated ID-less node +construction. It exposes the historical full-tree `max_id` search separately +from the A1 row-presence cost and can be reused unchanged to validate a future +topology-growth correction. + The writer gate exercises 40 features at 1,000 and 10,000 nodes. It compares the streaming path with the compatibility materialization path, measures seven alternating samples after warmup, and checks linear scaling. diff --git a/benchmark/benchmarks.jl b/benchmark/benchmarks.jl index fa59f31b..eea1a522 100644 --- a/benchmark/benchmarks.jl +++ b/benchmark/benchmarks.jl @@ -3,6 +3,9 @@ using MultiScaleTreeGraph using Random using Tables +include("test-row-mutation-topology-benchmark.jl") +using .A1RowMutationTopologyBenchmarks: build_a1_benchmark_suite! + const SUITE = BenchmarkGroup() const SIZE_TIERS = ( @@ -325,6 +328,7 @@ SUITE[suite_name] = BenchmarkGroup() build_tier!(SUITE[suite_name], "small", SIZE_TIERS.small) build_tier!(SUITE[suite_name], "medium", SIZE_TIERS.medium) build_tier!(SUITE[suite_name], "large", SIZE_TIERS.large) +build_a1_benchmark_suite!(SUITE[suite_name]) # Keep the largest tier focused on critical hot paths. delete!(SUITE[suite_name]["large"], "api_surface_small_only") diff --git a/benchmark/test-row-mutation-topology-benchmark.jl b/benchmark/test-row-mutation-topology-benchmark.jl new file mode 100644 index 00000000..fe9ebf49 --- /dev/null +++ b/benchmark/test-row-mutation-topology-benchmark.jl @@ -0,0 +1,346 @@ +module A1RowMutationTopologyBenchmarks + +using BenchmarkTools +using Logging: NullLogger, with_logger +using MultiScaleTreeGraph + +export A1_BENCHMARK_SAMPLES, + A1_MUTATION_REPETITIONS, + A1_PACKAGE_VERSION, + A1_SIZE_TIERS, + ROW_MUTATION_PROBE, + append_dense_explicit!, + build_a1_benchmark_suite!, + build_dense_auto_id, + build_dense_explicit, + build_sparse_explicit, + empty_restore_batch!, + pop_restore_batch!, + prepare_sparse_toggle_fixture, + row_local_mutation_supported, + row_mutation_mode, + sparse_toggle_batch!, + write_fixture + +const A1_SIZE_TIERS = (32, 256, 1024) +const A1_BENCHMARK_SAMPLES = 12 +const A1_MUTATION_REPETITIONS = 1024 +const A1_ABSENT = Symbol("__a1_absent__") +const A1_PACKAGE_VERSION = Base.pkgversion(MultiScaleTreeGraph) + +@inline _leaf_link(index::Int) = isone(index) ? :/ : :+ + +function fresh_root() + return Node( + 1, + MutableNodeMTG(:/, :Plant, 1, 1), + (root_value=1,), + ) +end + +function grow_dense_explicit!(root, n::Int) + leaves = Vector{typeof(root)}() + sizehint!(leaves, n) + for index in 1:n + leaf = Node( + index + 1, + root, + MutableNodeMTG(_leaf_link(index), :Leaf, index, 2), + (x=index, y=Float64(index), flag=isodd(index)), + ) + push!(leaves, leaf) + end + return (root=root, leaves=leaves) +end + +function build_dense_explicit(n::Int) + return grow_dense_explicit!(fresh_root(), n) +end + +function append_dense_explicit!(root, existing_leaf_count::Int) + leaf_index = existing_leaf_count + 1 + return Node( + leaf_index + 1, + root, + MutableNodeMTG(:+, :Leaf, leaf_index, 2), + (x=leaf_index, y=Float64(leaf_index), flag=isodd(leaf_index)), + ) +end + +@inline function _sparse_attributes(index::Int) + if iszero(index % 8) + return NamedTuple{(:shared, :local)}((index, index)) + end + return (shared=index,) +end + +function grow_sparse_explicit!(root, n::Int) + leaves = Vector{typeof(root)}() + sizehint!(leaves, n) + for index in 1:n + leaf = Node( + index + 1, + root, + MutableNodeMTG(_leaf_link(index), :Leaf, index, 2), + _sparse_attributes(index), + ) + push!(leaves, leaf) + end + return (root=root, leaves=leaves) +end + +function build_sparse_explicit(n::Int) + return grow_sparse_explicit!(fresh_root(), n) +end + +function build_dense_auto_id(n::Int) + root = fresh_root() + leaves = Vector{typeof(root)}() + sizehint!(leaves, n) + for index in 1:n + # This intentionally exercises the historical ID-less constructor. At + # the revisions used as A1 baselines, it calls max_id(root) for every + # new node and therefore performs a repeated full-tree search. + leaf = Node( + root, + MutableNodeMTG(_leaf_link(index), :Leaf, index, 2), + (x=index, y=Float64(index), flag=isodd(index)), + ) + push!(leaves, leaf) + end + return (root=root, leaves=leaves) +end + +function _mutation_probe() + pop_data = build_dense_explicit(2) + popped_value = Base.pop!(node_attributes(pop_data.leaves[1]), :x) + pop_target_absent = !haskey(pop_data.leaves[1], :x) + pop_neighbor_present = + haskey(pop_data.leaves[2], :x) && + attribute(pop_data.leaves[2], :x, A1_ABSENT) == 2 + + delete_data = build_dense_explicit(2) + Base.delete!(node_attributes(delete_data.leaves[1]), :x) + delete_target_absent = !haskey(delete_data.leaves[1], :x) + delete_neighbor_present = + haskey(delete_data.leaves[2], :x) && + attribute(delete_data.leaves[2], :x, A1_ABSENT) == 2 + + empty_data = build_dense_explicit(2) + Base.empty!(node_attributes(empty_data.leaves[1])) + empty_target_empty = isempty(node_attributes(empty_data.leaves[1])) + empty_neighbor_present = + haskey(empty_data.leaves[2], :x) && + attribute(empty_data.leaves[2], :x, A1_ABSENT) == 2 + + nullable_data = build_dense_explicit(2) + nullable_data.leaves[1][:nullable] = nothing + nullable_target_present = + haskey(nullable_data.leaves[1], :nullable) && + nullable_data.leaves[1][:nullable] === nothing + nullable_neighbor_present = haskey(nullable_data.leaves[2], :nullable) + + schema_drop_data = build_dense_explicit(2) + drop_column!(schema_drop_data.root, :Leaf, :x) + explicit_schema_drop = all(leaf -> !haskey(leaf, :x), schema_drop_data.leaves) + + row_local_signature = + popped_value == 1 && + pop_target_absent && + pop_neighbor_present && + delete_target_absent && + delete_neighbor_present && + empty_target_empty && + empty_neighbor_present && + nullable_target_present && + !nullable_neighbor_present && + explicit_schema_drop + + schema_wide_signature = + popped_value == 1 && + pop_target_absent && + !pop_neighbor_present && + delete_target_absent && + !delete_neighbor_present && + empty_target_empty && + !empty_neighbor_present && + nullable_target_present && + nullable_neighbor_present && + explicit_schema_drop + + mode = if row_local_signature + :row_local + elseif schema_wide_signature + :legacy_schema_wide + else + :unknown + end + + return ( + mode=mode, + package_version=A1_PACKAGE_VERSION, + popped_value=popped_value, + pop_target_absent=pop_target_absent, + pop_neighbor_present=pop_neighbor_present, + delete_target_absent=delete_target_absent, + delete_neighbor_present=delete_neighbor_present, + empty_target_empty=empty_target_empty, + empty_neighbor_present=empty_neighbor_present, + nullable_target_present=nullable_target_present, + nullable_neighbor_present=nullable_neighbor_present, + explicit_schema_drop=explicit_schema_drop, + ) +end + +const ROW_MUTATION_PROBE = _mutation_probe() + +row_mutation_mode() = ROW_MUTATION_PROBE.mode +row_local_mutation_supported() = row_mutation_mode() === :row_local + +function pop_restore_batch!(leaves, repetitions::Int=A1_MUTATION_REPETITIONS) + row_local_mutation_supported() || throw(ArgumentError( + "pop/restore is only benchmarked when row-local ColumnarAttrs mutation is available", + )) + checksum = 0 + nleaves = length(leaves) + @inbounds for repetition in 1:repetitions + leaf = leaves[mod1(17 * repetition, nleaves)] + attrs = node_attributes(leaf) + value = Base.pop!(attrs, :x) + attrs[:x] = value + checksum += value + end + return checksum +end + +function empty_restore_batch!(leaves, repetitions::Int=A1_MUTATION_REPETITIONS) + row_local_mutation_supported() || throw(ArgumentError( + "empty/restore is only benchmarked when row-local ColumnarAttrs mutation is available", + )) + checksum = 0 + nleaves = length(leaves) + @inbounds for repetition in 1:repetitions + leaf = leaves[mod1(17 * repetition, nleaves)] + attrs = node_attributes(leaf) + x = attrs[:x] + y = attrs[:y] + flag = attrs[:flag] + Base.empty!(attrs) + attrs[:x] = x + attrs[:y] = y + attrs[:flag] = flag + checksum += x + flag + end + return checksum +end + +function prepare_sparse_toggle_fixture(n::Int) + row_local_mutation_supported() || throw(ArgumentError( + "sparse toggles require row-local ColumnarAttrs mutation", + )) + data = build_sparse_explicit(n) + seed_attrs = node_attributes(first(data.leaves)) + seed_attrs[:ephemeral] = 0 + Base.pop!(seed_attrs, :ephemeral) + return data +end + +function sparse_toggle_batch!(leaves, repetitions::Int=A1_MUTATION_REPETITIONS) + row_local_mutation_supported() || throw(ArgumentError( + "sparse toggles require row-local ColumnarAttrs mutation", + )) + checksum = 0 + nleaves = length(leaves) + @inbounds for repetition in 1:repetitions + leaf = leaves[mod1(17 * repetition, nleaves)] + attrs = node_attributes(leaf) + attrs[:ephemeral] = repetition + checksum += Base.pop!(attrs, :ephemeral) + end + return checksum +end + +function write_fixture(path::AbstractString, root) + try + with_logger(NullLogger()) do + write_mtg(path, root) + end + return filesize(path) + catch + isfile(path) && rm(path; force=true) + rethrow() + end +end + +function build_a1_benchmark_suite!(suite::BenchmarkGroup) + mode = row_mutation_mode() + mode in (:legacy_schema_wide, :row_local) || error( + "Unsupported ColumnarAttrs mutation behavior $(mode); " * + "refusing to assemble a partial A1 benchmark suite.", + ) + a1 = BenchmarkGroup() + suite["a1_row_mutation_topology"] = a1 + repetitions = A1_MUTATION_REPETITIONS + + for n in A1_SIZE_TIERS + size_key = string(n) + + a1["explicit_cold"]["dense"][size_key] = + @benchmarkable build_dense_explicit($n) samples=A1_BENCHMARK_SAMPLES evals=1 + a1["explicit_cold"]["sparse"][size_key] = + @benchmarkable build_sparse_explicit($n) samples=A1_BENCHMARK_SAMPLES evals=1 + a1["explicit_hot_append"]["dense"][size_key] = @benchmarkable( + append_dense_explicit!(data_.root, $n), + setup=(data_ = build_dense_explicit($n)), + samples=A1_BENCHMARK_SAMPLES, + evals=1, + ) + + # Kept separate because this is a known pre-existing full-tree-search + # path, not a regression introduced by row-presence storage. + a1["preexisting_auto_id"]["dense"][size_key] = + @benchmarkable build_dense_auto_id($n) samples=A1_BENCHMARK_SAMPLES evals=1 + + dense_root = build_dense_explicit(n).root + sparse_root = build_sparse_explicit(n).root + a1["write_mtg"]["dense"][size_key] = @benchmarkable( + write_fixture(path_, $dense_root), + setup=(path_ = tempname() * ".mtg"), + teardown=(isfile(path_) && rm(path_; force=true)), + samples=A1_BENCHMARK_SAMPLES, + evals=1, + ) + a1["write_mtg"]["sparse"][size_key] = @benchmarkable( + write_fixture(path_, $sparse_root), + setup=(path_ = tempname() * ".mtg"), + teardown=(isfile(path_) && rm(path_; force=true)), + samples=A1_BENCHMARK_SAMPLES, + evals=1, + ) + + if mode === :row_local + a1["row_local"]["pop_restore"][size_key] = @benchmarkable( + pop_restore_batch!(data_.leaves, $repetitions), + setup=(data_ = build_dense_explicit($n)), + samples=A1_BENCHMARK_SAMPLES, + evals=1, + ) + a1["row_local"]["empty_restore"][size_key] = @benchmarkable( + empty_restore_batch!(data_.leaves, $repetitions), + setup=(data_ = build_dense_explicit($n)), + samples=A1_BENCHMARK_SAMPLES, + evals=1, + ) + a1["row_local"]["sparse_toggle"][size_key] = @benchmarkable( + sparse_toggle_batch!(data_.leaves, $repetitions), + setup=(data_ = prepare_sparse_toggle_fixture($n)), + samples=A1_BENCHMARK_SAMPLES, + evals=1, + ) + end + end + + return a1 +end + +end diff --git a/benchmark/test/runtests.jl b/benchmark/test/runtests.jl new file mode 100644 index 00000000..66597e64 --- /dev/null +++ b/benchmark/test/runtests.jl @@ -0,0 +1,225 @@ +using Test +using BenchmarkTools +using MultiScaleTreeGraph + +include(joinpath(@__DIR__, "..", "test-row-mutation-topology-benchmark.jl")) +using .A1RowMutationTopologyBenchmarks + +function assert_topology(data, n::Int) + @test length(data.leaves) == n + @test length(children(data.root)) == n + @test length(data.root) == n + 1 + @test max_id(data.root) == n + 1 + @test node_id.(data.leaves) == collect(2:(n + 1)) + + for index in unique((1, cld(n, 2), n)) + leaf = data.leaves[index] + @test parent(leaf) === data.root + @test get_node(data.root, index + 1) === leaf + end +end + +function assert_dense_attributes(data, n::Int) + for index in unique((1, cld(n, 2), n)) + leaf = data.leaves[index] + @test attribute(leaf, :x, nothing) == index + @test attribute(leaf, :y, nothing) == Float64(index) + @test attribute(leaf, :flag, nothing) == isodd(index) + end + dense_values = descendants(data.root, :x; symbol=:Leaf, ignore_nothing=true) + @test eltype(dense_values) === Int + @test dense_values == collect(1:n) +end + +function assert_sparse_attributes(data, n::Int, mode::Symbol) + for index in 1:n + leaf = data.leaves[index] + @test attribute(leaf, :shared, nothing) == index + if iszero(index % 8) + @test haskey(leaf, :local) + @test attribute(leaf, :local, nothing) == index + elseif mode === :row_local + @test !haskey(leaf, :local) + @test attribute(leaf, :local, :absent) === :absent + else + @test mode === :legacy_schema_wide + @test haskey(leaf, :local) + @test attribute(leaf, :local, :absent) === nothing + end + end + sparse_values = descendants(data.root, :local; symbol=:Leaf, ignore_nothing=true) + @test eltype(sparse_values) === Int + @test sparse_values == collect(8:8:n) +end + +function assert_benchmark_size_group(group) + expected_sizes = Set(string.(A1_SIZE_TIERS)) + @test Set(keys(group)) == expected_sizes + for size_key in expected_sizes + benchmark_parameters = params(group[size_key]) + @test benchmark_parameters.samples == A1_BENCHMARK_SAMPLES + @test benchmark_parameters.evals == 1 + end +end + +@testset "A1 mutation capability" begin + probe = ROW_MUTATION_PROBE + @test probe.package_version == A1_PACKAGE_VERSION + @test probe.mode in (:legacy_schema_wide, :row_local) + @test probe.popped_value == 1 + @test probe.pop_target_absent + @test probe.delete_target_absent + @test probe.empty_target_empty + @test probe.nullable_target_present + @test probe.explicit_schema_drop + + if probe.mode === :row_local + @test probe.pop_neighbor_present + @test probe.delete_neighbor_present + @test probe.empty_neighbor_present + @test !probe.nullable_neighbor_present + else + @test !probe.pop_neighbor_present + @test !probe.delete_neighbor_present + @test !probe.empty_neighbor_present + @test probe.nullable_neighbor_present + end +end + +@testset "A1 BenchmarkTools suite assembly" begin + suite = BenchmarkGroup() + a1 = build_a1_benchmark_suite!(suite) + + @test Set(keys(suite)) == Set(["a1_row_mutation_topology"]) + @test suite["a1_row_mutation_topology"] === a1 + + expected_groups = Set([ + "explicit_cold", + "explicit_hot_append", + "preexisting_auto_id", + "write_mtg", + ]) + row_local_mutation_supported() && push!(expected_groups, "row_local") + @test Set(keys(a1)) == expected_groups + + @test Set(keys(a1["explicit_cold"])) == Set(["dense", "sparse"]) + assert_benchmark_size_group(a1["explicit_cold"]["dense"]) + assert_benchmark_size_group(a1["explicit_cold"]["sparse"]) + + @test Set(keys(a1["explicit_hot_append"])) == Set(["dense"]) + assert_benchmark_size_group(a1["explicit_hot_append"]["dense"]) + + @test Set(keys(a1["preexisting_auto_id"])) == Set(["dense"]) + assert_benchmark_size_group(a1["preexisting_auto_id"]["dense"]) + + @test Set(keys(a1["write_mtg"])) == Set(["dense", "sparse"]) + assert_benchmark_size_group(a1["write_mtg"]["dense"]) + assert_benchmark_size_group(a1["write_mtg"]["sparse"]) + + if row_local_mutation_supported() + @test haskey(a1, "row_local") + @test Set(keys(a1["row_local"])) == + Set(["empty_restore", "pop_restore", "sparse_toggle"]) + assert_benchmark_size_group(a1["row_local"]["empty_restore"]) + assert_benchmark_size_group(a1["row_local"]["pop_restore"]) + assert_benchmark_size_group(a1["row_local"]["sparse_toggle"]) + else + @test row_mutation_mode() === :legacy_schema_wide + @test !haskey(a1, "row_local") + end +end + +@testset "A1 deterministic topology fixtures" begin + for n in A1_SIZE_TIERS + dense = build_dense_explicit(n) + assert_topology(dense, n) + assert_dense_attributes(dense, n) + + sparse = build_sparse_explicit(n) + assert_topology(sparse, n) + assert_sparse_attributes(sparse, n, row_mutation_mode()) + + auto_id = build_dense_auto_id(n) + assert_topology(auto_id, n) + assert_dense_attributes(auto_id, n) + end +end + +@testset "A1 hot explicit-ID append" begin + for n in A1_SIZE_TIERS + data = build_dense_explicit(n) + existing_last = last(data.leaves) + existing_last_before = attributes(existing_last; format=:dict) + + added = append_dense_explicit!(data.root, n) + + @test node_id(added) == n + 2 + @test parent(added) === data.root + @test last(children(data.root)) === added + @test length(children(data.root)) == n + 1 + @test length(data.root) == n + 2 + @test max_id(data.root) == n + 2 + @test get_node(data.root, n + 2) === added + @test attribute(added, :x, nothing) == n + 1 + @test attribute(added, :y, nothing) == Float64(n + 1) + @test attribute(added, :flag, nothing) == isodd(n + 1) + @test attributes(existing_last; format=:dict) == existing_last_before + end +end + +@testset "A1 modern row-local workloads restore their input" begin + if row_local_mutation_supported() + dense = build_dense_explicit(32) + dense_before = [attributes(leaf; format=:dict) for leaf in dense.leaves] + @test pop_restore_batch!(dense.leaves, 257) > 0 + @test [attributes(leaf; format=:dict) for leaf in dense.leaves] == dense_before + + @test empty_restore_batch!(dense.leaves, 257) > 0 + @test [attributes(leaf; format=:dict) for leaf in dense.leaves] == dense_before + + sparse = prepare_sparse_toggle_fixture(32) + @test all(leaf -> !haskey(leaf, :ephemeral), sparse.leaves) + @test sparse_toggle_batch!(sparse.leaves, 257) > 0 + @test all(leaf -> !haskey(leaf, :ephemeral), sparse.leaves) + else + @test row_mutation_mode() === :legacy_schema_wide + end +end + +@testset "A1 dense and sparse MTG round trips" begin + mktempdir() do directory + dense = build_dense_explicit(32) + dense_path_1 = joinpath(directory, "dense-1.mtg") + dense_path_2 = joinpath(directory, "dense-2.mtg") + @test write_fixture(dense_path_1, dense.root) > 0 + @test write_fixture(dense_path_2, dense.root) > 0 + @test read(dense_path_1) == read(dense_path_2) + + dense_roundtrip = read_mtg(dense_path_1) + @test length(dense_roundtrip) == 33 + @test max_id(dense_roundtrip) == 33 + for index in (1, 16, 32) + leaf = get_node(dense_roundtrip, index + 1) + @test attribute(leaf, :x, nothing) == index + @test attribute(leaf, :y, nothing) == Float64(index) + @test attribute(leaf, :flag, nothing) == isodd(index) + end + + sparse = build_sparse_explicit(32) + sparse_path_1 = joinpath(directory, "sparse-1.mtg") + sparse_path_2 = joinpath(directory, "sparse-2.mtg") + @test write_fixture(sparse_path_1, sparse.root) > 0 + @test write_fixture(sparse_path_2, sparse.root) > 0 + @test read(sparse_path_1) == read(sparse_path_2) + + sparse_roundtrip = read_mtg(sparse_path_1) + @test length(sparse_roundtrip) == 33 + @test max_id(sparse_roundtrip) == 33 + roundtrip_leaves = [get_node(sparse_roundtrip, index + 1) for index in 1:32] + assert_sparse_attributes( + (root=sparse_roundtrip, leaves=roundtrip_leaves), + 32, + row_mutation_mode(), + ) + end +end From ac316f72bcde2b342cbbc53bf58db54424940ead Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Thu, 27 Aug 2026 04:50:19 +0200 Subject: [PATCH 09/13] perf: optimize row-local column insertion --- src/types/Attributes.jl | 14 +++++++++----- test/test-columnar.jl | 26 ++++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 5 deletions(-) diff --git a/src/types/Attributes.jl b/src/types/Attributes.jl index fbbec524..1dc98751 100644 --- a/src/types/Attributes.jl +++ b/src/types/Attributes.jl @@ -319,6 +319,13 @@ end return nothing end +@inline function _append_default_row!(column::Column) + push!(column.data, column.default) + push!(column.present, column.default_present) + column.default_present && (column.n_present += 1) + return nothing +end + function _set_value!(bucket::SymbolBucket, row::Int, key::Symbol, value) col_idx = get(bucket.col_index, key, 0) if col_idx == 0 @@ -375,17 +382,14 @@ function _add_node_with_attrs!(store::MTGAttributeStore, node_id::Int, symbol::S bucket.node_to_row[node_id] = row @inbounds for i in eachindex(bucket.columns) - col = bucket.columns[i] - push!(col.data, col.default) - push!(col.present, col.default_present) - col.default_present && (col.n_present += 1) + _append_default_row!(bucket.columns[i]) end store.node_bucket[node_id] = bid store.node_row[node_id] = row for (k, v) in attrs - _set_value!(bucket, row, _normalize_attr_key(k), v) + _set_value_bound!(bucket, row, _normalize_attr_key(k), v) end return nothing diff --git a/test/test-columnar.jl b/test/test-columnar.jl index 201fbd16..968091f5 100644 --- a/test/test-columnar.jl +++ b/test/test-columnar.jl @@ -107,6 +107,32 @@ end @test !haskey(leaf_bucket.col_index, :x) end +@testset "Columnar child insertion preserves presence and widening" begin + root = Node(NodeMTG(:/, :Plant, 1, 1)) + add_column!(root, :Leaf, :temperature, Float64, default=20.0) + first_leaf = Node( + 2, + root, + NodeMTG(:/, :Leaf, 1, 2), + Dict{Any,Any}("temperature" => 21.0, "nullable" => nothing, :value => 1), + ) + second_leaf = Node(3, root, NodeMTG(:+, :Leaf, 2, 2), (value="widened",)) + + @test first_leaf[:temperature] == 21.0 + @test second_leaf[:temperature] == 20.0 + @test haskey(first_leaf, :nullable) + @test first_leaf[:nullable] === nothing + @test !haskey(second_leaf, :nullable) + @test first_leaf[:value] == 1 + @test second_leaf[:value] == "widened" + + store = MultiScaleTreeGraph._node_store(root) + leaf_bucket = store.buckets[store.symbol_to_bucket[:Leaf]] + @test leaf_bucket.columns[leaf_bucket.col_index[:temperature]].n_present == 2 + @test leaf_bucket.columns[leaf_bucket.col_index[:nullable]].n_present == 1 + @test leaf_bucket.columns[leaf_bucket.col_index[:value]].n_present == 2 +end + @testset "Sparse columnar attributes survive growth, merge, and MTG I/O" begin root = Node(NodeMTG(:/, :Plant, 1, 1)) first_leaf = Node(root, NodeMTG(:/, :Leaf, 1, 2), (local_value=1,)) From 6bbc3626cc7d761dcfaa1c5450bb116110112c9d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Thu, 27 Aug 2026 04:57:11 +0200 Subject: [PATCH 10/13] fix: restore column table bounds checks --- src/conversion/Tables.jl | 1 + test/test-columnar.jl | 5 +++++ 2 files changed, 6 insertions(+) diff --git a/src/conversion/Tables.jl b/src/conversion/Tables.jl index a1649cc8..e830abde 100644 --- a/src/conversion/Tables.jl +++ b/src/conversion/Tables.jl @@ -23,6 +23,7 @@ Base.length(col::ColumnarTableView) = length(col.column.data) end @inline function Base.getindex(col::ColumnarTableView{T}, i::Int) where {T} + @boundscheck checkbounds(col, i) column = col.column _row_has_value(column, i) || return missing::T value = @inbounds column.data[i] diff --git a/test/test-columnar.jl b/test/test-columnar.jl index 968091f5..bf1e4706 100644 --- a/test/test-columnar.jl +++ b/test/test-columnar.jl @@ -208,6 +208,11 @@ leaf_df = DataFrame(leaf_table) leaf_selected = to_table(mtg, symbol=:Leaf, vars=[:Width, "Length"]) @test Tables.columnnames(leaf_selected) == (:node_id, :Width, :Length) @test length(Tables.getcolumn(leaf_selected, :Width)) == nrow(leaf_df) +width_column = Tables.getcolumn(leaf_selected, :Width) +@test width_column[firstindex(width_column)] == leaf_df.Width[1] +@test width_column[lastindex(width_column)] == leaf_df.Width[end] +@test_throws BoundsError width_column[0] +@test_throws BoundsError width_column[lastindex(width_column) + 1] all_table = to_table(mtg) all_df = DataFrame(all_table) From bf2e3a6faa853d8bc4572117efec4fa693592cbc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Thu, 27 Aug 2026 05:10:30 +0200 Subject: [PATCH 11/13] fix: preserve schemas during columnarization --- .../tutorials/7.performance_considerations.md | 6 ++ src/compute_MTG/columnarize.jl | 64 ++++++++++++++++++- test/test-columnar.jl | 64 +++++++++++++++++++ 3 files changed, 133 insertions(+), 1 deletion(-) diff --git a/docs/src/tutorials/7.performance_considerations.md b/docs/src/tutorials/7.performance_considerations.md index 67976769..e134801e 100644 --- a/docs/src/tutorials/7.performance_considerations.md +++ b/docs/src/tutorials/7.performance_considerations.md @@ -54,6 +54,12 @@ Schema operations are deliberately separate: column. Use a schema operation only when the intended scope is all nodes of a symbol. +When `columnarize!` repairs a graph containing several attribute stores, the +root node's store is the schema authority: its column types and future defaults +are retained, while the current values from the other stores are imported. +The `.mtg` file format stores current values, not Julia-side `default` and +`default_present` metadata; reapply future-row defaults with `add_column!` +after reading a file when that behavior is required. ### MTG encoding diff --git a/src/compute_MTG/columnarize.jl b/src/compute_MTG/columnarize.jl index 2a681ad8..e5c3b1e8 100644 --- a/src/compute_MTG/columnarize.jl +++ b/src/compute_MTG/columnarize.jl @@ -2,21 +2,37 @@ columnarize!(mtg::Node) Bind all node attributes to a single `MTGAttributeStore`. + +The store that owns the root node is the schema authority. Its column types, +order, defaults, and row-local absences are preserved. Nodes imported from a +different store keep their current values and receive root-store defaults for +attributes they do not currently have, matching the normal subtree-attachment +semantics. """ function columnarize!(mtg::Node) root = get_root(mtg) nodes = traverse(root, node -> node, type=typeof(root)) isempty(nodes) && return mtg + root_attrs = node_attributes(root) + root_attrs isa ColumnarAttrs || + error("columnarize! expects nodes with ColumnarAttrs attributes.") + root_store = _store_for_node_attrs(root_attrs) store = MTGAttributeStore() + root_store === nothing || _copy_columnar_schema!(store, root_store) for n in nodes if _maybe_traversal_cache(n) !== nothing _register_traversal_cache!(store, n) end attrs = node_attributes(n) - attrs isa ColumnarAttrs || error("columnarize! expects nodes with ColumnarAttrs attributes.") + attrs isa ColumnarAttrs || + error("columnarize! expects nodes with ColumnarAttrs attributes.") + source_store = _store_for_node_attrs(attrs) raw = _isbound(attrs) ? Dict{Symbol,Any}(pairs(attrs)) : attrs.staged _add_node_with_attrs!(store, node_id(n), symbol(n), raw) + if root_store !== nothing && source_store === root_store + _restore_root_row_absences!(store, root_store, node_id(n)) + end end for n in nodes @@ -27,3 +43,49 @@ function columnarize!(mtg::Node) end return mtg end + +function _copy_columnar_schema!(target::MTGAttributeStore, source::MTGAttributeStore) + for source_bucket in source.buckets + target_bid = _get_or_create_bucket!(target, source_bucket.symbol) + target_bucket = target.buckets[target_bid] + @inbounds for i in eachindex(source_bucket.columns) + source_column = _column(source_bucket, i) + _copy_column_schema!(target_bucket, source_column) + end + end + return target +end + +@inline function _copy_column_schema!(target_bucket::SymbolBucket, source::Column{T}) where {T} + _add_column_internal!( + target_bucket, + source.name, + T, + source.default, + source.default_present, + ) + return nothing +end + +function _restore_root_row_absences!( + target::MTGAttributeStore, + source::MTGAttributeStore, + node_id::Int, +) + source_bid = source.node_bucket[node_id] + source_row = source.node_row[node_id] + target_bid = target.node_bucket[node_id] + target_row = target.node_row[node_id] + source_bucket = source.buckets[source_bid] + target_bucket = target.buckets[target_bid] + source_bucket.symbol === target_bucket.symbol || return nothing + + @inbounds for i in eachindex(source_bucket.columns) + source_column = _column(source_bucket, i) + if !source_column.present[source_row] + target_column = _column(target_bucket, target_bucket.col_index[source_column.name]) + _mark_row_absent!(target_column, target_row) + end + end + return nothing +end diff --git a/test/test-columnar.jl b/test/test-columnar.jl index bf1e4706..e2a8fa5d 100644 --- a/test/test-columnar.jl +++ b/test/test-columnar.jl @@ -133,6 +133,70 @@ end @test leaf_bucket.columns[leaf_bucket.col_index[:value]].n_present == 2 end +@testset "columnarize! preserves the root store schema" begin + root = Node(1, NodeMTG(:/, :Plant, 1, 1), (root_value=1,)) + first_leaf = Node(2, root, NodeMTG(:/, :Leaf, 1, 2), (leaf_value=1,)) + second_leaf = Node(3, root, NodeMTG(:+, :Leaf, 2, 2), (leaf_value=2,)) + + add_column!(root, :Leaf, :temperature, Float64, default=20.0) + add_column!(root, :Bud, :dormancy, Float64, default=1.5) + first_leaf[:retained_absent_column] = "temporary" + delete!(first_leaf, :retained_absent_column) + pop!(first_leaf, :temperature) + # A symbol edit changes the logical destination bucket but not the old + # physical bucket; `columnarize!` must handle that transition explicitly. + symbol!(second_leaf, :RenamedLeaf) + + root_store = MultiScaleTreeGraph._node_store(root) + root_leaf_bucket = root_store.buckets[root_store.symbol_to_bucket[:Leaf]] + root_leaf_columns = [column.name for column in root_leaf_bucket.columns] + + foreign_root = Node(100, NodeMTG(:+, :Branch, 1, 2), (foreign_root=true,)) + foreign_leaf = Node( + 101, + foreign_root, + NodeMTG(:+, :Leaf, 1, 3), + (foreign_value=99,), + ) + add_column!(foreign_root, :Leaf, :source_default, Float64, default=99.0) + # Build the mixed-store state that `columnarize!` is documented to repair. + push!(children(root), foreign_root) + setfield!(foreign_root, :parent, root) + + columnarize!(root) + + unified_store = MultiScaleTreeGraph._node_store(root) + @test unified_store !== root_store + @test all( + MultiScaleTreeGraph._node_store(node) === unified_store for + node in traverse(root, identity, type=typeof(root)) + ) + + leaf_bucket = unified_store.buckets[unified_store.symbol_to_bucket[:Leaf]] + unified_leaf_columns = [column.name for column in leaf_bucket.columns] + @test unified_leaf_columns[eachindex(root_leaf_columns)] == root_leaf_columns + temperature_index = leaf_bucket.col_index[:temperature] + temperature_column = leaf_bucket.columns[temperature_index] + @test leaf_bucket.col_types[temperature_index] === Float64 + @test temperature_column.default === 20.0 + @test temperature_column.default_present + @test haskey(leaf_bucket.col_index, :retained_absent_column) + + @test !haskey(first_leaf, :temperature) + @test symbol(second_leaf) === :RenamedLeaf + @test second_leaf[:temperature] === 20.0 + @test foreign_leaf[:temperature] === 20.0 + @test foreign_leaf[:foreign_value] == 99 + @test foreign_leaf[:source_default] === 99.0 + + new_leaf = Node(102, root, NodeMTG(:+, :Leaf, 3, 2)) + new_bud = Node(103, root, NodeMTG(:+, :Bud, 1, 2)) + @test new_leaf[:temperature] === 20.0 + @test !haskey(new_leaf, :retained_absent_column) + @test !haskey(new_leaf, :source_default) + @test new_bud[:dormancy] === 1.5 +end + @testset "Sparse columnar attributes survive growth, merge, and MTG I/O" begin root = Node(NodeMTG(:/, :Plant, 1, 1)) first_leaf = Node(root, NodeMTG(:/, :Leaf, 1, 2), (local_value=1,)) From a284956522e31c0b505b39747eecde8cf2a57493 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Thu, 27 Aug 2026 06:16:36 +0200 Subject: [PATCH 12/13] perf: avoid repeated MTG ID traversal --- src/compute_MTG/insert_nodes.jl | 51 +++++++++++-------- src/compute_MTG/node_funs.jl | 16 ++++-- src/types/Attributes.jl | 74 ++++++++++++++++++++++++--- src/types/Node.jl | 7 +++ test/test-caching.jl | 28 +++++++++++ test/test-columnar.jl | 89 +++++++++++++++++++++++++++++++++ test/test-delete-prune.jl | 22 ++++++++ test/test-insert_node.jl | 74 +++++++++++++++++++++++++++ test/test-read_mtg.jl | 2 + 9 files changed, 331 insertions(+), 32 deletions(-) diff --git a/src/compute_MTG/insert_nodes.jl b/src/compute_MTG/insert_nodes.jl index f427edba..cc33aa66 100644 --- a/src/compute_MTG/insert_nodes.jl +++ b/src/compute_MTG/insert_nodes.jl @@ -132,7 +132,7 @@ function insert_nodes!( filter_fun=nothing ) where {N<:AbstractNodeMTG,A} - max_node_id = [max_id(node)] + max_node_id = [_max_assigned_id(node)] # # Check the filters once, and then compute the descendants recursively using `descendants_` check_filters(node, scale=scale, symbol=symbol, link=link) @@ -214,10 +214,10 @@ function new_node_MTG(node::Node{N,A}, template::T) where {N<:AbstractNodeMTG,A, end """ - insert_parent!(node, template, attr_fun = node -> typeof(node_attributes(node))(), max_id = [max_id(node)]) - insert_generation!(node, template, attr_fun = node -> typeof(node_attributes(node))(), max_id = [max_id(node)]) - insert_child!(node, template, attr_fun = node -> typeof(node_attributes(node))(), max_id = [max_id(node)]) - insert_sibling!(node, template, attr_fun = node -> typeof(node_attributes(node))(), max_id = [max_id(node)]) + insert_parent!(node, template[, attr_fun[, maxid]]) + insert_generation!(node, template[, attr_fun[, maxid]]) + insert_child!(node, template[, attr_fun[, maxid]]) + insert_sibling!(node, template[, attr_fun[, maxid]]) Insert a node in an MTG as: @@ -237,8 +237,10 @@ Insert a node in an MTG as: - `attr_fun`: A function to compute new attributes based on the filtered node. Must return attribute values of the same type as the one used in other nodes from the MTG (*e.g.* Dict or NamedTuple). If you just need to pass attributes values to a node use `x -> your_values`. -- `max_id::Vector{Int64}`: The maximum id of the nodes in the MTG as a vector of length one. It is incremented in the function, -and use by default the value from [`max_id`](@ref). +- `maxid`: A one-element mutable vector containing the last assigned node id. It is + updated only after a successful insertion. When omitted, columnar MTGs seed it from + all active ids in their shared attribute store; other backends use [`max_id`](@ref) + on the current component. # Examples @@ -283,14 +285,14 @@ function _bind_inserted_columnar!(::Type{ColumnarAttrs}, parent_for_store::Node, return nothing end -function insert_parent!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[max_id(node)]) where {N<:AbstractNodeMTG,A} +function insert_parent!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[_max_assigned_id(node)]) where {N<:AbstractNodeMTG,A} - maxid[1] += 1 + candidate_id = maxid[1] + 1 if isroot(node) new_node = Node( - maxid[1], + candidate_id, nothing, Node{N,A}[node], new_node_MTG(node, template), @@ -312,7 +314,7 @@ function insert_parent!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[ reparent!(node, new_node) else new_node = Node( - maxid[1], + candidate_id, parent(node), Node{N,A}[node], new_node_MTG(node, template), @@ -334,26 +336,33 @@ function insert_parent!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[ reparent!(node, new_node) end + maxid[1] = candidate_id return node end -function insert_child!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[max_id(node)]) where {N<:AbstractNodeMTG,A} +function insert_child!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[_max_assigned_id(node)]) where {N<:AbstractNodeMTG,A} - maxid[1] += 1 + candidate_id = maxid[1] + 1 - addchild!(node, maxid[1], new_node_MTG(node, template), attr_fun(node)) + addchild!( + node, + candidate_id, + new_node_MTG(node, template), + _coerce_insert_attrs(A, copy(attr_fun(node))), + ) + maxid[1] = candidate_id return node end -function insert_sibling!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[max_id(node)]) where {N<:AbstractNodeMTG,A} +function insert_sibling!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[_max_assigned_id(node)]) where {N<:AbstractNodeMTG,A} - maxid[1] += 1 + candidate_id = maxid[1] + 1 new_node = Node( - maxid[1], + candidate_id, parent(node), Vector{Node{N,A}}(), new_node_MTG(node, template), @@ -366,15 +375,16 @@ function insert_sibling!(node::Node{N,A}, template, attr_fun=node -> A(), maxid= push!(children(parent(node)), new_node) _mark_structure_mutation!(parent(node)) + maxid[1] = candidate_id return node end -function insert_generation!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[max_id(node)]) where {N<:AbstractNodeMTG,A} +function insert_generation!(node::Node{N,A}, template, attr_fun=node -> A(), maxid=[_max_assigned_id(node)]) where {N<:AbstractNodeMTG,A} - maxid[1] += 1 + candidate_id = maxid[1] + 1 new_node = Node( - maxid[1], + candidate_id, node, children(node), new_node_MTG(node, template), @@ -391,6 +401,7 @@ function insert_generation!(node::Node{N,A}, template, attr_fun=node -> A(), max reparent!(chnode, new_node) end + maxid[1] = candidate_id return node end diff --git a/src/compute_MTG/node_funs.jl b/src/compute_MTG/node_funs.jl index 7d276986..8f61a8a6 100644 --- a/src/compute_MTG/node_funs.jl +++ b/src/compute_MTG/node_funs.jl @@ -83,6 +83,11 @@ end return _store_for_node_attrs(attrs) end +@inline function _max_assigned_id(node::Node) + store = _columnar_store_or_nothing(node) + return store === nothing ? max_id(node) : store.max_node_id +end + function _subtree_has_node_id_conflict(node::Node, store::MTGAttributeStore) seen_ids = Set{Int}() stack = typeof(node)[node] @@ -237,16 +242,19 @@ end """ new_id(mtg) - new_id(mtg, max_id) + new_id(max_id::Int) Make a new unique identifier by incrementing on the maximum node id. -Hint: prefer using `max_id = max_id(mtg)` and then `new_id(mtg, max_is)` for performance -if you do it repeatidely. + +For columnar MTGs, uniqueness is enforced across the complete attribute store, +including detached components that still share it. [`max_id`](@ref) remains a +component traversal and can therefore be lower than `new_id(mtg) - 1` in that +case. """ function new_id(max_id::Int) max_id + 1 end function new_id(mtg::Node) - new_id(max_id(mtg)) + new_id(_max_assigned_id(mtg)) end diff --git a/src/types/Attributes.jl b/src/types/Attributes.jl index 1dc98751..6f284966 100644 --- a/src/types/Attributes.jl +++ b/src/types/Attributes.jl @@ -49,6 +49,7 @@ mutable struct MTGAttributeStore node_bucket::Vector{Int} node_row::Vector{Int} subtree_index::SubtreeIndexCache + max_node_id::Int end struct ColumnarQueryPlan @@ -58,7 +59,37 @@ struct ColumnarQueryPlan end function MTGAttributeStore() - MTGAttributeStore(Dict{Symbol,Int}(), SymbolBucket[], Int[], Int[], SubtreeIndexCache()) + MTGAttributeStore( + Dict{Symbol,Int}(), + SymbolBucket[], + Int[], + Int[], + SubtreeIndexCache(), + 0, + ) +end + +function MTGAttributeStore( + symbol_to_bucket, + buckets, + node_bucket, + node_row, + subtree_index, +) + store = MTGAttributeStore( + convert(Dict{Symbol,Int}, symbol_to_bucket), + convert(Vector{SymbolBucket}, buckets), + convert(Vector{Int}, node_bucket), + convert(Vector{Int}, node_row), + convert(SubtreeIndexCache, subtree_index), + 0, + ) + max_node_id = 0 + @inbounds for node_id in eachindex(store.node_bucket) + store.node_bucket[node_id] == 0 || (max_node_id = node_id) + end + store.max_node_id = max_node_id + return store end @inline function _validate_descendants_strategy(strategy::Symbol) @@ -214,6 +245,27 @@ end @inline _isbound(attrs::ColumnarAttrs) = attrs.ref.store !== nothing && attrs.ref.node_id > 0 +function _assert_columnar_attrs_unbound(attrs::ColumnarAttrs) + _isbound(attrs) || return nothing + throw(ArgumentError( + "ColumnarAttrs is already bound to node id $(attrs.ref.node_id); " * + "pass `copy(attrs)` to a Node constructor to copy its current values.", + )) +end + +@inline function _node_id_is_active(store::MTGAttributeStore, node_id::Int) + 1 <= node_id <= length(store.node_bucket) || return false + @inbounds return store.node_bucket[node_id] != 0 +end + +function _assert_node_id_available(store::MTGAttributeStore, node_id::Int) + node_id > 0 || throw(ArgumentError("node id must be positive; got $(node_id)")) + _node_id_is_active(store, node_id) && throw(ArgumentError( + "node id $(node_id) is already registered in the destination attribute store", + )) + return nothing +end + @inline function _ensure_node_capacity!(store::MTGAttributeStore, node_id::Int) if node_id > length(store.node_bucket) old = length(store.node_bucket) @@ -372,6 +424,7 @@ end end function _add_node_with_attrs!(store::MTGAttributeStore, node_id::Int, symbol::Symbol, attrs::AbstractDict) + _assert_node_id_available(store, node_id) _mark_subtree_index_mutation!(store) _ensure_node_capacity!(store, node_id) bid = _get_or_create_bucket!(store, symbol) @@ -387,6 +440,7 @@ function _add_node_with_attrs!(store::MTGAttributeStore, node_id::Int, symbol::S store.node_bucket[node_id] = bid store.node_row[node_id] = row + store.max_node_id = max(store.max_node_id, node_id) for (k, v) in attrs _set_value_bound!(bucket, row, _normalize_attr_key(k), v) @@ -396,10 +450,10 @@ function _add_node_with_attrs!(store::MTGAttributeStore, node_id::Int, symbol::S end function _remove_node!(store::MTGAttributeStore, node_id::Int) - _mark_subtree_index_mutation!(store) node_id > length(store.node_bucket) && return nothing bid = store.node_bucket[node_id] bid == 0 && return nothing + _mark_subtree_index_mutation!(store) bucket = store.buckets[bid] row = bucket.node_to_row[node_id] @@ -431,6 +485,13 @@ function _remove_node!(store::MTGAttributeStore, node_id::Int) end store.node_bucket[node_id] = 0 store.node_row[node_id] = 0 + if node_id == store.max_node_id + next_max = node_id - 1 + while next_max > 0 && !_node_id_is_active(store, next_max) + next_max -= 1 + end + store.max_node_id = next_max + end return nothing end @@ -552,7 +613,7 @@ function infer_columnar_attr_type(node, key::Symbol, symbol_filter, ignore_nothi end function init_columnar_root!(attrs::ColumnarAttrs, node_id::Int, symbol::Symbol) - _isbound(attrs) && return attrs + _assert_columnar_attrs_unbound(attrs) store = MTGAttributeStore() _add_node_with_attrs!(store, node_id, symbol, attrs.staged) attrs.ref.store = store @@ -562,13 +623,10 @@ function init_columnar_root!(attrs::ColumnarAttrs, node_id::Int, symbol::Symbol) end function bind_columnar_child!(parent_attrs::ColumnarAttrs, child_attrs::ColumnarAttrs, node_id::Int, symbol::Symbol) - if _isbound(child_attrs) - child_attrs.ref.node_id = node_id - return child_attrs - end - + _assert_columnar_attrs_unbound(child_attrs) store = _store_for_node_attrs(parent_attrs) store === nothing && error("Parent node is not attached to a columnar attribute store.") + _assert_node_id_available(store, node_id) _add_node_with_attrs!(store, node_id, symbol, child_attrs.staged) child_attrs.ref.store = store child_attrs.ref.node_id = node_id diff --git a/src/types/Node.jl b/src/types/Node.jl index 0c004ae3..a92dea67 100644 --- a/src/types/Node.jl +++ b/src/types/Node.jl @@ -103,6 +103,7 @@ end @deprecate Node(name::String, id::Int, parent::Node, MTG::M, attributes::T) where {M<:AbstractNodeMTG,T<:MutableNamedTuple} Node(id, parent, MTG, attributes) function Node(id::Int, MTG::T, attributes::ColumnarAttrs) where {T<:AbstractNodeMTG} + _assert_columnar_attrs_unbound(attributes) node = Node{T,ColumnarAttrs}( id, nothing, Vector{Node{T,ColumnarAttrs}}(), MTG, attributes, nothing ) @@ -136,6 +137,12 @@ function _to_columnar_attrs(attributes) end function Node(id::Int, parent::Node{M,ColumnarAttrs}, MTG::M, attributes::ColumnarAttrs) where {M<:AbstractNodeMTG} + parent_attrs = node_attributes(parent) + parent_store = _store_for_node_attrs(parent_attrs) + parent_store === nothing && + error("Parent node is not attached to a columnar attribute store.") + _assert_columnar_attrs_unbound(attributes) + _assert_node_id_available(parent_store, id) node = Node{M,ColumnarAttrs}( id, parent, Vector{Node{M,ColumnarAttrs}}(), MTG, attributes, nothing ) diff --git a/test/test-caching.jl b/test/test-caching.jl index 0c65430b..7f4ac252 100644 --- a/test/test-caching.jl +++ b/test/test-caching.jl @@ -286,6 +286,25 @@ end @test MultiScaleTreeGraph._maybe_traversal_cache(nonroot_columnar_mtg) === nothing @test :NonrootColumnarAfterCache in get_features(nonroot_columnar_mtg).NAME + id_root = Node(1, MutableNodeMTG(:/, :IdRoot, 1, 0), Dict{Symbol,Any}()) + id_high = addchild!( + id_root, + 100, + MutableNodeMTG(:+, :IdHigh, 1, 1), + Dict{Symbol,Any}(), + ) + reparent!(id_high, nothing) + id_store = MultiScaleTreeGraph._node_store(id_root) + @test MultiScaleTreeGraph._node_store(id_high) === id_store + @test max_id(id_root) == 1 + @test id_store.max_node_id == 100 + @test new_id(id_root) == 101 + id_root_new = Node(id_root, MutableNodeMTG(:+, :IdRootNew, 1, 1)) + id_high_new = Node(id_high, MutableNodeMTG(:+, :IdHighNew, 1, 2)) + @test node_id(id_root_new) == 101 + @test node_id(id_high_new) == 102 + @test id_store.max_node_id == 102 + source_tree = Node( 100, MutableNodeMTG(:/, :SourceRoot, 1, 0), @@ -322,6 +341,8 @@ end ) source_store = MultiScaleTreeGraph._node_store(source_tree) target_store = MultiScaleTreeGraph._node_store(target_tree) + @test source_store.max_node_id == 103 + @test target_store.max_node_id == 2 cache_nodes!(source_tree) cache_nodes!(moved_subtree) cache_nodes!(moved_descendant) @@ -338,6 +359,8 @@ end @test MultiScaleTreeGraph._node_store(moved_descendant) === target_store @test source_store.node_bucket[node_id(moved_subtree)] == 0 @test source_store.node_bucket[node_id(moved_descendant)] == 0 + @test source_store.max_node_id == 101 + @test target_store.max_node_id == 103 @test source_store.subtree_index.traversal_cache_nodes === nothing cache_nodes!(target_tree) addchild!( @@ -349,6 +372,7 @@ end @test MultiScaleTreeGraph._maybe_traversal_cache(target_tree) === nothing @test MultiScaleTreeGraph._maybe_traversal_cache(moved_descendant) === nothing @test :CrossStoreAfterCache in get_features(target_tree).NAME + @test target_store.max_node_id == 104 conflict_tree = Node( 1, @@ -444,6 +468,8 @@ end Dict{Symbol,Any}(), ) relabelled_target_store = MultiScaleTreeGraph._node_store(relabelled_target) + @test relabelled_source_store.max_node_id == 20 + @test relabelled_target_store.max_node_id == 51 addchild!(relabelled_target_parent, relabelled_source) @@ -464,4 +490,6 @@ end old_id -> relabelled_source_store.node_bucket[old_id] == 0, (20, 1, 2), ) + @test relabelled_source_store.max_node_id == 0 + @test relabelled_target_store.max_node_id == 51 end diff --git a/test/test-columnar.jl b/test/test-columnar.jl index e2a8fa5d..c468e697 100644 --- a/test/test-columnar.jl +++ b/test/test-columnar.jl @@ -167,6 +167,7 @@ end unified_store = MultiScaleTreeGraph._node_store(root) @test unified_store !== root_store + @test unified_store.max_node_id == 101 @test all( MultiScaleTreeGraph._node_store(node) === unified_store for node in traverse(root, identity, type=typeof(root)) @@ -195,6 +196,7 @@ end @test !haskey(new_leaf, :retained_absent_column) @test !haskey(new_leaf, :source_default) @test new_bud[:dormancy] === 1.5 + @test unified_store.max_node_id == 103 end @testset "Sparse columnar attributes survive growth, merge, and MTG I/O" begin @@ -239,6 +241,93 @@ end @test node_id(copied) == node_id(root) end +@testset "Columnar stores track active IDs and reject aliases atomically" begin + root = Node(10, NodeMTG(:/, :Plant, 1, 1), (root_value=10,)) + low_child = Node(2, root, NodeMTG(:/, :Leaf, 1, 2), (value=2,)) + high_child = Node(20, root, NodeMTG(:+, :Leaf, 2, 2), (value=20,)) + store = MultiScaleTreeGraph._node_store(root) + + @test store.max_node_id == 20 + + legacy_root = Node(10, NodeMTG(:/, :LegacyPlant, 1, 1), (root_value=10,)) + Node(2, legacy_root, NodeMTG(:/, :LegacyLeaf, 1, 2), (value=2,)) + legacy_high = Node( + 20, + legacy_root, + NodeMTG(:+, :LegacyLeaf, 2, 2), + (value=20,), + ) + legacy_source_store = MultiScaleTreeGraph._node_store(legacy_root) + delete_node!(legacy_high) + @test length(legacy_source_store.node_bucket) == 20 + @test legacy_source_store.max_node_id == 10 + legacy_store = MultiScaleTreeGraph.MTGAttributeStore( + copy(legacy_source_store.symbol_to_bucket), + copy(legacy_source_store.buckets), + copy(legacy_source_store.node_bucket), + copy(legacy_source_store.node_row), + deepcopy(legacy_source_store.subtree_index), + ) + @test legacy_store.max_node_id == 10 + + store_state() = ( + child_ids=[node_id(child) for child in children(root)], + node_bucket=copy(store.node_bucket), + node_row=copy(store.node_row), + max_node_id=store.max_node_id, + index_dirty=store.subtree_index.dirty, + index_built=store.subtree_index.built, + index_mutations=store.subtree_index.mutation_count, + ) + + before_invalid = store_state() + @test_throws ArgumentError Node( + 20, root, NodeMTG(:+, :DuplicateLeaf, 3, 2), (duplicate=true,) + ) + @test store_state() == before_invalid + @test_throws ArgumentError Node( + 0, root, NodeMTG(:+, :InvalidLeaf, 3, 2), (invalid=true,) + ) + @test store_state() == before_invalid + + linked_attrs = node_attributes(low_child) + @test_throws ArgumentError Node( + 30, root, NodeMTG(:+, :AliasedLeaf, 3, 2), linked_attrs + ) + @test_throws ArgumentError Node( + 30, NodeMTG(:/, :AliasedRoot, 1, 1), linked_attrs + ) + @test_throws ArgumentError MultiScaleTreeGraph.bind_columnar_child!( + node_attributes(root), linked_attrs, 30, :AliasedLeaf + ) + @test store_state() == before_invalid + @test linked_attrs.ref.store === store + @test linked_attrs.ref.node_id == node_id(low_child) + @test low_child[:value] == 2 + + copied_child = Node( + 30, + root, + NodeMTG(:+, :CopiedLeaf, 3, 2), + copy(linked_attrs), + ) + @test copied_child[:value] == 2 + @test store.max_node_id == 30 + + copied_root = deepcopy(root) + copied_store = MultiScaleTreeGraph._node_store(copied_root) + @test copied_store !== store + @test copied_store.max_node_id == store.max_node_id == 30 + copied_new = Node(copied_root, NodeMTG(:+, :CopiedNewLeaf, 4, 2)) + @test node_id(copied_new) == 31 + @test copied_store.max_node_id == 31 + @test store.max_node_id == 30 + delete_node!(copied_new) + @test copied_store.max_node_id == 30 + @test store.max_node_id == 30 + @test high_child[:value] == 20 +end + leaf = traverse(mtg, node -> node, symbol=:Leaf, type=typeof(mtg))[1] leaf_width = attribute(leaf, :Width, default=nothing) diff --git a/test/test-delete-prune.jl b/test/test-delete-prune.jl index a9c77883..1e7b6d4a 100644 --- a/test/test-delete-prune.jl +++ b/test/test-delete-prune.jl @@ -20,6 +20,28 @@ file = joinpath(dirname(dirname(pathof(MultiScaleTreeGraph))), "test", "files", @test length(new_mtg) == length_start - 1 end +@testset "delete_node! maintains the active maximum node ID" begin + root = Node(1, NodeMTG(:/, :Plant, 1, 1)) + low = Node(2, root, NodeMTG(:/, :Leaf, 1, 2)) + high = Node(10, root, NodeMTG(:+, :Leaf, 2, 2)) + next_high = Node(7, root, NodeMTG(:+, :Leaf, 3, 2)) + store = MultiScaleTreeGraph._node_store(root) + + @test store.max_node_id == 10 + delete_node!(high) + @test store.max_node_id == 7 + @test new_id(root) == 8 + + delete_node!(low) + @test store.max_node_id == 7 + delete_node!(next_high) + @test store.max_node_id == 1 + @test new_id(root) == 2 + + MultiScaleTreeGraph.remove_columnar_node!(node_attributes(root)) + @test store.max_node_id == 0 +end + @testset "delete_node!: delete root node" begin # Delete a node: mtg = read_mtg(file) diff --git a/test/test-insert_node.jl b/test/test-insert_node.jl index 485b8bb1..b60a6ba1 100644 --- a/test/test-insert_node.jl +++ b/test/test-insert_node.jl @@ -171,3 +171,77 @@ end @test get_node(mtg, 8) |> node_mtg == template @test get_node(mtg, 9) |> node_mtg == template end + +function auto_id_fixture() + root = Node(1, MutableNodeMTG(:/, :Root, 1, 0)) + target = Node(2, root, MutableNodeMTG(:+, :Target, 1, 1)) + detached_high = Node(100, root, MutableNodeMTG(:+, :DetachedHigh, 1, 1)) + reparent!(detached_high, nothing) + return root, target, detached_high +end + +@testset "insertions use the store-wide active maximum ID" begin + template = MutableNodeMTG(:+, :Inserted, 1, 2) + for insert! in (insert_parent!, insert_child!, insert_sibling!, insert_generation!) + root, target, detached_high = auto_id_fixture() + store = MultiScaleTreeGraph._node_store(root) + @test MultiScaleTreeGraph._node_store(detached_high) === store + @test max_id(root) == 2 + @test store.max_node_id == 100 + + insert!(target, template) + + @test get_node(root, 101) !== nothing + @test store.max_node_id == 101 + @test new_id(root) == 102 + end + + root = Node(1, MutableNodeMTG(:/, :Root, 1, 0)) + Node(2, root, MutableNodeMTG(:+, :Target, 1, 1)) + Node(3, root, MutableNodeMTG(:+, :Target, 2, 1)) + detached_high = Node(100, root, MutableNodeMTG(:+, :DetachedHigh, 1, 1)) + reparent!(detached_high, nothing) + shared_attrs = MultiScaleTreeGraph.ColumnarAttrs(Dict{Symbol,Any}(:marker => 42)) + insert_children!(root, template, shared_attrs, symbol=:Target) + first_inserted = get_node(root, 101) + second_inserted = get_node(root, 102) + @test first_inserted !== nothing + @test second_inserted !== nothing + @test node_attributes(first_inserted) !== node_attributes(second_inserted) + @test node_attributes(first_inserted).ref.node_id == 101 + @test node_attributes(second_inserted).ref.node_id == 102 + @test first_inserted[:marker] == 42 + @test second_inserted[:marker] == 42 + @test shared_attrs.ref.store === nothing + @test shared_attrs.ref.node_id == 0 + @test MultiScaleTreeGraph._node_store(root).max_node_id == 102 + + root, target, _ = auto_id_fixture() + store = MultiScaleTreeGraph._node_store(root) + children_before = [node_id(child) for child in children(target)] + node_bucket_before = copy(store.node_bucket) + max_before = store.max_node_id + stale_max = [1] + @test_throws ArgumentError insert_child!( + target, + template, + _ -> typeof(node_attributes(target))(), + stale_max, + ) + @test stale_max == [1] + @test [node_id(child) for child in children(target)] == children_before + @test store.node_bucket == node_bucket_before + @test store.max_node_id == max_before + + failing_max = [100] + @test_throws ErrorException insert_child!( + target, + template, + _ -> error("sentinel attribute failure"), + failing_max, + ) + @test failing_max == [100] + @test [node_id(child) for child in children(target)] == children_before + @test store.node_bucket == node_bucket_before + @test store.max_node_id == max_before +end diff --git a/test/test-read_mtg.jl b/test/test-read_mtg.jl index a4261ac7..904ea3c0 100644 --- a/test/test-read_mtg.jl +++ b/test/test-read_mtg.jl @@ -24,6 +24,8 @@ end @test mtg[:symbols] == ["Scene", "Individual", "Axis", "Internode", "Leaf"] @test node_mtg(mtg) == NodeMTG("/", "Scene", 0, 0) @test typeof(children(mtg)) <: Vector{Node{NodeMTG,MultiScaleTreeGraph.ColumnarAttrs}} + @test MultiScaleTreeGraph._node_store(mtg).max_node_id == max_id(mtg) == 7 + @test new_id(mtg) == 8 leaf_1 = get_node(mtg, 5) @test leaf_1[:Length] == 0.2 From c1525100f5e23c0430e0a133b4f1be9f07b2abba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9mi=20Vezy?= Date: Fri, 28 Aug 2026 13:53:29 +0200 Subject: [PATCH 13/13] update benchmarks to new efficient max node id --- benchmark/README.md | 8 ++++---- benchmark/test-row-mutation-topology-benchmark.jl | 12 ++++++------ benchmark/test/runtests.jl | 6 +++--- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/benchmark/README.md b/benchmark/README.md index 58e91026..caa3254e 100644 --- a/benchmark/README.md +++ b/benchmark/README.md @@ -29,10 +29,10 @@ revision supports row-local absence. Historical schema-wide revisions keep their semantic oracle but are not used as a timing baseline for those different operations. -The `preexisting_auto_id` subgroup deliberately exercises repeated ID-less node -construction. It exposes the historical full-tree `max_id` search separately -from the A1 row-presence cost and can be reused unchanged to validate a future -topology-growth correction. +The `automatic_id` subgroup exercises repeated ID-less node construction. For +columnar MTGs, the next ID comes from the store's cached maximum instead of a +full-tree search. The subgroup keeps the cost of automatic ID allocation visible +separately from explicit-ID construction. The writer gate exercises 40 features at 1,000 and 10,000 nodes. It compares the streaming path with the compatibility materialization path, measures seven alternating diff --git a/benchmark/test-row-mutation-topology-benchmark.jl b/benchmark/test-row-mutation-topology-benchmark.jl index fe9ebf49..c5f7c5b4 100644 --- a/benchmark/test-row-mutation-topology-benchmark.jl +++ b/benchmark/test-row-mutation-topology-benchmark.jl @@ -98,9 +98,9 @@ function build_dense_auto_id(n::Int) leaves = Vector{typeof(root)}() sizehint!(leaves, n) for index in 1:n - # This intentionally exercises the historical ID-less constructor. At - # the revisions used as A1 baselines, it calls max_id(root) for every - # new node and therefore performs a repeated full-tree search. + # Exercise the public ID-less constructor separately from explicit-ID + # construction. Columnar MTGs obtain the next ID from the store's + # cached maximum instead of traversing the tree. leaf = Node( root, MutableNodeMTG(_leaf_link(index), :Leaf, index, 2), @@ -296,9 +296,9 @@ function build_a1_benchmark_suite!(suite::BenchmarkGroup) evals=1, ) - # Kept separate because this is a known pre-existing full-tree-search - # path, not a regression introduced by row-presence storage. - a1["preexisting_auto_id"]["dense"][size_key] = + # Kept separate so the cost of automatic ID allocation remains visible + # independently of explicit-ID construction. + a1["automatic_id"]["dense"][size_key] = @benchmarkable build_dense_auto_id($n) samples=A1_BENCHMARK_SAMPLES evals=1 dense_root = build_dense_explicit(n).root diff --git a/benchmark/test/runtests.jl b/benchmark/test/runtests.jl index 66597e64..79de30db 100644 --- a/benchmark/test/runtests.jl +++ b/benchmark/test/runtests.jl @@ -96,7 +96,7 @@ end expected_groups = Set([ "explicit_cold", "explicit_hot_append", - "preexisting_auto_id", + "automatic_id", "write_mtg", ]) row_local_mutation_supported() && push!(expected_groups, "row_local") @@ -109,8 +109,8 @@ end @test Set(keys(a1["explicit_hot_append"])) == Set(["dense"]) assert_benchmark_size_group(a1["explicit_hot_append"]["dense"]) - @test Set(keys(a1["preexisting_auto_id"])) == Set(["dense"]) - assert_benchmark_size_group(a1["preexisting_auto_id"]["dense"]) + @test Set(keys(a1["automatic_id"])) == Set(["dense"]) + assert_benchmark_size_group(a1["automatic_id"]["dense"]) @test Set(keys(a1["write_mtg"])) == Set(["dense", "sparse"]) assert_benchmark_size_group(a1["write_mtg"]["dense"])