Skip to content

Fix display for nested/vlen cmpds - #474

Open
mattjala wants to merge 8 commits into
HDFGroup:masterfrom
mattjala:nested_seq_cmpd
Open

Fix display for nested/vlen cmpds#474
mattjala wants to merge 8 commits into
HDFGroup:masterfrom
mattjala:nested_seq_cmpd

Conversation

@mattjala

@mattjala mattjala commented May 20, 2026

Copy link
Copy Markdown
Collaborator

Currently, attempting to open a compound dataset with members containing vlen sequences, or compounds nested inside of vlen sequences, leads to missing data, missing column headers, or failure to open the dataset entirely.

This set of changes introduces a new standard for the display of vlen sequence data, and resolves several related bugs in vlen sequence/compound type handling.

The new practice for variable sequences is to expand into one column per sequence element. Empty sequences expand into a single empty placeholder column. This is a change from #472, which handled variable length sequences by displaying them as strings.

This is more complex and may inhibit readability for dataset with long sequences. However, something of this form is necessary for datasets with complex nested types to be viewable at all within HDFView, so the tradeoff is judged to be worthwhile.

  • filterNonSelectedMembers removes unselected member fields from a compound by checking against the dataset's flat selected-member list. This list only enumerates the dset's top-level leaf members. When called on an inner compound reached via recursion (e.g. when a cmpd was the base type of a vlen sequence), the member fields did not appear in the flat list, and so the inner compound came back empty. Resolved by the introduction of the isTopLevel parameter, which can be specified as false to skip the filter and preserve the inner compound's members.

  • recursiveColumnHeaderSetup walks the dataset's flat list of leaf names and looks up the corresponding top-level member type for each to decide how to render the column. It did this by retrieving the n-th member, where n was current index modulo the size of the member types. This calculation only made sense when each top-level meber had one leaf, and would produce incorrect results for heterogenous shapes. I replaced the calculation with explicit topIdx tracking via a new countLeafNames helper.

  • In H5CompoundDS, a vlen member is read with H5DreadVL using a single-field compound transfer type. The JNI returns each row wrapped in a one-element record, so memberData[r] was an ArrayList containing the data rather than the data itself. This is now unwrapped to expose the data directly.

These changes depend on changes to HDF5's JNI VL reading in HDFGroup/hdf5#6413, and so these won't be able to be merged into the master branch until hdf5#6413 is in a new version of the HDF5 library, and HDFView can be bumped to use HDF5 2.2.0 instead of HDF5 2.0.0.

Resolves #470

@jhendersonHDF

Copy link
Copy Markdown
Collaborator

The new practice for variable sequences is to expand into one column per sequence element. Empty sequences expand into a single empty placeholder column. This is a change from #472, which handled variable length sequences by displaying them as strings.

This is more complex and may inhibit readability for dataset with long sequences. However, something of this form is necessary for datasets with complex nested types to be viewable at all within HDFView, so the tradeoff is judged to be worthwhile.

Just an initial few comments; I think that expanding into one column per variable-length sequence element will create far too many columns to be useful in general. I don't think this is generally necessary either, as the tcompound_complex2.h5 file shows that a compound datatype nested 3 levels down inside a very complex top-level compound datatype can still be displayed fairly reasonably.

Screenshot_2026-06-02_15-10-47

