Fix defects with query channels #4786
Conversation
… not actually a channel. Added methods in `class Query` for it, and rewrote the channel C API handle class to use it. Correctly initialize the underlying channel object within the query field C API handle class, and return a channel handle with the right lifespan. (This fixes the defect reported.) Reworked test fixture to use `TemporaryLocalDirectory`. Reworked tests for isolation with SECTION. Rewrote the two tests with aggegrates to use correct calling sequence. Changed `ensure_query_is_not_initialized` to take a reference. Changed `tiledb_query_get_default_channel` to return an actual channel handle rather than a stub for one. Reference: https://app.shortcut.com/tiledb-inc/story/39123/tiledb-field-channel-returns-the-same-handle-and-is-prone-to-double-freeing
|
This pull request has been linked to Shortcut Story #39123: tiledb_field_channel returns the same handle and is prone to double freeing.. |
KiterLuc
force-pushed
the
eh/query_channel
branch
from
e5f823f to
74285c6
Compare
| * Responsibility for choosing channel identifiers is the responsibility of | ||
| * `class Query`; this class merely carries the resulting identifier. | ||
| */ | ||
| class QueryChannelActual { |
There was a problem hiding this comment.
Maybe we should rename this to QueryChannel and rename the one above to QueryChannelSerialization?
There was a problem hiding this comment.
This one: class QueryChannel
The previous one: class LegacyQueryAggregates
The existing class isn't really about channels so much as all the aggregates over the default channel. class LegacyQueryAggregatesOverDefault would be more accurate, but far wordier.
There was a problem hiding this comment.
The wordier version might be fine. It's only used in a few places.
tiledb/sm/query/query.h
Outdated
| * without any grouping. | ||
| * - Channel 1: (optional) Simple aggregates, if any exist. | ||
| */ | ||
| inline size_t number_of_actual_channels() { |
There was a problem hiding this comment.
If we do the rename below we might be able to get rid of the word actual?
There was a problem hiding this comment.
Sure. I did it that way to avoid conflating a bunch of renames with the rest of the PR.
New
class QueryChannelto replace the previous class that was not actually a channel. Added methods inclass Queryfor it, and rewrote the channel C API handle class to use it. Correctly initialize the underlying channel object within the query field C API handle class, and return a channel handle with the right lifespan. (This fixes the defect reported.)Reworked test fixture to use
TemporaryLocalDirectory. Reworked tests for isolation with SECTION. Rewrote the two tests with aggegrates to use correct calling sequence.Changed
ensure_query_is_not_initializedto take a reference. Changedtiledb_query_get_default_channelto return an actual channel handle rather than a stub for one.Reference:
https://app.shortcut.com/tiledb-inc/story/39123/tiledb-field-channel-returns-the-same-handle-and-is-prone-to-double-freeing
[sc-39123]
TYPE: BUG
DESC: Fix defects with query channels