anxkhn opened a new pull request, #4979:
URL: https://github.com/apache/polaris/pull/4979
The `storageLocation` test in `TestContentIdentifier` is meant to verify
that a
`ContentIdentifier` survives the `IndexKey` round-trip: it builds
`identifierFromKey` via
`ContentIdentifier.indexKeyToIdentifier(identifier.toIndexKey())`
and then asserts on it. However the assertion compares the reconstructed
value to
itself:
```java
var identifierFromKey = ContentIdentifier.indexKeyToIdentifier(inKey);
soft.assertThat(identifierFromKey).isEqualTo(identifierFromKey);
```
`identifierFromKey isEqualTo identifierFromKey` is vacuously true, so the
test never
actually exercises `toIndexKey()` / `indexKeyToIdentifier()`. A regression
in that
serialization (for example a changed element separator or a broken split)
would go
undetected while the test stayed green.
This changes the expected value to the original `identifier` so the
assertion checks
the real round-trip:
```java
soft.assertThat(identifierFromKey).isEqualTo(identifier);
```
The corrected assertion passes for every parameterized input. `toIndexKey()`
joins
`elements()` on a NUL byte (`IndexKey.key(String.join("\u0000",
elements()))`) and
`indexKeyToIdentifier()` splits `IndexKey.toString()` back on NUL, so the
elements are
reconstructed exactly; `ContentIdentifier` is value-based
(`@PolarisImmutable` over
`elements()`), so equality holds. None of the test inputs contain a NUL
byte, and the
empty case (`""`, `List.of()`) round-trips to empty elements as well.
No production code changes; this only strengthens an existing test into a
real
regression check.
### There is no related issue
This was found by code inspection, not filed as an issue, so there is
nothing to
link with `Fixes #`. (A `gh` search for `ContentIdentifier` /
`TestContentIdentifier`
found no open issue or competing PR.)
## Checklist (from `.github/pull_request_template.md`)
- [x] Don't disclose security issues! (contact [email protected]) - N/A,
no security content
- [ ] Clearly explained why the changes are needed, or linked related
issues: Fixes #
-> Explained above; there is no related issue to link (found by
inspection).
- [x] Added/updated tests with good coverage, or manually tested (and
explained how)
-> The change itself corrects a test so it provides real coverage;
`:polaris-persistence-nosql-metastore:check` passes (11 cases green).
- [x] Added comments for complex logic -> N/A, one-line assertion change
- [x] Updated `CHANGELOG.md` (if needed) -> Not needed: no user-facing
behavior change (test-only)
- [x] Updated documentation in `site/content/in-dev/unreleased` (if needed)
-> Not needed: test-only
---
## Diff (final, after rebase)
1 file changed, 1 insertion(+), 1 deletion(-)
```diff
diff --git
a/persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestContentIdentifier.java
b/persistence/nosql/persistence/metastore/src/test/java/org/apache/polaris/persistence/nosql/metastore/TestContentIdentifier.java
@@ -42,7 +42,7 @@ public class TestContentIdentifier {
soft.assertThat(identifier.elements()).containsExactlyElementsOf(elements);
var inKey = identifier.toIndexKey();
var identifierFromKey = ContentIdentifier.indexKeyToIdentifier(inKey);
- soft.assertThat(identifierFromKey).isEqualTo(identifierFromKey);
+ soft.assertThat(identifierFromKey).isEqualTo(identifier);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
