From bf8215e1e5ffd35c5f21e810560e467e2d399b6a Mon Sep 17 00:00:00 2001 From: Ypatia Tsavliri Date: Thu, 14 Nov 2024 17:03:58 +0200 Subject: [PATCH] Revert "Do not pass back RTrees to clients unless it's needed (#5265)" This reverts commit a14f9e14e745d6defb40001c4a2717d6926fb0ed. --- tiledb/sm/serialization/array.cc | 3 ++ tiledb/sm/serialization/fragment_info.cc | 4 +-- tiledb/sm/serialization/fragment_metadata.cc | 38 ++++++++------------ tiledb/sm/serialization/fragment_metadata.h | 9 ----- tiledb/sm/serialization/query.cc | 36 ++++++------------- 5 files changed, 30 insertions(+), 60 deletions(-) diff --git a/tiledb/sm/serialization/array.cc b/tiledb/sm/serialization/array.cc index d05ee46d960..1958b8406ce 100644 --- a/tiledb/sm/serialization/array.cc +++ b/tiledb/sm/serialization/array.cc @@ -326,6 +326,9 @@ void array_from_capnp( // pass the right schema to deserialize fragment metadata throw_if_not_ok( fragment_metadata_from_capnp(schema, frag_meta_reader, meta)); + if (client_side) { + meta->loaded_metadata()->set_rtree_loaded(); + } fragment_metadata.emplace_back(meta); } array->set_fragment_metadata(std::move(fragment_metadata)); diff --git a/tiledb/sm/serialization/fragment_info.cc b/tiledb/sm/serialization/fragment_info.cc index 55be440d521..96fba72589f 100644 --- a/tiledb/sm/serialization/fragment_info.cc +++ b/tiledb/sm/serialization/fragment_info.cc @@ -242,6 +242,8 @@ single_fragment_info_from_capnp( meta->non_empty_domain(), expanded_non_empty_domain, meta}; + // This is needed so that we don't try to load rtee from disk + single_frag_info.meta()->loaded_metadata()->set_rtree_loaded(); return {Status::Ok(), single_frag_info}; } @@ -257,8 +259,6 @@ Status single_fragment_info_to_capnp( auto frag_meta_builder = single_frag_info_builder->initMeta(); RETURN_NOT_OK( fragment_metadata_to_capnp(*single_frag_info.meta(), &frag_meta_builder)); - rtree_to_capnp( - single_frag_info.meta()->loaded_metadata()->rtree(), &frag_meta_builder); // set fragment size single_frag_info_builder->setFragmentSize(single_frag_info.fragment_size()); diff --git a/tiledb/sm/serialization/fragment_metadata.cc b/tiledb/sm/serialization/fragment_metadata.cc index 7328cb77668..bd4ebb3e372 100644 --- a/tiledb/sm/serialization/fragment_metadata.cc +++ b/tiledb/sm/serialization/fragment_metadata.cc @@ -372,8 +372,6 @@ Status fragment_metadata_from_capnp( } frag_meta->last_tile_cell_num() = frag_meta_reader.getLastTileCellNum(); - frag_meta->loaded_metadata()->set_loaded_metadata(loaded_metadata); - if (frag_meta_reader.hasRtree()) { auto data = frag_meta_reader.getRtree(); auto& domain = fragment_array_schema->domain(); @@ -390,8 +388,6 @@ Status fragment_metadata_from_capnp( // deserialize it as well in that way. frag_meta->loaded_metadata()->rtree().deserialize( deserializer, &domain, constants::format_version); - - frag_meta->loaded_metadata()->set_rtree_loaded(); } // It's important to do this here as init_domain depends on some fields @@ -415,6 +411,8 @@ Status fragment_metadata_from_capnp( frag_meta_reader.getGtOffsets(), frag_meta->generic_tile_offsets()); } + frag_meta->loaded_metadata()->set_loaded_metadata(loaded_metadata); + return Status::Ok(); } @@ -537,25 +535,6 @@ void fragment_meta_sizes_offsets_to_capnp( } } } - - rtree_to_capnp(frag_meta.loaded_metadata()->rtree(), frag_meta_builder); -} - -void rtree_to_capnp( - const RTree& rtree, capnp::FragmentMetadata::Builder* frag_meta_builder) { - // TODO: Can this be done better? Does this make a lot of copies? - SizeComputationSerializer size_computation_serializer; - rtree.serialize(size_computation_serializer); - if (size_computation_serializer.size() != 0) { - std::vector buff(size_computation_serializer.size()); - Serializer serializer(buff.data(), buff.size()); - rtree.serialize(serializer); - - auto vec = kj::Vector(); - vec.addAll( - kj::ArrayPtr(static_cast(buff.data()), buff.size())); - frag_meta_builder->setRtree(vec.asPtr()); - } } Status fragment_metadata_to_capnp( @@ -710,6 +689,19 @@ Status fragment_metadata_to_capnp( frag_meta.non_empty_domain(), frag_meta.array_schema()->dim_num())); + // TODO: Can this be done better? Does this make a lot of copies? + SizeComputationSerializer size_computation_serializer; + frag_meta.loaded_metadata()->rtree().serialize(size_computation_serializer); + + std::vector buff(size_computation_serializer.size()); + Serializer serializer(buff.data(), buff.size()); + frag_meta.loaded_metadata()->rtree().serialize(serializer); + + auto vec = kj::Vector(); + vec.addAll( + kj::ArrayPtr(static_cast(buff.data()), buff.size())); + frag_meta_builder->setRtree(vec.asPtr()); + auto gt_offsets_builder = frag_meta_builder->initGtOffsets(); generic_tile_offsets_to_capnp( frag_meta.generic_tile_offsets(), gt_offsets_builder); diff --git a/tiledb/sm/serialization/fragment_metadata.h b/tiledb/sm/serialization/fragment_metadata.h index 6741c83f01e..e570bc7fba7 100644 --- a/tiledb/sm/serialization/fragment_metadata.h +++ b/tiledb/sm/serialization/fragment_metadata.h @@ -84,15 +84,6 @@ void fragment_meta_sizes_offsets_to_capnp( const FragmentMetadata& frag_meta, capnp::FragmentMetadata::Builder* frag_meta_builder); -/** - * Serializes FragmentMetadata's RTree to Cap'n Proto message - * - * @param rtree RTREE to serialize - * @param frag_meta_builder cap'n proto class - */ -void rtree_to_capnp( - const RTree& rtree, capnp::FragmentMetadata::Builder* frag_meta_builder); - /** * Convert Fragment Metadata to Cap'n Proto message * diff --git a/tiledb/sm/serialization/query.cc b/tiledb/sm/serialization/query.cc index 4def65c3820..a56e22ba6b0 100644 --- a/tiledb/sm/serialization/query.cc +++ b/tiledb/sm/serialization/query.cc @@ -621,8 +621,7 @@ Status read_state_from_capnp( const capnp::ReadState::Reader& read_state_reader, Query* query, Reader* reader, - ThreadPool* compute_tp, - bool client_side) { + ThreadPool* compute_tp) { auto read_state = reader->read_state(); read_state->overflowed_ = read_state_reader.getOverflowed(); @@ -642,7 +641,7 @@ Status read_state_from_capnp( // If the current partition is unsplittable, this means we need to make // sure the tile_overlap for the current is computed because we won't go // to the next partition - read_state->unsplittable_ && !client_side)); + read_state->unsplittable_)); } return Status::Ok(); @@ -670,8 +669,7 @@ Status dense_read_state_from_capnp( const capnp::ReadState::Reader& read_state_reader, Query* query, DenseReader* reader, - ThreadPool* compute_tp, - bool client_side) { + ThreadPool* compute_tp) { auto read_state = reader->read_state(); read_state->overflowed_ = read_state_reader.getOverflowed(); @@ -691,7 +689,7 @@ Status dense_read_state_from_capnp( // If the current partition is unsplittable, this means we need to make // sure the tile_overlap for the current is computed because we won't go // to the next partition - read_state->unsplittable_ && !client_side)); + read_state->unsplittable_)); } return Status::Ok(); @@ -1099,8 +1097,7 @@ Status reader_from_capnp( const capnp::QueryReader::Reader& reader_reader, Query* query, Reader* reader, - ThreadPool* compute_tp, - bool client_side) { + ThreadPool* compute_tp) { auto array = query->array(); // Layout @@ -1116,12 +1113,7 @@ Status reader_from_capnp( // Read state if (reader_reader.hasReadState()) RETURN_NOT_OK(read_state_from_capnp( - array, - reader_reader.getReadState(), - query, - reader, - compute_tp, - client_side)); + array, reader_reader.getReadState(), query, reader, compute_tp)); // Query condition if (reader_reader.hasCondition()) { @@ -1182,8 +1174,7 @@ Status dense_reader_from_capnp( const capnp::QueryReader::Reader& reader_reader, Query* query, DenseReader* reader, - ThreadPool* compute_tp, - bool client_side) { + ThreadPool* compute_tp) { auto array = query->array(); // Layout @@ -1199,12 +1190,7 @@ Status dense_reader_from_capnp( // Read state if (reader_reader.hasReadState()) RETURN_NOT_OK(dense_read_state_from_capnp( - array, - reader_reader.getReadState(), - query, - reader, - compute_tp, - client_side)); + array, reader_reader.getReadState(), query, reader, compute_tp)); // Query condition if (reader_reader.hasCondition()) { @@ -2175,16 +2161,14 @@ Status query_from_capnp( reader_reader, query, dynamic_cast(query->strategy()), - compute_tp, - context == SerializationContext::CLIENT)); + compute_tp)); } else { auto reader_reader = query_reader.getReader(); RETURN_NOT_OK(reader_from_capnp( reader_reader, query, dynamic_cast(query->strategy()), - compute_tp, - context == SerializationContext::CLIENT)); + compute_tp)); } } else if (query_type == QueryType::WRITE) { auto writer_reader = query_reader.getWriter();