Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress filters on variables with non-fixed-size types. #2716

Merged
merged 4 commits into from
Jun 26, 2023

Conversation

DennisHeimbigner
Copy link
Collaborator

re: Discussion #2554
re: PR #2231
re: Issue #2189

After some discussion, the issue of applying filters on variables
whose type is not fixed size, was resolved as follows:

  1. A call to nc_def_var_filter will ignore such filters, but will issue a log warning.
  2. Loading (from an existing file) a variable whose type is not fixed-size and which has filters, will cause the variable to be suppressed.

This PR enforces those rules.

Misc. Other changes

  • Add a test case to test the vlen change.
  • Make some minor clean-ups in various cmake and automake files.
  • Remove unused test

re: Discussion Unidata#2554
re: PR Unidata#2231
re: Issue Unidata#2189

After some discussion, the issue of applying filters on variables
whose type is not fixed size, was resolved as follows:
1. A call to nc_def_var_filter will ignore such filters, but will issue a log warning.
2. Loading (from an existing file) a variable whose type is not fixed-size and which has filters, will cause the variable to be suppressed.

This PR enforces those rules.

### Misc. Other changes
* Add a test case to test the vlen change.
* Make some minor clean-ups in various cmake and automake files.
* Remove unused test
Copy link
Member

@WardF WardF left a comment

Choose a reason for hiding this comment

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

Do we need to document this change anywhere? Other than that, looks good.

@DennisHeimbigner
Copy link
Collaborator Author

Good point, I should make sure it is mentioned in docs/filters.md

@WardF WardF added this to the 4.9.3 milestone Jun 26, 2023
@WardF
Copy link
Member

WardF commented Jun 26, 2023

Thanks @DennisHeimbigner !

@WardF WardF self-requested a review June 26, 2023 16:28
@WardF WardF merged commit 6b430c9 into Unidata:main Jun 26, 2023
DennisHeimbigner added a commit to DennisHeimbigner/netcdf-c that referenced this pull request Aug 3, 2023
re: PR Unidata#2716).
re: Issue Unidata#2189

The basic change is to make use of the fact that HDF5 automatically suppresses optional filters when an attempt is made to apply them to variable-length typed arrays.
This means that e.g. ncdump or nccopy will properly see meaningful data.
Note that if a filter is defined as HDF5 mandatory, then the corresponding variable will be suppressed and will be invisible to ncdump and nccopy.
This functionality is also propagated to NCZarr.

This PR makes some minor changes to PR Unidata#2716 as follows:
* Move the test for filter X variable-length from dfilter.c down into the dispatch table functions.
* Make all filters for HDF5 optional rather than mandatory so that the built-in HDF5 test for filter X variable-length will be invoked.

The test case for this was expanded to verify that the filters are defined, but suppressed.
@DennisHeimbigner DennisHeimbigner deleted the filtervlen.dmh branch September 27, 2023 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants