Add Zarr v3 OME-Arrow Compatibility Layer via ome_arrow(...)#3
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded an Changes
Sequence DiagramsequenceDiagram
participant User as User Query
participant DuckDB as DuckDB Engine
participant Binder as ome_arrow Binder
participant Store as Zarr v3 Store
participant Parser as Metadata Parser
participant Scanner as ome_arrow Scanner
User->>DuckDB: ome_arrow(path, array_path)
DuckDB->>Binder: Bind function call
Binder->>Store: Validate that store is Zarr v3
alt Valid v3 Store
Binder->>Parser: Read zarr.json and extract dimension_names
Parser->>Parser: Normalize names (trim, lowercase, sanitize, ensure unique/count==rank)
Parser-->>Binder: dimension_names list
Binder->>DuckDB: Return prepared statement with dimension columns
DuckDB->>Scanner: Execute scan
Scanner->>Store: Read array chunks/cells
Scanner->>Scanner: Map dimensions -> columns and emit rows
Scanner-->>DuckDB: Result rows
DuckDB-->>User: Query results
else Invalid / v2 Store
Binder-->>DuckDB: Error: unrecognizable v3 metadata
DuckDB-->>User: Error
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds an ome_arrow(...) table function set that exposes a Zarr v3 / OME-Arrow-friendly view by (a) returning v3 array metadata including dimension_names, and (b) using those dimension names as the relational coordinate column names for cell scans.
Changes:
- Introduces
ome_arrow(path)(v3-only array overview) andome_arrow(path, array_path[, version_override])(v3-only cells scan with dimension-named columns). - Extends v3 metadata parsing to read/normalize
dimension_namesand plumbs them through array entries. - Updates SQL + Python smoke tests and fixtures/docs to cover
ome_arrow(...)and v3dimension_names.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/zarr_metadata.cpp |
Parses/normalizes v3 dimension_names; adds binder/scan paths for ome_arrow overview + dimension-named cell scans. |
src/include/zarr_metadata.hpp |
Exposes GetOmeArrowFunction() in the public header. |
src/duckdb_zarr_extension.cpp |
Registers the new ome_arrow function set with the extension loader. |
test/sql/duckdb_zarr.test |
Adds SQL coverage for v3-only behavior, dimension name reporting, and dimension-named cell output. |
test/python_smoke.py |
Adds a Python-side ome_arrow query and column-name assertion. |
test/data/simple_v3.zarr/*/zarr.json |
Updates v3 fixtures to include dimension_names. |
scripts/create_sample_zarr.py |
Updates sample v3 store generator to emit dimension_names. |
README.md |
Documents new ome_arrow(...) entrypoints and intent/limitations. |
test/README.md |
Notes ome_arrow(...) coverage in the test suite overview. |
ARCHITECTURE.md |
Mentions ome_arrow(...) as a thin convention adapter in the MVP. |
ROADMAP.md |
Marks convention adapters as partially implemented via ome_arrow(...). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return_types.reserve(cell_array.rank + 1); | ||
| for (idx_t dim = 0; dim < NumericCast<idx_t>(cell_array.rank); dim++) { | ||
| return_types.push_back(LogicalType::BIGINT); | ||
| } | ||
| names.push_back("value"); |
There was a problem hiding this comment.
ome_arrow derives output column names from v3 dimension_names and later appends a fixed "value" column. If a store uses a dimension name that normalizes to "value", this will produce duplicate output column names (dimension + value), which can break queries or result binding. Consider rejecting/resolving reserved names (e.g., disallow "value" in NormalizeDimensionNames, or auto-disambiguate by renaming to something like "dim_value" / "value_0").
| return_types.reserve(cell_array.rank + 1); | |
| for (idx_t dim = 0; dim < NumericCast<idx_t>(cell_array.rank); dim++) { | |
| return_types.push_back(LogicalType::BIGINT); | |
| } | |
| names.push_back("value"); | |
| // Ensure that the appended value column name does not collide with any dimension name | |
| unordered_set<string> existing_names(names.begin(), names.end()); | |
| string value_col_name = "value"; | |
| if (existing_names.find(value_col_name) != existing_names.end()) { | |
| // Find a unique variant: value_0, value_1, ... | |
| const string base_name = value_col_name; | |
| idx_t suffix = 0; | |
| do { | |
| value_col_name = StringUtil::Format("%s_%llu", base_name.c_str(), | |
| static_cast<unsigned long long>(suffix)); | |
| suffix++; | |
| } while (existing_names.find(value_col_name) != existing_names.end()); | |
| } | |
| return_types.reserve(cell_array.rank + 1); | |
| for (idx_t dim = 0; dim < NumericCast<idx_t>(cell_array.rank); dim++) { | |
| return_types.push_back(LogicalType::BIGINT); | |
| } | |
| names.push_back(value_col_name); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/zarr_metadata.cpp`:
- Around line 279-283: OptionalStringArray currently returns an empty vector for
both missing/null keys and present-but-empty arrays, which skips the rank
validation in NormalizeDimensionNames; change OptionalStringArray to return an
optional<vector<string>> (or a pair<bool, vector<string>>/out-present flag) so
callers can distinguish "key absent/null" from "present empty array", update
call sites (notably NormalizeDimensionNames) to treat std::nullopt as
"unspecified" (keep old unnamed behavior) but if present and vector.size() !=
rank (including size==0 when rank>0) return/emit the validation error; apply the
same change to the other occurrence around lines 349-355 so both paths validate
array size against rank.
- Around line 2339-2345: names currently unconditionally appends a "value"
column which can collide with existing dimension names from
GetOutputDimensionNames(cell_array); change the logic after names is populated
to check for an existing "value" entry and, if present, pick a non-colliding
name (e.g., "value", "value_1", "value_2", ...) before pushing to names and
return_types so the pushed dtype.logical_type remains aligned; update the code
paths that push to return_types (the vector reserved and the push_back of
dtype.logical_type) to use the chosen unique column name and avoid duplicate
output column names when using GetOutputDimensionNames, cell_array.rank, names,
return_types, and dtype.logical_type.
- Around line 360-364: Assign the return value of StringUtil::Trim to the local
variable so trimming actually happens: replace the discarded call
StringUtil::Trim(name) with name = StringUtil::Trim(name); then the subsequent
name = StringUtil::Lower(name) and the empty check that throws
InvalidInputException for Zarr v3 dimension_names will behave correctly; look
for this logic around the dimension_names loop in zarr_metadata.cpp where
variable name is used.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6107bd5a-aca3-4caa-b902-9f4821ed7b22
⛔ Files ignored due to path filters (3)
test/data/simple_v3.zarr/fortran_v3/zarr.jsonis excluded by!test/data/**/*.zarr/**test/data/simple_v3.zarr/mask_v3/zarr.jsonis excluded by!test/data/**/*.zarr/**test/data/simple_v3.zarr/temperature_v3/zarr.jsonis excluded by!test/data/**/*.zarr/**
📒 Files selected for processing (10)
ARCHITECTURE.mdREADME.mdROADMAP.mdscripts/create_sample_zarr.pysrc/duckdb_zarr_extension.cppsrc/include/zarr_metadata.hppsrc/zarr_metadata.cpptest/README.mdtest/python_smoke.pytest/sql/duckdb_zarr.test
| auto name = dimension_names[i]; | ||
| StringUtil::Trim(name); | ||
| name = StringUtil::Lower(name); | ||
| if (name.empty()) { | ||
| throw InvalidInputException("Zarr v3 dimension_names cannot contain empty names"); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
DuckDB C++ StringUtil::Trimsignature induckdb/common/string_util.hpp — does it return a new string or trim in place?
💡 Result:
The DuckDB C++ StringUtil::Trim function in duckdb/common/string_util.hpp returns a new string rather than trimming in place. DuckDB's string_t type is immutable (as shown in string_type.hpp source code from GitHub result 6), so string utility functions like Trim create and return new string_t instances with trimmed content allocated in a string heap or inlined if small. In-place modification would violate immutability. No source explicitly shows the signature, but design principles confirm it returns new string. Recent PRs mentioning StringUtil (e.g., #14391) use it for enum-string conversions, consistent with functional style (new strings).
Citations:
- 1: https://github.com/duckdb/duckdb/blob/main/src/include/duckdb/common/types/string_type.hpp
- 2: Rework generated EnumUtil code duckdb/duckdb#14391
- 3: reformat string functions duckdb/duckdb#14400
🏁 Script executed:
# First, let's locate the string_util.hpp file in the repository or check if it's available
find . -name "string_util.hpp" -o -name "*string_util*" | head -20Repository: d33bs/duckdb_zarr
Length of output: 43
🏁 Script executed:
# Let's also examine the exact code at lines 360-364 in src/zarr_metadata.cpp
sed -n '355,370p' src/zarr_metadata.cppRepository: d33bs/duckdb_zarr
Length of output: 506
🏁 Script executed:
# Search for StringUtil in the repository - might be in DuckDB headers or local includes
find . -type f -name "*.hpp" -o -name "*.h" | xargs grep -l "class StringUtil\|namespace StringUtil" 2>/dev/null | head -10Repository: d33bs/duckdb_zarr
Length of output: 43
🏁 Script executed:
# Also search for the Trim function definition
rg "StringUtil::Trim|void Trim|string Trim" -A 2 --type cpp --type h | head -40Repository: d33bs/duckdb_zarr
Length of output: 208
🏁 Script executed:
# Check if DuckDB is a submodule or vendored
ls -la | grep -i duckdb
git submodule statusRepository: d33bs/duckdb_zarr
Length of output: 491
🏁 Script executed:
# Look for StringUtil files in the duckdb submodule
find ./duckdb -name "*string_util*" -type f | head -10Repository: d33bs/duckdb_zarr
Length of output: 43
🏁 Script executed:
# Search for Trim and Lower function definitions in duckdb submodule
rg "Trim|Lower" ./duckdb --type cpp --type h -B 2 -A 5 | grep -E "(static.*Trim|static.*Lower|string.*Trim|string.*Lower)" | head -20Repository: d33bs/duckdb_zarr
Length of output: 43
🏁 Script executed:
# Check if duckdb directory is properly populated
ls -la ./duckdb | head -20Repository: d33bs/duckdb_zarr
Length of output: 232
🏁 Script executed:
# Try broader search for StringUtil
find ./duckdb -type f \( -name "*.hpp" -o -name "*.h" \) -exec grep -l "StringUtil" {} \; | head -10Repository: d33bs/duckdb_zarr
Length of output: 43
🏁 Script executed:
# Also check what's in include/duckdb directory if it exists
ls -la ./duckdb/src/include/duckdb/common/ 2>/dev/null | grep -i string | head -10Repository: d33bs/duckdb_zarr
Length of output: 43
Assign the return value of StringUtil::Trim.
The Trim function returns a new string rather than modifying in place. Line 361 calls StringUtil::Trim(name) but discards the result, making the operation a no-op. Whitespace-only dimension names would incorrectly pass the empty check. Change to name = StringUtil::Trim(name); to match the correct pattern used on line 362 with StringUtil::Lower.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/zarr_metadata.cpp` around lines 360 - 364, Assign the return value of
StringUtil::Trim to the local variable so trimming actually happens: replace the
discarded call StringUtil::Trim(name) with name = StringUtil::Trim(name); then
the subsequent name = StringUtil::Lower(name) and the empty check that throws
InvalidInputException for Zarr v3 dimension_names will behave correctly; look
for this logic around the dimension_names loop in zarr_metadata.cpp where
variable name is used.
|
This ended up too specific to non-Zarr work. Closing this. |
Summary by CodeRabbit
New Features
Documentation
Tests
Sample Data