fix(nd-array): make SELECT * dimension auto-selection robust - #312
Merged
Merged
Conversation
The broadcast-compatible default dimension set for SELECT *-style reads
could pick a bookkeeping grid over the real data grid, and could even
select a grid that materialises zero rows.
On an Argo profile file the score (variables retained) tied at 43 between
`N_PROF x N_LEVELS` and `N_HISTORY x N_PROF x DATE_TIME`, because every
scalar/attribute and every N_PROF-only variable counts toward both. The
old tie-break ("higher dimensionality") then picked the history grid -
which, with an empty unlimited `N_HISTORY` dimension, broadcasts the whole
table down to 0 rows.
Rework `default_broadcast_dimensions` to compare candidates by, in order:
1. non-empty before empty (never auto-pick a 0-row grid when a non-empty
alternative exists);
2. most *gridded* data variables retained (scalars/attributes, which
broadcast onto every grid, no longer inflate or tie the score);
3. native dimension set of the most variables;
4. largest data volume (product of dimension sizes), replacing the
arbitrary dimensionality tie-break;
5. first-encountered order, for determinism.
This is shared logic, so netcdf and zarr both benefit. Adds tests for the
native-grid tie-break, the empty-grid guard, the volume tie-break, and
scalar/attribute neutrality.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refines Dataset::default_broadcast_dimensions() in beacon-nd-array to make SELECT * dimension auto-selection more robust for NetCDF/Zarr datasets with multiple incompatible grids, especially avoiding empty (0-row) grids and reducing bias from scalar/attribute arrays.
Changes:
- Updates the default-dimension selection heuristic to prioritize non-empty grids, gridded-variable retention, native-grid prevalence, and grid volume for tie-breaking.
- Excludes scalar/attribute arrays from the scoring function so they don’t inflate counts and force ties.
- Adds/updates unit tests to cover empty-grid avoidance, native-grid tie-breaking, volume tie-breaking, and scalar neutrality.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+152
to
+156
| // down to nothing. `u128` so large grids cannot overflow the product. | ||
| let grid_rows = |c: &[String]| -> u128 { | ||
| c.iter() | ||
| .map(|d| self.dimensions.get(d).copied().unwrap_or(0) as u128) | ||
| .product() |
| // broadcast onto every grid and must not tip the score. The data grid wins | ||
| // on gridded-variable count regardless of how many scalars surround it. | ||
| let scalar = || async { | ||
| NdArray::<f64>::try_new_from_vec_in_mem(vec![1.0], vec![1], vec![] as Vec<String>, None) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
SELECT *over a NetCDF/Zarr file auto-selects a broadcast-compatible default dimension set (added in #310). On an Argo profile file (1900683_prof.nc) it picked the wrong grid:The expected grid is
N_PROF × N_LEVELS— the home of the actual measurement variables (PRES/TEMP/PSAL and their adjusted/QC/error variants).Root cause
The score (number of variables that broadcast onto a grid) tied at 43 between
N_PROF × N_LEVELSandN_HISTORY × N_PROF × DATE_TIME, because every scalar/attribute and everyN_PROF-only variable counts toward both. The tie-break ("higher dimensionality") then picked the 3-dim history grid.Worse: in this file
N_HISTORYis an unlimited dimension that is currently empty (size 0), so the chosen grid broadcasts the whole table down to zero rows —SELECT *would have returned an empty result.Fix
Rework
default_broadcast_dimensionsto compare candidates by, in priority order:<var>.<attr>arrays) broadcast onto every grid, so they no longer inflate or tie the score;grid_rowsaccumulates inu128so large grids cannot overflow the product.This is shared logic in
beacon-nd-array, so both NetCDF and Zarr benefit — both formats route throughresolve_read_dimensionsand build their dataset viaDataset::new, which populates dimension sizes.