Fix multi-component dynamic-dimension names

Summary:
Explorer incorrectly names multi-component dynamic dimensions. Standalone tables are registered as `name_0`, `name_1`, and so on, but each value column remains `name`; every component also receives the same `src.name` and `dst.name` object-table columns. This makes the standalone schemas inconsistent and prevents the object table from exposing each component under a unique name.

This diff fixes the bug with a file-local [[https://www.internalfb.com/code/fbsource/[bb5ba9591dd9]/fbcode/rebalancer/explorer/cpp_server/lib/LoadModel.cpp?lines=43-54 | `makeScalarDimensionName()`]] helper used by `getDynamicDimensionNames()`, standalone-table registration and construction, and object-column scheduling. Single-component names are unchanged; multi-component tables, value columns, and `src.` and `dst.` columns consistently use `name_index`.

Reviewed By: sud03r

Differential Revision: D116511110

fbshipit-source-id: c3e4b7bdafe4e3b46a1a50c7699bfcfc626c4031

Author: vj-menon

algopt/rebalancer/explorer/cpp_server/lib/LoadModel.cpp

@@ -36,6 +36,18 @@ namespace facebook::rebalancer::explorer {
 using namespace facebook::rebalancer::entities;
 using namespace facebook::rebalancer::interface;
 
+namespace {
+
+std::string makeScalarDimensionName(
+    const std::string& dimensionName,
+    const int scalarCount,
+    const int scalarIndex) {
+  return scalarCount > 1 ? fmt::format("{}_{}", dimensionName, scalarIndex)
+                         : dimensionName;
+}
+
+} // namespace
+
 static EquivalenceSetsData buildEquivalenceSetData(
     const interface::EquivalenceSetInfo& equivSetInfo,
     const Universe& universe) {
@@ -281,9 +293,8 @@ buildStaticObjectDimensionCols(const entities::Universe& universe) {
       if (scalarDimension.isRoutingConfigBased()) {
         continue;
       }
-      auto colName = (objectDimension.size() > 1)
-          ? fmt::format("{}_{}", dimName, i)
-          : dimName;
+      auto colName =
+          makeScalarDimensionName(dimName, objectDimension.size(), i);
 
       if (!scalarDimension.isDynamic()) {
         DataCell defaultDimension(0.0);
@@ -815,9 +826,8 @@ std::vector<std::string> LoadModel::getDynamicDimensionNames(
     for (int i = 0; i < dimension.size(); i++) {
       const ObjectScalarDimension& scalarDimension = dimension.at(i);
       if (scalarDimension.isDynamic()) {
-        auto dimensionName =
-            (dimension.size() > 1) ? fmt::format("{}_{}", dimName, i) : dimName;
-        dynamicDimensionNames.emplace_back(dimensionName);
+        dynamicDimensionNames.emplace_back(
+            makeScalarDimensionName(dimName, dimension.size(), i));
       }
     }
   }
@@ -829,15 +839,15 @@ folly::coro::Task<void> buildDynamicTableAsync(
     const Universe& universe,
     DimensionId dimId,
     int index,
-    std::string dimName,
+    std::string scalarDimName,
     std::shared_ptr<folly::SharedPromise<Table>> promise) {
   try {
     const ObjectDimension& dimension =
         universe.getObjects().getDimension(dimId);
     const ObjectScalarDimension& scalarDimension = dimension.at(index);
     promise->setValue(
         LoadModel::buildDynamicDimensionTable(
-            universe, scalarDimension, dimName));
+            universe, scalarDimension, scalarDimName));
   } catch (...) {
     promise->setException(folly::exception_wrapper(std::current_exception()));
   }
@@ -861,15 +871,16 @@ void LoadModel::initDynamicDimensionTables(
     for (int i = 0; i < dimension.size(); i++) {
       const ObjectScalarDimension& scalarDimension = dimension.at(i);
       if (scalarDimension.isDynamic()) {
-        auto dimensionName =
-            (dimension.size() > 1) ? fmt::format("{}_{}", dimName, i) : dimName;
+        auto scalarDimName =
+            makeScalarDimensionName(dimName, dimension.size(), i);
         auto promise = std::make_shared<folly::SharedPromise<Table>>();
-        tablePromises[dimensionName] = promise;
+        tablePromises[scalarDimName] = promise;
         // Launch the coroutine on the thread pool
         asyncScope.add(
             folly::coro::co_withExecutor(
                 executor,
-                buildDynamicTableAsync(universe, dimId, i, dimName, promise)));
+                buildDynamicTableAsync(
+                    universe, dimId, i, std::move(scalarDimName), promise)));
       }
     }
   }
@@ -882,11 +893,11 @@ buildDynamicObjectDimensionColAsync(
     const Universe& universe,
     DimensionId dimId,
     int index,
-    std::string dimName,
+    std::string scalarDimName,
     const Map<entities::ContainerId, std::vector<entities::ObjectId>>&
         finalAssignment) {
   algopt::treeprof::EventRecorder event("Build dynamic object dimension cols");
-  XLOG(INFO) << "Building object dimension cols for " << dimName;
+  XLOG(INFO) << "Building object dimension cols for " << scalarDimName;
   const algopt::Timer timer(true);
 
   const ObjectDimension& dimension = universe.getObjects().getDimension(dimId);
@@ -900,13 +911,13 @@ buildDynamicObjectDimensionColAsync(
   auto srcColumn = std::make_shared<Column>(
       getObjectDimensionValues(initialAssignment, scalarDimension, universe),
       defaultCell,
-      fmt::format("src.{}", dimName),
+      fmt::format("src.{}", scalarDimName),
       ColumnType::DIMENSION);
 
   auto dstColumn = std::make_shared<Column>(
       getObjectDimensionValues(finalAssignment, scalarDimension, universe),
       defaultCell,
-      fmt::format("dst.{}", dimName),
+      fmt::format("dst.{}", scalarDimName),
       ColumnType::DIMENSION);
 
   // Insert columns into the objects table
@@ -915,7 +926,7 @@ buildDynamicObjectDimensionColAsync(
   result.emplace_back(std::move(srcColumn));
   result.emplace_back(std::move(dstColumn));
 
-  XLOG(INFO) << "Built object dimension cols for " << dimName << " in "
+  XLOG(INFO) << "Built object dimension cols for " << scalarDimName << " in "
              << timer.getSeconds() << " seconds";
   event.stop();
   co_return result;
@@ -942,12 +953,18 @@ buildDynamicObjectDimensionColsAsync(
     for (int i = 0; i < dimension.size(); i++) {
       const ObjectScalarDimension& scalarDimension = dimension.at(i);
       if (scalarDimension.isDynamic()) {
+        auto scalarDimName =
+            makeScalarDimensionName(dimName, dimension.size(), i);
         // Launch the coroutine on the thread pool
         tasks.push_back(
             folly::coro::co_withExecutor(
                 executor,
                 buildDynamicObjectDimensionColAsync(
-                    universe, dimId, i, dimName, finalAssignment)));
+                    universe,
+                    dimId,
+                    i,
+                    std::move(scalarDimName),
+                    finalAssignment)));
       }
     }
   }

algopt/rebalancer/explorer/cpp_server/lib/LoadModel.h

@@ -63,7 +63,6 @@ class LoadModel {
           tablePromises,
       folly::coro::AsyncScope& asyncScope,
       folly::Executor* executor);
-
   static std::vector<std::string> getDynamicDimensionNames(
       const entities::Universe& universe);
 };

algopt/rebalancer/explorer/cpp_server/tests/LoadModelTest.cpp

@@ -171,10 +171,15 @@ TEST(LoadModelTest, Basic) {
       // assert normal object dimension
       EXPECT_DOUBLE_EQ(
           128000, column->getDouble(toEntityId(universe.getObjectId("host2"))));
-    } else if (column->getColumnName() == "network") {
-      // asset network dimension for host3 are added
+    } else if (column->getColumnName() == "network_0") {
+      EXPECT_DOUBLE_EQ(
+          3.0, column->getDouble(toEntityId(universe.getObjectId("host3"))));
+    } else if (column->getColumnName() == "network_1") {
+      EXPECT_DOUBLE_EQ(
+          4.5, column->getDouble(toEntityId(universe.getObjectId("host3"))));
+    } else if (column->getColumnName() == "network_2") {
       EXPECT_DOUBLE_EQ(
-          8.0, column->getDouble(toEntityId(universe.getObjectId("host3"))));
+          1.0, column->getDouble(toEntityId(universe.getObjectId("host3"))));
     } else if (column->getColumnName() == "scheme") {
       // assert partition data for object
       EXPECT_EQ(
@@ -373,6 +378,69 @@ TEST(LoadModelTest, DynamicDimensionTableUsesGroupRowsForCompactStorage) {
   EXPECT_DOUBLE_EQ(7.0, dimensionValues->getDouble(groupRow));
 }
 
+TEST(LoadModelTest, MultiComponentDynamicDimensionsHaveUniqueNames) {
+  auto bundle = TestUtils::buildBundle();
+  auto& dimensions = *bundle.problem()->universe()->objects()->dimensions();
+  entities::thrift::ObjectDimension* dynamicDimension = nullptr;
+  for (auto& entry : dimensions) {
+    auto& dimension = entry.second;
+    if (*dimension.isDynamic()) {
+      dynamicDimension = &dimension;
+      break;
+    }
+  }
+  if (!dynamicDimension) {
+    FAIL() << "Expected the test universe to contain a dynamic dimension";
+  }
+  auto& scalarDimensions = *dynamicDimension->scalarDimensions();
+  ASSERT_EQ(1, scalarDimensions.size());
+  scalarDimensions.push_back(scalarDimensions.front());
+
+  auto explorerModel = LoadModel::buildData(std::move(bundle));
+  addObjectTable(explorerModel);
+  const auto& universe = *explorerModel.universe;
+  auto dynamicDimensionName = explorerModel.dynamicDimensionNames.begin();
+  std::set<std::string> detailColumnNames;
+  for (const auto dimensionId : universe.getObjects().getDimensionIds()) {
+    const auto& dimension = universe.getObjects().getDimension(dimensionId);
+    for (int scalarIndex = 0; scalarIndex < dimension.size(); ++scalarIndex) {
+      if (!dimension.at(scalarIndex).isDynamic()) {
+        continue;
+      }
+      ASSERT_NE(
+          dynamicDimensionName, explorerModel.dynamicDimensionNames.end());
+      const auto table = LoadModel::buildDynamicDimensionTable(
+          universe, dimension.at(scalarIndex), *dynamicDimensionName++);
+      for (const auto& column : table.getColumnData()) {
+        if (column->getColumnType() == ColumnType::DIMENSION) {
+          detailColumnNames.insert(column->getColumnName());
+        }
+      }
+    }
+  }
+  EXPECT_EQ(dynamicDimensionName, explorerModel.dynamicDimensionNames.end());
+
+  const auto expectedDetailColumnNames =
+      makeSet<std::string>({"dynamicLoad_0", "dynamicLoad_1"});
+  EXPECT_EQ(expectedDetailColumnNames, detailColumnNames);
+
+  std::set<std::string> objectColumnNames;
+  for (const auto& columnName :
+       explorerModel.tableData.at(universe.getObjectTypeName())
+           .getColumnNames()) {
+    if (columnName.starts_with("src.dynamicLoad") ||
+        columnName.starts_with("dst.dynamicLoad")) {
+      objectColumnNames.insert(columnName);
+    }
+  }
+  const auto expectedObjectColumnNames = makeSet<std::string>(
+      {"src.dynamicLoad_0",
+       "dst.dynamicLoad_0",
+       "src.dynamicLoad_1",
+       "dst.dynamicLoad_1"});
+  EXPECT_EQ(expectedObjectColumnNames, objectColumnNames);
+}
+
 TEST(ModelTest, MoveGroupTogether) {
   // Build the problem.
   UniverseProblemBuilder builder(
@@ -622,10 +690,15 @@ TEST(LoadModelTest, BasicWithNoSolutionObject) {
     } else if (column->getColumnName() == "dst.dynamicLoad") {
       EXPECT_DOUBLE_EQ(
           1.0, column->getDouble(toEntityId(universe.getObjectId("host0"))));
-    } else if (column->getColumnName() == "network") {
-      // assert network dimension for host3 are added
+    } else if (column->getColumnName() == "network_0") {
+      EXPECT_DOUBLE_EQ(
+          3.0, column->getDouble(toEntityId(universe.getObjectId("host3"))));
+    } else if (column->getColumnName() == "network_1") {
+      EXPECT_DOUBLE_EQ(
+          4.5, column->getDouble(toEntityId(universe.getObjectId("host3"))));
+    } else if (column->getColumnName() == "network_2") {
       EXPECT_DOUBLE_EQ(
-          8.0, column->getDouble(toEntityId(universe.getObjectId("host3"))));
+          1.0, column->getDouble(toEntityId(universe.getObjectId("host3"))));
     } else if (column->getColumnName() == kEquivSetPartition.data()) {
       const auto allObjectIds = universe.getObjects().getObjectIds();
       std::set<std::string> equivSetNames;