A variable-length sequence string as a member of that compound should be easy to display in a single column as well. One issue that now makes this difficult is that something changed to make the column headers no longer collapsible or selectable, and instead if r/w is enabled the table allows you to try editing them (which shouldn't be possible). The column headers with arrows used to be collapsible by double-clicking, which made reading complex compound types a bit easier.

For editing, one column per sequence element would be convenient, but likely only if the sequence is very short which often isn't the case. The current method of displaying them as a string in brackets isn't particularly nice for editing, but it's been suggested in the past that editing a variable-length sequence could open a separate table for viewing/editing, similar to how object/region reference objects work currently (though those have some issues as well).

I also want to mention that display of fairly arbitrarily-nested compound/vlens inside compounds also used to work previously, which makes me think that the fix for displaying these should generally be a simple bugfix for something that changed several releases ago.

@mattjala

Copy link
Copy Markdown
Collaborator Author

The initial implementation on this branch displayed a vlen seq by expanding it into one column per sequence element, producing an unbounded number of columns for long or deeply-nested sequences.

Every vlen now renders as a single column showing the whole sequence as a bracketed string, recursing into nested compounds. For example, a VLEN<COMPOUND{a, sub:{p,q}}> member shows [{10, {11, 12}}, {20, {21, 22}}].

For a vlen seq that is a member of a compound, the column-index maps treat it as one column and CompoundDataProvider delegates the cell to VlenDataProvider's whole-sequence rendering instead of fetching one element per column offset.

For a top-level vlen-of-compound dataset, the whole sequence is read in one H5DreadVL call, and the datatype is enumerated as a single member, so the dataset dispatches to VlenDataProvider as one column instead of being peeled into per-member columns.

This is more in line with HDFView's historic pattern for displaying nested data.

@mattjala
mattjala marked this pull request as ready for review June 19, 2026 16:02
@mattjala mattjala moved this from To be triaged to In progress in HDFView - TRIAGE & TRACK Aug 11, 2026
@nbagha1 nbagha1 added this to the HDFView 3.x.x milestone Aug 21, 2026
@mattjala mattjala added the Component - HDFView Improvements to the visual interface layer label Aug 21, 2026
@nbagha1 nbagha1 added the HDFG_Internal Internally coded for use by the HDFG label Aug 21, 2026

@jhendersonHDF jhendersonHDF left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

After reviewing this, I think a decent bit of the changes are mostly unnecessary or obsolete after the fixes made in the JNI. We should retest with an HDF5 2.3.0 build and see where remaining issues are at. The only datatype that seems problematic is a vlen of compound, which was just unsupported in 3.1.1 but at least didn't cause *ERROR* like 3.4.1. Compound of vlen of compound displayed fine in 3.1.1. Even a compound of array of compound of vlen of compound OR a compound of vlen of compound of array of compound displayed fine in 3.1.1. Of course "fine" is up to taste since it was nested braces and brackets and could probably be display better in separate tables, but that's really a different issue from this PR.

* Show the editing-disabled notice. Called when the user attempts to edit
* a cell in a dataset whose datatype has no symmetric write path (see
* {@link DataFactoryUtils#isUnsafeForWrite}). Throttled to one dialog per
* second so a held-down key or rapid clicks don't stack popups.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This shouldn't be necessary; the dialog box opened should be a blocking dialog so the user should have to interact with it first before being able to open another.

if (dtype == null)
return false;

if (dtype.isVLEN() && !dtype.isVarStr())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

If I'm reading this correctly it doesn't seem quite right, as HDFView has had limited support for writing variable-length types for a bit now.

*/
public static boolean isUnsafeForWrite(Datatype dtype) { return isUnsafe(dtype, false); }

private static boolean isUnsafe(Datatype dtype, boolean insideCompound)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

For maintainability this really seems like something that should be handled by the individual DataProviderFactory classes instead of here, since that's where the editing logic will be at. And instead of displaying an information dialog each class should throw an UnsupportedOperationException instead. See https://archive.eclipse.org/nattable/releases/1.1.0/apidocs/org/eclipse/nebula/widgets/nattable/data/IDataProvider.html#setDataValue(int,%20int,%20java.lang.Object)

// unexpanded member count. Editing is already disabled for
// these via unsafeForWrite, and the listener's side effects
// (ref preview, etc.) don't apply, so just return.
if (unsafeForWrite)

@jhendersonHDF jhendersonHDF Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand returning here, because this is an event that fires for updating the cell value textarea when selecting a cell. It shouldn't have anything to do with whether a value is safe for writing.

if (baseType != null && baseType.isCompound()) {
theData = compoundTypeIO(ioType, did, spaceIDs, nSelPoints, (H5Datatype)baseType, writeBuf,
globalMemberIndex);
if (ioType == H5File.IO_TYPE.READ) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This change doesn't make sense to me - it seems to just throw out the writing case entirely

if (dsDatatype.isVarStr()) {
if (dsDatatype.isVarStr() ||
(dsDatatype.isArray() && dsDatatype.getDatatypeBase() != null &&
dsDatatype.getDatatypeBase().isVarStr())) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I don't think this should be needed; Arrays of variable-length strings displayed fine in HDFView 3.1.1. They cause a crash in 3.4.1, but that's probably due to the fixes that were needed in the JNI.


if (dsDatatype.isVarStr()) {
if (dsDatatype.isVarStr() ||
(dsDatatype.isArray() && dsDatatype.getDatatypeBase() != null &&

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Same here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component - HDFView Improvements to the visual interface layer HDFG_Internal Internally coded for use by the HDFG

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

Nested compound datatypes report error values on Windows

3 participants