fix(cpp-client): Add comments, fix typos, correct parameter names (#6319

)

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 = {});

cpp-client/deephaven/dhclient/include/public/deephaven/client/client_options.h

@@ -73,32 +73,32 @@ 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);
   /**
    *
    * 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,16 +117,16 @@ 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);
   /**
    * Adds an extra header with a constant name and value to be sent with every outgoing server request.
    *
    * @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);
   /**

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<RowSequence> AddKeys(const RowSequence &rows_to_add_key_space);
 
-  void AddData(const std::vector<std::shared_ptr<ColumnSource>> &begin_index, const std::vector<size_t> &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'.
+   * </summary>
+   * @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<std::shared_ptr<ColumnSource>> &sources, const std::vector<size_t> &begins,
                const std::vector<size_t> &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<RowSequence> Erase(const RowSequence &begin_key);
+  std::shared_ptr<RowSequence> 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<RowSequence> 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<ClientTable> Snapshot() const;
 

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<RowSequence> AddKeys(const RowSequence &begin_key);
+  std::shared_ptr<RowSequence> 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<RowSequence> ConvertKeysToIndices(const RowSequence &begin_key) const;
+  std::shared_ptr<RowSequence> ConvertKeysToIndices(const RowSequence &keys) const;
 
   /**
    * Note: this call iterates over the Roaring64Map and is not constant-time.

cpp-client/deephaven/dhcore/include/public/deephaven/dhcore/clienttable/client_table.h

@@ -75,14 +75,14 @@ class ClientTable {
   [[nodiscard]]
   virtual std::shared_ptr<RowSequence> 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<ColumnSource> 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<ColumnSource> 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.

cpp-client/deephaven/dhcore/src/ticking/barrage_processor.cc

@@ -330,10 +330,10 @@ AwaitingMetadata::ProcessRemoves(const RowSequence &removed_rows) {
   std::shared_ptr<ClientTable> 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<TickingUpdate> 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 {};
     }
   }

cpp-client/deephaven/dhcore/src/ticking/immer_table_state.cc

@@ -97,19 +97,19 @@ std::shared_ptr<RowSequence> ImmerTableState::AddKeys(const RowSequence &rows_to
   return spaceMapper_.AddKeys(rows_to_add_key_space);
 }
 
-void ImmerTableState::AddData(const std::vector<std::shared_ptr<ColumnSource>> &src,
+void ImmerTableState::AddData(const std::vector<std::shared_ptr<ColumnSource>> &sources,
     const std::vector<size_t> &begins, const std::vector<size_t> &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<std::unique_ptr<AbstractFlexVectorBase>>(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<std::shared_ptr<ColumnSource>> &
   rows_to_add_index_space.ForEachInterval(add_chunk);
 }
 
-std::shared_ptr<RowSequence> ImmerTableState::Erase(const RowSequence &rows_to_remove_key_space) {
-  auto result = spaceMapper_.ConvertKeysToIndices(rows_to_remove_key_space);
+std::shared_ptr<RowSequence> 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<RowSequence> 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<RowSequence> 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,18 +182,18 @@ 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;
     uint64_t end = static_cast<uint64_t>(last) + 1;
     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<ClientTable> ImmerTableState::Snapshot() const {