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

Bug: Using session.list() with @EmbeddedId causes bad cache hits #89

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

vnayar
Copy link
Contributor

@vnayar vnayar commented Apr 17, 2024

Currently, when a session is populating its cache, it does so by extracting they "key" of an entity, and comparing it to the "key" as read from database columns. However, this logic only reads a single column, thus, @EmbeddedId keys will falsely match in the cache, resulting in a call to session.list() returning the same cached Entity multiple times instead of reading each row.

While this can be fixed by modifying the getKey method of metadata.EntityInfo, this is not a complete solution, because the cache first calls normalize on the Variant which is used as a key. normalize is not aware of objects, it has special cases for long and int, and everything else uses toString, meaning that all Object instances (unless they implement toString) will have the same cache key. This is resolved by modifying normalize to simply return a Variant containing an object as-is, which relies on opEquals being implemented.

This means that, if an @EmbeddedId property refers to an @Embeddable entity that does NOT implement opEquals, then it will never be considered a cache-hit. This limitation was added to the documentation and README, so that users wishing for the performance benefit of the cache remember to implement opEquals.

  • session.d: Variant normalize(Variant v) is used to form session cache keys, it was modified to support object keys used by @EmbeddedId.
  • metadata.d: EntityInfo.getKey(DataSetReader r, int startColumn) has been renamed to getKeyFromColumns to disambiguate it from another overload. Added logic to read multiple columns for an @EmbeddedId key.
  • annotations.d: This is the only file using CRLF line endings, standardize it.

Currently, when a session is populating its cache, it does so by extracting
they "key" of an entity, and comparing it to the "key" as read from database
columns. However, this logic only reads a single column, thus, `@EmbeddedId`
keys will falsely match in the cache, resulting in a call to `session.list()`
returning the same cached Entity multiple times instead of reading each row.

While this can be fixed by modifying the `getKey` method of `metadata.EntityInfo`,
this is not a complete solution, because the cache first calls `normalize` on the
`Variant` which is used as a key. `normalize` is not aware of objects, it has
special cases for `long` and `int`, and everything else uses `toString`, meaning
that all `Object` instances (unless they implement `toString`) will have the same
cache key. This is resolved by modifying `normalize` to simply return a `Variant`
containing an object as-is, which relies on `opEquals` being implemented.

This means that, if an `@EmbeddedId` property refers to an `@Embeddable` entity
that does NOT implement `opEquals`, then it will never be considered a cache-hit.
This limitation was added to the documentation and README, so that users wishing
for the performance benefit of the cache remember to implement `opEquals`.

- session.d: `Variant normalize(Variant v)` is used to form session cache keys,
  it was modified to support object keys used by `@EmbeddedId`.
- metadata.d: `EntityInfo.getKey(DataSetReader r, int startColumn)` has been
  renamed to `getKeyFromColumns` to disambiguate it from another overload.
  Added logic to read multiple columns for an `@EmbeddedId` key.
- annotations.d: This is the only file using CRLF line endings, standardize it.
@SingingBush SingingBush merged commit 9ce43e6 into buggins:master Apr 19, 2024
41 checks passed
@SingingBush SingingBush added this to the 0.5.0 milestone Apr 19, 2024
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