From e5ddf7f7e6888b4ebaad5fe921c455c02925ca2d Mon Sep 17 00:00:00 2001
From: Serge Druzkin <sdruzkin@meta.com>
Date: Fri, 10 Jan 2025 09:45:39 -0800
Subject: [PATCH] Skip incompatible feature ordering configuration

Summary: Skip incompatible feature ordering configuration to avoid writer failures in case of mismatch in the schema or flat map configuration.

Differential Revision: D68024263
---
 dwio/nimble/velox/LayoutPlanner.cpp           | 28 +++++++++++--------
 .../nimble/velox/tests/LayoutPlannerTests.cpp | 21 +++-----------
 2 files changed, 20 insertions(+), 29 deletions(-)

diff --git a/dwio/nimble/velox/LayoutPlanner.cpp b/dwio/nimble/velox/LayoutPlanner.cpp
index 21bdf98..ee6ab01 100644
--- a/dwio/nimble/velox/LayoutPlanner.cpp
+++ b/dwio/nimble/velox/LayoutPlanner.cpp
@@ -117,19 +117,23 @@ std::vector<Stream> DefaultLayoutPlanner::getLayout(
   orderedFlatMapOffsets.reserve(flatMapFeatureOrder_.size() * 3);
 
   for (const auto& flatMapFeatures : flatMapFeatureOrder_) {
-    NIMBLE_CHECK(
-        std::get<0>(flatMapFeatures) < root.childrenCount(),
-        fmt::format(
-            "Column ordinal {} for feature ordering is out of range. "
-            "Top-level row has {} columns.",
-            std::get<0>(flatMapFeatures),
-            root.childrenCount()));
+    if (std::get<0>(flatMapFeatures) >= root.childrenCount()) {
+      LOG(WARNING) << fmt::format(
+          "Column ordinal {} for feature ordering is out of range. "
+          "Top-level row has {} columns.",
+          std::get<0>(flatMapFeatures),
+          root.childrenCount());
+      continue;
+    }
+
     auto& column = root.childAt(std::get<0>(flatMapFeatures));
-    NIMBLE_CHECK(
-        column.kind() == Kind::FlatMap,
-        fmt::format(
-            "Column '{}' for feature ordering is not a flat map.",
-            root.nameAt(std::get<0>(flatMapFeatures))));
+
+    if (column.kind() != Kind::FlatMap) {
+      LOG(WARNING) << fmt::format(
+          "Column '{}' for feature ordering is not a flat map.",
+          root.nameAt(std::get<0>(flatMapFeatures)));
+      continue;
+    }
 
     auto& flatMap = column.asFlatMap();
 
diff --git a/dwio/nimble/velox/tests/LayoutPlannerTests.cpp b/dwio/nimble/velox/tests/LayoutPlannerTests.cpp
index f8d73dc..8589c43 100644
--- a/dwio/nimble/velox/tests/LayoutPlannerTests.cpp
+++ b/dwio/nimble/velox/tests/LayoutPlannerTests.cpp
@@ -394,7 +394,7 @@ TEST(DefaultLayoutPlannerTests, NoFeatureReordering) {
   testStreamLayout(rng, planner, std::move(streams), std::move(expected));
 }
 
-TEST(DefaultLayoutPlannerTests, IncompatibleOrdinals) {
+TEST(DefaultLayoutPlannerTests, IncompatibleOrdinalsAreIgnored) {
   nimble::SchemaBuilder builder;
 
   nimble::test::FlatMapChildAdder fm1;
@@ -422,17 +422,10 @@ TEST(DefaultLayoutPlannerTests, IncompatibleOrdinals) {
   nimble::DefaultLayoutPlanner planner{
       [&]() { return builder.getRoot(); },
       {{{1, {3, 42, 9, 2, 21}}, {2, {3, 2, 42, 21}}}}};
-  try {
-    planner.getLayout({});
-    FAIL() << "Factory should have failed.";
-  } catch (const nimble::NimbleUserError& e) {
-    EXPECT_THAT(
-        e.what(),
-        ::testing::HasSubstr("for feature ordering is not a flat map"));
-  }
+  planner.getLayout({});
 }
 
-TEST(DefaultLayoutPlannerTests, OrdinalOutOfRange) {
+TEST(DefaultLayoutPlannerTests, OrdinalOutOfRangeAreIgnored) {
   nimble::SchemaBuilder builder;
 
   nimble::test::FlatMapChildAdder fm1;
@@ -460,13 +453,7 @@ TEST(DefaultLayoutPlannerTests, OrdinalOutOfRange) {
   nimble::DefaultLayoutPlanner planner{
       [&]() { return builder.getRoot(); },
       {{{6, {3, 42, 9, 2, 21}}, {3, {3, 2, 42, 21}}}}};
-  try {
-    planner.getLayout({});
-    FAIL() << "Factory should have failed.";
-  } catch (const nimble::NimbleUserError& e) {
-    EXPECT_THAT(
-        e.what(), ::testing::HasSubstr("for feature ordering is out of range"));
-  }
+  planner.getLayout({});
 }
 
 TEST(DefaultLayoutPlannerTests, IncompatibleSchema) {