Conversation
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. 
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. |
|
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. |
jhendersonHDF
left a comment
There was a problem hiding this comment.
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.
| */ | ||
| public static boolean isUnsafeForWrite(Datatype dtype) { return isUnsafe(dtype, false); } | ||
|
|
||
| private static boolean isUnsafe(Datatype dtype, boolean insideCompound) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Tested this concretely before deciding, rather than going by code reading alone: temporarily made CompoundDataProvider.setDataValue() throw an unconditional UnsupportedOperationException and drove a real edit through NatTable's actual cell editor (SWTBot-driven, via the project's own UI test harness) to see what happens on commit.
Result: NatTable's own command layer (UpdateDataCommandHandler / DataLayer.setDataValueByPosition) catches the exception itself and only logs it:
ERROR UpdateDataCommandHandler - Failed to update value to: 999
java.lang.UnsupportedOperationException: ...
at ...CompoundDataProvider.setDataValue(...)
at org.eclipse.nebula.widgets.nattable.layer.DataLayer.setDataValue(...)
...
commit() then returns normally to the caller. The cell silently reverts to its old value - no dialog, no visible feedback at all, just that one backend log line.
So moving the check fully into the individual DataProviderFactory classes and relying on the thrown exception (replacing the current dialog) would mean a user editing an unsupported cell just sees their edit vanish with no indication why - worse than today's explicit "Editing disabled" dialog.
Given that, I've kept the pre-check + dialog as the primary safeguard rather than replacing it. I'm open to also adding the throw in setDataValue() as a cheap backstop underneath the dialog if that addresses the maintainability concern, but I don't think it should replace the dialog outright.
There was a problem hiding this comment.
I dont see a dialog as being particularly better in this case, especially since there are other unsupported types we dont raise a dialog for at the moment. Either way, all of the checking logic should exist only in the DataProviderFactory classes. That is the primary module concerned with data editing and display and other parts of HDFView shouldn't be concerned about whether data of a particular type can be edited. Putting that logic elsewhere only adds to the chance that someone will forget to remove the checking logic in the future.
As for the fact that editing an unsupported type only logs the issue and silently discards the edit, this is because we dont currently register an error handling type and use the default one. We could use a DialogErrorHandling to issue a dialog, though I'd lean more toward RenderErrorHandling. See https://github.com/eclipse-nattable/nattable/wiki/Editing#error-handling. I think those changes are completely unrelated to this PR though. In fact, I dont think they are needed at all for the types being discussed here. Since updating HDFView to use real backing data for everything instead of strings, if we can display a type, there's no reason we cant also edit that type.
The JNI in HDF5 wants one slot per point, with each slot holding a list of that point's elements. That model came in with HDFGroup/hdf5#2156. The slots are left for the read routines to populate (see HDFGroup/hdf5#6413). Arrays containing variable-length data are now allocated as documented, and arrays of fixed-length data have the flat layout the plain read path fills. Detection of variable-length data now recurses the way the JNI's own does. I removed the special case that routed arrays of vlen strings through the VLStrings entry points, since those choose between two different return shapes at runtime and the ordinary path is correct once handed the right buffer. Providers now say whether a cell can be written before an editor opens, since an exception thrown while committing an edit escapes without the table reporting it, and leaves the editor stuck open. New tests cover the datatypes this work is about, several of which had none before. They build their files at test time under the build directory. One test is disabled for now. A vlen of fixed-length strings reads each element to the first null byte rather than for its declared width, so elements run into each other and past the end of the buffer. That seems to be in HDF5's JNI rather than here.
20b1c71 to
3a3f0bb
Compare
| * | ||
| * @return true when the cell can be edited | ||
| */ | ||
| public boolean isCellEditable(int columnIndex, int rowIndex) { return true; } |
There was a problem hiding this comment.
This is fine as an abstraction for now, but I don't really see this as being the responsibility of the DataProvider interface to determine. DataProvider only updates in-memory structures and doesn't actually map anything back to storage directly, the object library and JNI do. This is a determination that (at least currently) should be able to be made much earlier on, pretty much at the time the TableView is constructed.
| * Return this array's elements for one selected point, or null when the buffer is | ||
| * a flat run of base-type values rather than a List per point. | ||
| */ | ||
| private Object[] retrieveObjectModelElements(Object objBuf, int pointIndex) |
There was a problem hiding this comment.
What is this function used for? It seems to be special casing data retrieval for some type that I'd expect to be covered by one of the "retrieve" methods below or by a new one if this is specific to vlen types. For example, retrieveArrayOfAtomicElements should already be covering opaque, reference and bitfield elements, so the comment below makes me suspicious that this is being modeled at the wrong abstraction level.
There was a problem hiding this comment.
It reads arrays that contain variable length data. Those are now allocated as Object[numPoints] with an ArrayList of elements in each slot. The other retrieve methods index into flat arrays for non-variable length data.
The comment below was wrong - bitfields don't arrive as raw bytes, and the other types shouldn't take this path.
|
|
||
| // The bracketed cell text is only unambiguous when the elements are scalars. | ||
| if (!isVarStrBase) | ||
| throw new UnsupportedOperationException( |
There was a problem hiding this comment.
This seems incorrect, or rather a regression, as I believe we had support for this outside of just variable-length strings previously.
There was a problem hiding this comment.
The edit path existed in 3.3.2, but it didn't actually change anything for an array of vlen data. The edited pieces of cell data were passed to setDataValue, which would throw a silently-logged exception and then discard the edit. The cell text is split on ",[]", which works for flat arrays but doesn't necessarily give one token per element for nested text. I think changing here to explicitly disallow trying to edit cells containing data like this until they're properly supported is an improvement.
Supporting editing of arbitrarily types would require a new parser that handles nesting and escapes strings properly. Definitely something we want to do at some point, but probably outside the scope of this PR.
| * routines allocate those lists, so the slots are left null, and the | ||
| * container must be Object[] rather than a narrowly-typed array. | ||
| */ | ||
| if (containsVlenData(dtype)) { |
There was a problem hiding this comment.
This seems like another case of data model mismatch between the object library and JNI. H5Datatype.allocateArray() should be able to correctly handle this case (with some potential updates) and it already has logic for variable-length strings which differs slightly from here (String[] vs Object[]). We should avoid special-casing vlen types as much as possible, especially when there is existing logic in other places.
There was a problem hiding this comment.
Agreed. allocateArray now follow the single rule that, for vlen data, any non-compound type containing it receives Object[numPoints] with the slots left null.
| * column can fall outside the map. Reference detection is best-effort; | ||
| * the label and value field below are updated either way. | ||
| */ | ||
| Integer bIndexObj = baseIndexMap.get(fieldIndex - 1); |
There was a problem hiding this comment.
If this index can fall outside the map now, that seems to imply something was designed wrong with these changes and the range checks are just masking that architectural issue
There was a problem hiding this comment.
These checks replaced an early return that a previous version of this PR added back when vlen members were expanded into one column per element and the lookup could potentially run past the end of the map. I've removed these checks as the case they were guarding against no longer exists.
| * The single-field compound transfer type wraps each row in a | ||
| * one-element record holding the member's value; unwrap it. | ||
| */ | ||
| Object[] rows = (Object[])memberData; |
There was a problem hiding this comment.
Seems like another mismatch between the object library and JNI where this worked previously but was changed on the JNI side
There was a problem hiding this comment.
HDFView reads one compound member at a time through a single-field compound type. Under the buffer data model, HDF5 returns a compound as an ArrayList of its members, so for memers with variable-length data, each row arrives wrapped in a one-element list which this was meant to unwrap. From what I can tell, HDF5 has been returning data like this since HDFGroup/hdf5#2156. Before that, it came back as formatting strings carrying curly braces.
I think it makes sense to keep the unwrap here, at least for now, since removing it would require completely rewriting how compound types are parsed.
That said, as written this code didn't properly unwrap when there are multiple layers of nesting (e.g. a compound with a compound field that then contains variable-length data), and there was a related display bug preventing that data from being shown properly. Both should be fixed now.
There was a problem hiding this comment.
Agreed, this is something that should be fixed, but what's going to be involved in doing so would probably go well beyond the current size of this PR.
7155f9d to
9fd4949
Compare
9fd4949 to
7d0e18d
Compare
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.
filterNonSelectedMembersremoves 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 theisTopLevelparameter, which can be specified as false to skip the filter and preserve the inner compound's members.recursiveColumnHeaderSetupwalks 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 newcountLeafNameshelper.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
Resolves #491