From f62088dc8765e2e126ec70a2ba8e49fdc2a57762 Mon Sep 17 00:00:00 2001 From: Corey Kosak Date: Fri, 1 Nov 2024 20:14:27 -0400 Subject: [PATCH] fix(cpp-client): Add comments, fix typos, correct parameter names (#6319) --- .../include/public/deephaven/client/client.h | 5 +- .../public/deephaven/client/client_options.h | 24 +++---- .../dhcore/ticking/immer_table_state.h | 62 ++++++++++++++++--- .../deephaven/dhcore/ticking/space_mapper.h | 36 ++++++++++- .../dhcore/clienttable/client_table.h | 6 +- .../dhcore/src/ticking/barrage_processor.cc | 6 +- .../dhcore/src/ticking/immer_table_state.cc | 26 ++++---- 7 files changed, 121 insertions(+), 44 deletions(-) diff --git a/cpp-client/deephaven/dhclient/include/public/deephaven/client/client.h b/cpp-client/deephaven/dhclient/include/public/deephaven/client/client.h index 8a00f38ce9d..4fcc3f884e4 100644 --- a/cpp-client/deephaven/dhclient/include/public/deephaven/client/client.h +++ b/cpp-client/deephaven/dhclient/include/public/deephaven/client/client.h @@ -141,6 +141,7 @@ class TableHandleManager { * int64_t nanoseconds, or a string containing an ISO 8601 duration representation. * @param start_time When the table should start ticking, specified as a std::chrono::time_point, * int64_t nanoseconds since the epoch, or a string containing an ISO 8601 time point specifier. + * @param blink_table Whether the table is a blink table * @return The TableHandle of the new table. */ [[nodiscard]] @@ -150,6 +151,7 @@ class TableHandleManager { /** * Creates an input table from an initial table. When key columns are provided, the InputTable * will be keyed, otherwise it will be append-only. + * @param initial_table The initial table * @param columns The set of key columns * @return A TableHandle referencing the new table */ @@ -189,6 +191,7 @@ class TableHandleManager { /** * Execute a script on the server. This assumes that the Client was created with a sessionType corresponding to * the language of the script (typically either "python" or "groovy") and that the code matches that language. + * @param code The script to be run on the server */ void RunScript(std::string code) const; @@ -299,7 +302,7 @@ class Client { * Factory method to Connect to a Deephaven server using the specified options. * @param target A connection string in the format host:port. For example "localhost:10000". * @param options An options object for setting options like authentication and script language. - * @return A Client object conneted to the Deephaven server. + * @return A Client object connected to the Deephaven server. */ [[nodiscard]] static Client Connect(const std::string &target, const ClientOptions &options = {}); diff --git a/cpp-client/deephaven/dhclient/include/public/deephaven/client/client_options.h b/cpp-client/deephaven/dhclient/include/public/deephaven/client/client_options.h index f8207da083c..5f27a2769ff 100644 --- a/cpp-client/deephaven/dhclient/include/public/deephaven/client/client_options.h +++ b/cpp-client/deephaven/dhclient/include/public/deephaven/client/client_options.h @@ -73,7 +73,7 @@ class ClientOptions { * Configure whether to set server connections as TLS * * @param use_tls true if server connections should be TLS/SSL, false for insecure. - * @return *this, to be used for chaining + * @return *this, so that methods can be chained. */ ClientOptions &SetUseTls(bool use_tls); /** @@ -81,24 +81,24 @@ class ClientOptions { * Sets a PEM-encoded certificate root for server connections. The empty string * means use system defaults. * - * @param pem a PEM encoded certificate chain. - * @return *this, to be used for chaining + * @param tls_root_certs a PEM encoded certificate chain. + * @return *this, so that methods can be chained. */ ClientOptions &SetTlsRootCerts(std::string tls_root_certs); /** * Sets a PEM-encoded certificate for the client and use mutual TLS. * The empty string means don't use mutual TLS. * - * @param pem a PEM encoded certificate chain, or empty for no mutual TLS. - * @return *this, to be used for chaining + * @param client_cert_chain a PEM encoded certificate chain, or empty for no mutual TLS. + * @return *this, so that methods can be chained. */ ClientOptions &SetClientCertChain(std::string client_cert_chain); /** * Sets a PEM-encoded private key for the client certificate chain when using * mutual TLS. * - * @param pem a PEM encoded private key. - * @return *this, to be used for chaining + * @param client_private_key a PEM encoded private key. + * @return *this, so that methods can be chained. */ ClientOptions &SetClientPrivateKey(std::string client_private_key); /** @@ -107,8 +107,8 @@ class ClientOptions { * * @example copt.setIntOption("grpc.min_reconnect_backoff_ms", 2000) * @param opt The option key. - * @param val The option valiue. - * @return *this, to be used for chaining + * @param val The option value. + * @return *this, so that methods can be chained. */ ClientOptions &AddIntOption(std::string opt, int val); /** @@ -117,8 +117,8 @@ class ClientOptions { * * @example copt.setStringOption("grpc.target_name_override", "idonthaveadnsforthishost") * @param opt The option key. - * @param val The option valiue. - * @return *this, to be used for chaining + * @param val The option value. + * @return *this, so that methods can be chained. */ ClientOptions &AddStringOption(std::string opt, std::string val); /** @@ -126,7 +126,7 @@ class ClientOptions { * * @param header_name The header name * @param header_value The header value - * @return *this, to be used for chaining + * @return *this, so that methods can be chained. */ ClientOptions &AddExtraHeader(std::string header_name, std::string header_value); /** diff --git a/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/immer_table_state.h b/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/immer_table_state.h index 7f064f171a5..8746465c5e3 100644 --- a/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/immer_table_state.h +++ b/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/immer_table_state.h @@ -32,32 +32,74 @@ class ImmerTableState final { * all at once, or perhaps in slices) to fill in the data. Once the caller is done, the keys * and data will be consistent once again. Note that AddKeys/AddData only support adding new keys. * It is an error to try to re-add any existing key. + * @param rows_to_add_key_space Keys to add, represented in key space + * @return Added keys, represented in index space */ [[nodiscard]] std::shared_ptr AddKeys(const RowSequence &rows_to_add_key_space); - void AddData(const std::vector> &begin_index, const std::vector &end_index, + /** + * For each column i, insert the interval of data [begins[i], ends[i]) taken from the column source + * sources[i], into the table at the index space positions indicated by 'rowsToAddIndexSpace'. + * + * @param sources The ColumnSources + * @param begins The array of start indices (inclusive) for each column + * @param ends The array of end indices (exclusive) for each column + * @param rows_to_add_index_space Index space positions where the data should be inserted + */ + void AddData(const std::vector> &sources, const std::vector &begins, const std::vector &ends, const RowSequence &rows_to_add_index_space); + /** + * Erases the data at the positions in 'rowsToEraseKeySpace'. + * @param rowsToEraseKeySpace The keys, represented in key space, to erase + * @return The keys, represented in index space, that were erased + */ [[nodiscard]] - std::shared_ptr Erase(const RowSequence &begin_key); + std::shared_ptr Erase(const RowSequence &rows_to_erase_key_space); /** - * When the caller wants to modify data in the ImmerTableState, they do it in two steps: - * First, they call ConvertKeysToIndices, which gives them the - * (key space) -> (position space) mapping. Then, the caller calls ModifyData (perhaps all at - * once, perhaps in slices) to fill in the data. Note that ConvertKeysToIndices / ModifyData only support - * modifying rows. It is an error to try to use a key that is not already in the map. + * Converts a RowSequence of keys represented in key space to a RowSequence of keys represented in index space. + * It is an error to try to use a key that is not already in the map. + * @param keys_row_space Keys represented in key space + * @return Keys represented in index space */ [[nodiscard]] std::shared_ptr ConvertKeysToIndices(const RowSequence &keys_row_space) const; - void ModifyData(size_t begin_index, const ColumnSource &end_index, size_t begin, size_t end, - const RowSequence &rows_to_modify); - void ApplyShifts(const RowSequence &start_index, const RowSequence &end_index, + /** + * Modifies column 'col_num' with the contiguous data sourced in 'src' + * at the half-open interval [begin, end), to be stored in the destination + * at the positions indicated by 'rows_to_modify_index_space'. + * @param col_num Index of the column to be modified + * @param src A ColumnSource containing the source data + * @param begin The start of the source range + * @param end One past the end of the source range + * @param rows_to_modify_index_space The positions to be modified in the destination, + * represented in index space. + */ + void ModifyData(size_t col_num, const ColumnSource &src, size_t begin, size_t end, + const RowSequence &rows_to_modify_index_space); + + /** + * Applies shifts to the keys in key space. This does not affect the ordering of the keys, + * nor will it cause keys to overlap with other keys. Logically there is a set of tuples + * (first_key, last_key, dest_key) which is to be interpreted as take all the existing keys + * in the *closed* range [first_key, last_key] and move them to the range starting at dest_key. + * These tuples have been "transposed" into three different RowSequence data structures + * for possible better compression. + * @param first_index The RowSequence containing the begin_keys + * @param last_index The RowSequence containing the end_keys + * @param dest_index The RowSequence containing the dest_indexes + */ + void ApplyShifts(const RowSequence &first_index, const RowSequence &last_index, const RowSequence &dest_index); + /** + * Takes a snapshot of the current table state + * @return A ClientTable representing the current table state + */ [[nodiscard]] std::shared_ptr Snapshot() const; diff --git a/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/space_mapper.h b/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/space_mapper.h index 58c0fcf8513..ff25a2ba581 100644 --- a/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/space_mapper.h +++ b/cpp-client/deephaven/dhcore/include/private/deephaven/dhcore/ticking/space_mapper.h @@ -15,10 +15,40 @@ class SpaceMapper { SpaceMapper(); ~SpaceMapper(); + /** + * Adds the keys in the half-open interval [begin_key, end_key_) to the set. + * The keys must not already exist in the set. If they do, an exception is thrown. + * @param begin_key The first key to insert + * @param end_key One past the last key to insert + * @return The rank of begin_key + */ [[nodiscard]] uint64_t AddRange(uint64_t begin_key, uint64_t end_key); + + + /** + * Removes the keys in the half-open interval [begin_key, end_key_) from the set. + * It is ok if some or all of the keys do not exist in the set. + * @param begin_key The first key to insert + * @param end_key One past the last key to insert + * @return The rank of begin_key, prior to the deletion + */ [[nodiscard]] uint64_t EraseRange(uint64_t begin_key, uint64_t end_key); + + /** + * Delete all the keys that currently exist in the range [begin_key, end_key). + * Call that set of deleted keys K. The cardinality of K might be smaller than + * (end_key - begin_key) because not all keys in that range are expected to be present. + * + * Calculate a new set of keys KNew = {k ∈ K | (k - begin_key + dest_key)} + * and insert this new set of keys into the map. + * + * This has the effect of offsetting all the existing keys by (dest_key - begin_key) + * @param begin_key The start of the range of keys + * @param end_key One past the end of the range of keys + * @param dest_key The start of the target range to move keys to. + */ void ApplyShift(uint64_t begin_key, uint64_t end_key, uint64_t dest_key); /** @@ -32,15 +62,17 @@ class SpaceMapper { * AddKeys called with [1, 2, 200, 201, 400, 401] * SpaceMapper final state is [1, 2, 100, 200, 201, 300, 400, 401] * The returned result is [0, 1, 3, 4, 6, 7] + * @param keys The keys to add (represented in key space) + * @return The added keys (represented in position space) */ [[nodiscard]] - std::shared_ptr AddKeys(const RowSequence &begin_key); + std::shared_ptr AddKeys(const RowSequence &keys); /** * Looks up 'keys' (specified in key space) in the map, and returns the positions (in position * space) of those keys. */ [[nodiscard]] - std::shared_ptr ConvertKeysToIndices(const RowSequence &begin_key) const; + std::shared_ptr ConvertKeysToIndices(const RowSequence &keys) const; /** * Note: this call iterates over the Roaring64Map and is not constant-time. diff --git a/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/clienttable/client_table.h b/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/clienttable/client_table.h index 146b872ccfe..b3e9e54ae49 100644 --- a/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/clienttable/client_table.h +++ b/cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/clienttable/client_table.h @@ -75,14 +75,14 @@ class ClientTable { [[nodiscard]] virtual std::shared_ptr GetRowSequence() const = 0; /** - * Gets a ColumnSource from the clienttable by index. + * Gets a ColumnSource from the ClientTable by index. * @param column_index Must be in the half-open interval [0, NumColumns). */ [[nodiscard]] virtual std::shared_ptr GetColumn(size_t column_index) const = 0; /** - * Gets a ColumnSource from the clienttable by name. 'strict' controls whether the method + * Gets a ColumnSource from the ClientTable by name. 'strict' controls whether the method * must succeed. * @param name The name of the column. * @param strict Whether the method must succeed. @@ -92,7 +92,7 @@ class ClientTable { [[nodiscard]] std::shared_ptr GetColumn(std::string_view name, bool strict) const; /** - * Gets the index of a ColumnSource from the clienttable by name. 'strict' controls whether the method + * Gets the index of a ColumnSource from the ClientTable by name. 'strict' controls whether the method * must succeed. * @param name The name of the column. * @param strict Whether the method must succeed. diff --git a/cpp-client/deephaven/dhcore/src/ticking/barrage_processor.cc b/cpp-client/deephaven/dhcore/src/ticking/barrage_processor.cc index 1820576879b..644f9bc931e 100644 --- a/cpp-client/deephaven/dhcore/src/ticking/barrage_processor.cc +++ b/cpp-client/deephaven/dhcore/src/ticking/barrage_processor.cc @@ -330,10 +330,10 @@ AwaitingMetadata::ProcessRemoves(const RowSequence &removed_rows) { std::shared_ptr after_removes; if (removed_rows.Empty()) { removed_rows_index_space = RowSequence::CreateEmpty(); - after_removes = prev; + after_removes = prev; } else { removed_rows_index_space = table_state_.Erase(removed_rows); - after_removes = table_state_.Snapshot(); + after_removes = table_state_.Snapshot(); } return {std::move(prev), std::move(removed_rows_index_space), std::move(after_removes)}; } @@ -511,7 +511,7 @@ std::optional AwaitingModifies::ProcessNextChunk(BarrageProcessor for (const auto &mr : modified_rows_remaining_) { if (!mr->Empty()) { - // Need more data from caller. + // Return an indication that at least one column is hungry for more data return {}; } } diff --git a/cpp-client/deephaven/dhcore/src/ticking/immer_table_state.cc b/cpp-client/deephaven/dhcore/src/ticking/immer_table_state.cc index f90fe607bc1..f65e7165d7b 100644 --- a/cpp-client/deephaven/dhcore/src/ticking/immer_table_state.cc +++ b/cpp-client/deephaven/dhcore/src/ticking/immer_table_state.cc @@ -97,19 +97,19 @@ std::shared_ptr ImmerTableState::AddKeys(const RowSequence &rows_to return spaceMapper_.AddKeys(rows_to_add_key_space); } -void ImmerTableState::AddData(const std::vector> &src, +void ImmerTableState::AddData(const std::vector> &sources, const std::vector &begins, const std::vector &ends, const RowSequence &rows_to_add_index_space) { - auto ncols = src.size(); + auto ncols = sources.size(); auto nrows = rows_to_add_index_space.Size(); - AssertAllSame(src.size(), begins.size(), ends.size()); - AssertLeq(ncols, flexVectors_.size(), "More columns provided than was expected ({} vs {})"); + AssertAllSame(sources.size(), begins.size(), ends.size()); + AssertLeq(ncols, flexVectors_.size(), "More columns provided than was expected ({} vs {})"); for (size_t i = 0; i != ncols; ++i) { AssertLeq(nrows, ends[i] - begins[i], "Sources contain insufficient data ({} vs {})"); } auto added_data = MakeReservedVector>(ncols); for (size_t i = 0; i != ncols; ++i) { - added_data.push_back(MakeFlexVectorFromColumnSource(*src[i], begins[i], begins[i] + nrows)); + added_data.push_back(MakeFlexVectorFromColumnSource(*sources[i], begins[i], begins[i] + nrows)); } auto add_chunk = [this, &added_data](uint64_t begin_index, uint64_t end_index) { @@ -135,8 +135,8 @@ void ImmerTableState::AddData(const std::vector> & rows_to_add_index_space.ForEachInterval(add_chunk); } -std::shared_ptr ImmerTableState::Erase(const RowSequence &rows_to_remove_key_space) { - auto result = spaceMapper_.ConvertKeysToIndices(rows_to_remove_key_space); +std::shared_ptr ImmerTableState::Erase(const RowSequence &rows_to_erase_key_space) { + auto result = spaceMapper_.ConvertKeysToIndices(rows_to_erase_key_space); auto erase_chunk = [this](uint64_t begin_key, uint64_t end_key) { auto size = end_key - begin_key; @@ -150,7 +150,7 @@ std::shared_ptr ImmerTableState::Erase(const RowSequence &rows_to_r fv->InPlaceAppend(std::move(fv_temp)); } }; - rows_to_remove_key_space.ForEachInterval(erase_chunk); + rows_to_erase_key_space.ForEachInterval(erase_chunk); return result; } @@ -160,8 +160,8 @@ std::shared_ptr ImmerTableState::ConvertKeysToIndices( } void ImmerTableState::ModifyData(size_t col_num, const ColumnSource &src, size_t begin, size_t end, - const RowSequence &rows_to_modify) { - auto nrows = rows_to_modify.Size(); + const RowSequence &rows_to_modify_index_space) { + auto nrows = rows_to_modify_index_space.Size(); auto source_size = end - begin; AssertLeq(nrows, source_size, "Insufficient data in source ({} vs {})"); auto modified_data = MakeFlexVectorFromColumnSource(src, begin, begin + nrows); @@ -182,10 +182,10 @@ void ImmerTableState::ModifyData(size_t col_num, const ColumnSource &src, size_t // Append the residual items back from 'fvTemp'. fv->InPlaceAppend(std::move(fv_temp)); }; - rows_to_modify.ForEachInterval(modify_chunk); + rows_to_modify_index_space.ForEachInterval(modify_chunk); } -void ImmerTableState::ApplyShifts(const RowSequence &start_index, const RowSequence &end_index, +void ImmerTableState::ApplyShifts(const RowSequence &first_index, const RowSequence &last_index, const RowSequence &dest_index) { auto process_shift = [this](int64_t first, int64_t last, int64_t dest) { uint64_t begin = first; @@ -193,7 +193,7 @@ void ImmerTableState::ApplyShifts(const RowSequence &start_index, const RowSeque uint64_t dest_begin = dest; spaceMapper_.ApplyShift(begin, end, dest_begin); }; - ShiftProcessor::ApplyShiftData(start_index, end_index, dest_index, process_shift); + ShiftProcessor::ApplyShiftData(first_index, last_index, dest_index, process_shift); } std::shared_ptr ImmerTableState::Snapshot() const {