fix: DH-21193: Tie location key filter to table key - #8328
Draft
darinpetty wants to merge 4 commits into
Draft
Conversation
Contributor
No docs changes detected for ef0a023 |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds table-aware location filtering and avoids querying underlying services for fully excluded tables.
Changes:
- Binds location filters to table keys.
- Applies filters consistently during enumeration and lookup.
- Adds an empty provider and comprehensive tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
FilteredTableDataService.java |
Adds table-bound filtering and empty-provider optimization. |
NullTableLocationProvider.java |
Implements a provider with no visible locations. |
TestFilteredTableDataService.java |
Tests filtering, binding, and service avoidance. |
darinpetty
changed the title
Split LocationKeyFilter into a bound filter and a provider
A TableLocationKey holds partition values and no table identity, so it
identifies a location only relative to its table. Asking whether such a key
is acceptable without saying which table it belongs to is not a well-formed
question, and a filter whose answer depends on both -- Deephaven Enterprise
claims resolve a single owning claimant per location -- can only answer it by
guessing.
Filtering is now expressed in two steps. LocationKeyFilterProvider.forTable
binds to a table; the returned LocationKeyFilter decides that table's
locations. FilteredTableDataService binds once, where it builds the
TableLocationProvider, so the table half of the decision is made once per
table rather than once per location. A provider that returns
LocationKeyFilter.NONE says the table is entirely excluded, and the service
is then never consulted for it -- which matters because that service is
frequently remote.
An earlier revision kept one interface and made forTable a default returning
`this`. That preserved source compatibility, but it meant a table-aware
filter that forgot to override forTable silently degraded to table-blind --
the exact defect this change exists to remove. Two types make the omission
impossible: a provider has no accept, and a bound filter has no forTable.
This also fixes FilteredTableDataService.getTableLocationKeys(consumer,
filter), which passed the caller's predicate straight through to the input
provider and never applied the service's own filter, so enumeration exposed
locations that hasTableLocationKey, getTableLocationIfPresent, and
subscription delivery all hid.
IMPLICATIONS FOR RELEASE LINES ADOPTING THIS CHANGE
LocationKeyFilter is public and is implemented by every Deephaven Enterprise
release line (each carries its own TableKeyFilter and/or
EnterpriseFilterWrapper). Adoption is a source-breaking change, deliberately
so: it fails at compile time rather than silently answering the wrong
question.
Each adopting line must:
1. Change implementations that decide per table from
`implements LocationKeyFilter` to `implements LocationKeyFilterProvider`,
moving the logic into forTable and deleting accept. If an implementation
previously ignored the table, its provider returns the same filter for
every table.
2. Update FilteredTableDataService construction to pass a provider. A
table-blind call site becomes `tableKey -> existingFilter`.
3. Return LocationKeyFilter.NONE from forTable for a table that can supply
nothing, rather than a filter that rejects every location. Only the
former lets the service skip the underlying (often remote) provider; the
latter is correct but opens a subscription whose every result is
discarded.
4. Drop any interface that existed only to add a table-level question to
LocationKeyFilter -- in Core+ that was TableKeyFilter, whose
accept(TableKey) is now `forTable(tableKey) != LocationKeyFilter.NONE`.
Lambdas are unaffected: every lambda in this codebase was a bound filter, and
LocationKeyFilter remains a functional interface.
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.
No description provided.