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

Remove serialization non C41 constructors from stats #4706

Merged
merged 3 commits into from
Feb 12, 2024

Conversation

robertbindar
Copy link
Contributor

@robertbindar robertbindar commented Feb 8, 2024

With this change in, PR #4685 and others to follow should:

a) remove Subarray::set_stats and respective call from serialization/query.cc b) where Subarray is constructed in serialization/query.cc`, change constructor call with the following code snippet:

auto &stats_data = stats_from_capnp(reader.getStats());
Subarray subarray(array, layout, query_stats, stats_data, dummy_logger, true);

c) The constructor calls parent_stats->create_child(prefix, stats_data); d) When all migrations are done, make Stats::populate_with_data private


TYPE: NO_HISTORY
DESC: Remove serialization non C41 constructors from stats

With this change in, PR #4685 and others to follow should:

a) remove Subarray::set_stats and respective call from serialization/query.cc
b) where Subarray is constructed in serialization/query.cc, change constructor
call with the following code snippet:
 auto &stats_data = stats_from_capnp(reader.getStats());
 Subarray subarray(array, layout, query_stats, stats_data, dummy_logger, true);
c) The constructor calls parent_stats->create_child(prefix, stats_data);
d) When all migrations are done, make Stats::populate_with_data private
@robertbindar robertbindar requested a review from KiterLuc February 8, 2024 15:04
Copy link

This pull request has been linked to Shortcut Story #40316: Remove serialization non C.41 constructors from stats..

Copy link
Contributor

@KiterLuc KiterLuc left a comment

Choose a reason for hiding this comment

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

As discussed offline, let's try to make the stats() accessors of all class return a const Stats& object. Let's also validate that the stats we get from REST are identical before and after this change.

tiledb/sm/query/query.h Outdated Show resolved Hide resolved
tiledb/sm/subarray/subarray_partitioner.h Show resolved Hide resolved
tiledb/sm/subarray/subarray.h Outdated Show resolved Hide resolved
tiledb/sm/query/strategy_base.h Outdated Show resolved Hide resolved
tiledb/sm/stats/global_stats.h Outdated Show resolved Hide resolved
tiledb/sm/stats/stats.h Show resolved Hide resolved
tiledb/sm/stats/stats.h Outdated Show resolved Hide resolved
tiledb/sm/stats/stats.h Outdated Show resolved Hide resolved
tiledb/sm/stats/stats.h Outdated Show resolved Hide resolved
tiledb/sm/stats/stats.h Outdated Show resolved Hide resolved
@robertbindar
Copy link
Contributor Author

Manually tested this in a local REST server and stats are identical before and after the change.
We're stuck with Stats* Query::stats() and Stats* StrategyBase::stats() because due to the current architecture, the stats objects passed from serialization end up calling Stats::create_child which cannot be const.

tiledb/sm/stats/stats.h Show resolved Hide resolved
tiledb/sm/query/strategy_base.h Show resolved Hide resolved
tiledb/sm/query/query.h Show resolved Hide resolved
tiledb/sm/stats/stats.h Show resolved Hide resolved
tiledb/sm/subarray/subarray.h Show resolved Hide resolved
tiledb/sm/subarray/subarray_partitioner.h Show resolved Hide resolved
@KiterLuc KiterLuc merged commit 9b28f75 into dev Feb 12, 2024
64 checks passed
@KiterLuc KiterLuc deleted the rbin/ch40316/stats_serialization branch February 12, 2024 17:31
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