This is an automated email from the ASF dual-hosted git repository. quinnj pushed a commit to branch core-rewrite in repository https://gitbox.apache.org/repos/asf/arrow-julia.git
commit 071f367ed8dcc0667aabf43a92de91056ea4e87a Author: Jacob Quinn <[email protected]> AuthorDate: Wed Aug 19 00:31:25 2026 -0600 fix: resolve round 62 findings — bench command literals, research note, two dead helpers - bench/run.jl: the multi-line backtick command literals are single-line (interpolated locals for the long paths), so a formatter pass cannot change the raw string a `@cmd` macro receives - docs/dev/research-flatbuffers-cdata.md describes the present tree: the writer surface and FlatBuffers gap list without the deleted builder helpers, no line counts, the two malformed reference remnants removed; the C-data comparison states the semantic-tier import validation, the shared-cell `close!` revocation, the 1 MiB C-string bound, imported and exported schema metadata, the existing pyarrow C-data oracle, and the public `release!`/`close!` names - conformance/arrowjson.jl `_u64` and tools/fbsgen.jl `_ident` (both uncalled) removed; the four generated files still regenerate byte-identically after JuliaFormatter - The conformance image's warm list keeps EnumX on purpose: it is a direct Arrow dependency, which is the list's stated rule (690e1cc's message overstated the removal) Co-Authored-By: Claude Fable 5 <[email protected]> --- bench/run.jl | 22 +++++------ conformance/arrowjson.jl | 1 - docs/dev/research-flatbuffers-cdata.md | 68 +++++++++++++++------------------- tools/fbsgen.jl | 2 - 4 files changed, 41 insertions(+), 52 deletions(-) diff --git a/bench/run.jl b/bench/run.jl index 38a68ac..87e21d3 100644 --- a/bench/run.jl +++ b/bench/run.jl @@ -51,23 +51,23 @@ function main(workdir::String) # The 2.x leg resolves from the registry, pinned by env2x's compat: # instantiate it up front so the one-command invocation works from a # clean checkout (no manifest is committed). - run(`$(Base.julia_cmd()) --startup-file=no - --project=$(joinpath(here, "env2x")) - -e "using Pkg; Pkg.instantiate()"`) + env2x = joinpath(here, "env2x") + run( + `$(Base.julia_cmd()) --startup-file=no --project=$env2x -e "using Pkg; Pkg.instantiate()"`, + ) rewriteout = joinpath(workdir, "rewrite.jsonl") + rewritescript = joinpath(here, "bench_rewrite.jl") _runleg( - `$(Base.julia_cmd()) --startup-file=no --project=$repo - $(joinpath(here, "bench_rewrite.jl")) $workdir`, + `$(Base.julia_cmd()) --startup-file=no --project=$repo $rewritescript $workdir`, rewriteout, ) push!(legs, ("rewrite", rewriteout)) out2x = joinpath(workdir, "arrow2x.jsonl") + script2x = joinpath(here, "bench_2x.jl") _runleg( - `$(Base.julia_cmd()) --startup-file=no - --project=$(joinpath(here, "env2x")) - $(joinpath(here, "bench_2x.jl")) $workdir`, + `$(Base.julia_cmd()) --startup-file=no --project=$env2x $script2x $workdir`, out2x, ) push!(legs, ("arrow2x", out2x)) @@ -88,10 +88,10 @@ function main(workdir::String) false end if havedocker + image = "arrow-julia-conformance:latest" + python = "/opt/pyarrow/bin/python" _runleg( - `docker run --rm -v $workdir:/bench -v $here:/src - arrow-julia-conformance:latest - /opt/pyarrow/bin/python /src/bench_pyarrow.py /bench`, + `docker run --rm -v $workdir:/bench -v $here:/src $image $python /src/bench_pyarrow.py /bench`, pyout, ) push!(legs, ("pyarrow", pyout)) diff --git a/conformance/arrowjson.jl b/conformance/arrowjson.jl index 0949078..b1f9ab1 100644 --- a/conformance/arrowjson.jl +++ b/conformance/arrowjson.jl @@ -222,7 +222,6 @@ end _hex(bytes) = uppercase(bytes2hex(bytes)) _unhex(s::AbstractString) = hex2bytes(s) _i64(x) = x isa AbstractString ? parse(Int64, x) : Int64(x) -_u64(x) = x isa AbstractString ? parse(UInt64, x) : UInt64(x) function _validity(col, n::Int) v = get(col, "VALIDITY", nothing) diff --git a/docs/dev/research-flatbuffers-cdata.md b/docs/dev/research-flatbuffers-cdata.md index 24eecad..238bc62 100644 --- a/docs/dev/research-flatbuffers-cdata.md +++ b/docs/dev/research-flatbuffers-cdata.md @@ -29,19 +29,18 @@ compare against `src/cdata.jl`. Research only — no code changed. ### 1.1 What we have -Three files, ~580 lines: `src/FlatBuffers/FlatBuffers.jl` (58), -`builder.jl` (~390), `table.jl` (~120). Provenance: introduced whole in the +Three files, `src/FlatBuffers/FlatBuffers.jl`, `builder.jl`, `table.jl`. +Provenance: introduced whole in the 2020 donation commit (`50e015f` "Pure Julia implementation of apache arrow format") as a fresh, trimmed port of the Go runtime — it was **not** forked from JuliaData/FlatBuffers.jl's code; the two share only the Go-port ancestry. Around it the rewrite adds `src/metadata/VerifierRuntime.jl` -(261 lines, hand-maintained, schema-blind) and `tools/fbsgen.jl` (~660 lines), -which regenerates `src/metadata/{Schema,File,Message,Verifier}.jl`. +(hand-maintained, schema-blind) and `tools/fbsgen.jl`, which regenerates `src/metadata/{Schema,File,Message,Verifier}.jl`. The rewrite's usage surface is narrow. Read side: `getrootas`, `init`, `offset`, `get`, `indirect`, `String`, `Array`, `union`, `vector`/`vectorlen` (via generated getters). Write side: `Builder`, `startobject!`/`endobject!`, -`prependslot!`/`prependoffsetslot!`/`prependstructslot!`, +`prependslot!`/`prependoffsetslot!`, `startvector!`/`endvector!`, `createstring!`, `finish!`, `prep!`, `pad!`, `place!`. The vendored runtime carries only what that surface needs. @@ -49,8 +48,8 @@ The rewrite's usage surface is narrow. Read side: `getrootas`, `init`, Context first: in the rewrite **no generated getter runs on unverified bytes**. `verify_ipc_metadata` stages `verifyrootstart_Message` / -`verifyrootrest_Message` before `FB.getrootas` (`src/ipc_read.jl`, -comment at ), and `verify_footer` does the same for the file footer +`verifyrootrest_Message` before `FB.getrootas` (`src/ipc_read.jl`), and +`verify_footer` does the same for the file footer (`src/ipc_write.jl`). The verifier runtime does checked, byte-assembled loads with range/alignment/domain/budget proofs (`src/metadata/VerifierRuntime.jl`). The list below is therefore what @@ -105,8 +104,6 @@ without the generated verifier in front: fresh `FlatBuffers.Array` → one `unsafe_wrap` array header per property access (`table.jl`). Frequency is per-batch (nodes/buffers/fields), not per-value, so it is visible but not dominant. -- **Always-allocated shared-string Dict.** Every `Builder()` allocates a - `Dict{String,UOffsetT}` (`builder.jl,56`) that Arrow never uses. - Non-issues worth recording: generated tables are concrete immutable structs (no abstract fields); `Builder` fields are concrete; getter returns are `Union{Nothing,T}` **by design** (absent optional field) and the adapters @@ -116,12 +113,10 @@ without the generated verifier in front: Missing vs the FlatBuffers spec: any runtime verifier (Arrow supplies its own generated one), size-prefixed roots (`finishsizeprefixed!` / -size-prefixed `getrootas`), read-side file-identifier check (write-side -`finishwithfileidentifier` exists, unused), public alignment forcing beyond -internal `prep!`, `key`/sorted-vector lookup, nested-flatbuffer helpers, any -object/reflection API. Present but unused by Arrow: shared strings -(`createsharedstring!`, `builder.jl`), vtable deduplication (used). -None of the gaps matter for Arrow's three schemas. +size-prefixed `getrootas`), file identifiers, shared strings, public +alignment forcing beyond internal `prep!`, `key`/sorted-vector lookup, +nested-flatbuffer helpers, any object/reflection API. Vtable deduplication +is present and used. None of the gaps matter for Arrow's three schemas. ### 1.5 Upstream JuliaData/FlatBuffers.jl today @@ -186,7 +181,7 @@ general-purpose surface Arrow does not need. (option iii); do not retrofit FlatBuffers.jl v0.6 (option ii).** - The whole owned surface — runtime + verifier runtime + generator — is - ~1,500 lines, regeneration is mechanical (`tools/fbsgen.jl` exists exactly + small, regeneration is mechanical (`tools/fbsgen.jl` exists exactly because hand-drift was the bug class), and the verifier budgets are security posture the project must control and version with itself. - A dependency on an external FlatBuffers package re-couples the metadata hot @@ -245,8 +240,8 @@ that never trusts consumer-mutated counts, and an explicit (`test/cdata_battery.jl`). Import lifecycle: single `ForeignOwner` per moved tree, atomic exactly-once release with producer-conformance check (release must NULL the release field), declared buffer extents -from the layout registry, then the full three-stage Core -validation (`validate_structural`/`semantic`/`full`). Tests +from the layout registry, then Core's structural and semantic validation +(`validate_semantic`; `validate_full` is the caller's opt-in). Tests include per-ABI struct size/offset gates (64-bit, both 32-bit int64 alignments; `test/cdata_battery.jl`) and a four-thread re-exec stress child (`test/cdata_stress_child.jl`). @@ -263,11 +258,12 @@ Comparison against samtalki's #607 head (the code most likely to merge): the release field; theirs does not. - **Post-release semantics.** Theirs: every `getindex` runs inside `_with_live` — a ReentrantLock acquire per element — so reads after - `release_c_data` throw. Ours: reachability-based validity with documented - spec-UB after explicit `release!`, zero per-read overhead. Their - gate is a real safety-UX win and a real throughput cost; the right review - feedback is to make it optional, and the right 3.0 stance is to consider a - checked/debug import mode rather than an always-on lock. + `release_c_data` throw. Ours: reachability-based validity; explicit + `release!` documents spec-UB afterwards, while `close!` on the import's + owner revokes every region through one shared cell that each raw access + checks (an atomic load, no lock). Their per-element lock is a real + safety-UX win and a real throughput cost; the right review feedback is to + make it optional. - **Misaligned buffers.** Theirs copies misaligned fixed-width buffers into aligned storage (mirroring arrow-rs). Ours stays zero-copy for any alignment because `loadat` falls back to an unaligned load per element @@ -276,17 +272,16 @@ Comparison against samtalki's #607 head (the code most likely to merge): theirs resolves eagerly with a word-wise `_count_nulls`, ours defers to `ArrayData`'s on-demand atomic `nullcount` (`src/ArrowCore.jl`). - **Bounded string imports.** Theirs caps C-string scans at 4096 bytes - (`_unsafe_string_bounded`); our `_import_cstring` is an unbounded - `unsafe_string` (`cdata.jl`). Within the trusted-ABI rule this is - defensible, but the cap converts a missing NUL from a memory scan into a - clean error — cheap to adopt. + (`_unsafe_string_bounded`); ours caps them at 1 MiB, enforced before + every dereference (`CSTRING_SCAN_LIMIT`, `cdata.jl`), so a missing NUL is + a clean refusal rather than a memory scan. - **Schema metadata.** Their import validates the metadata block's bounds. - Ours neither imports (`_import_field` never reads `sch.metadata`) nor - exports it (`metadata = C_NULL`, `cdata.jl`) — a genuine functional gap - to close in the production adapter. + Ours imports it (`_import_cmetadata`, bounds-checked) and exports it + (`_cmetadata!`), in both the C data and C stream directions; the C-data + battery and the pyarrow oracle exercise both. - **Scope.** Ours covers unions, views/list-views, REE, dictionaries, and both stream directions with exception-safe move seams enumerated at each - boundary (); their landed scope (#607) is + boundary; their landed scope (#607) is null+primitive, with breadth and export still drafts and streams absent everywhere. @@ -302,14 +297,11 @@ map and property-test framing. Worth porting (with `Co-authored-by` credit): 1. #606's deterministic malformed-import fuzz corpus and compile-a-C-producer - smoke test; the optional **PyArrow capsule round-trip** — we currently - have no external-implementation integration test for C-data. + smoke test (the PyArrow round-trip exists: `conformance/cdata_oracle.jl` + proves both directions, PyArrow-native memory, slices, and C streams). 2. #594's C `offsetof()` probe alongside our static ABI gates. -3. #607's bounded C-string reads and metadata-bounds validation; schema - `metadata` import/export (our gap, §2.2). -4. Naming convergence is already free: `from_c_data`/`to_c_data` match; keep - `release_c_data`-style user-facing verbs in the facade docs so their users - land softly. +3. Naming: `from_c_data`/`to_c_data` match theirs; the public release verbs + are `release!` and `close!`. Engagement: these are three good-faith contributors who converged on the same wall (2.x internals lack an `ArrayData`-shaped core; five stalled diff --git a/tools/fbsgen.jl b/tools/fbsgen.jl index ee41499..7f2d7e3 100644 --- a/tools/fbsgen.jl +++ b/tools/fbsgen.jl @@ -79,8 +79,6 @@ function _strip_comments(src::String) return String(take!(out)) end -_ident(s) = strip(s) - """ Parse one .fbs source into ordered declarations. Order is preserved: the emitted Julia must define types before their users, and .fbs authors already
