From 00f9fc5e6a20e4b186b3fa4cffc04dcb8fd79ebd Mon Sep 17 00:00:00 2001
From: Daniel Munoz <munozdaniel@meta.com>
Date: Fri, 5 Apr 2024 12:09:36 -0700
Subject: [PATCH] Improve duplicated key error message (#9374)

Summary:
Pull Request resolved: https://github.com/facebookincubator/velox/pull/9374

So the user has more context.

Reviewed By: sdruzkin

Differential Revision: D55779686

fbshipit-source-id: 554be036bfba0534ebd8f3cc13bd7d36e1764e80
---
 velox/dwio/dwrf/test/ColumnWriterTest.cpp    | 45 ++++++++++++++++++--
 velox/dwio/dwrf/writer/FlatMapColumnWriter.h |  8 +++-
 2 files changed, 49 insertions(+), 4 deletions(-)

diff --git a/velox/dwio/dwrf/test/ColumnWriterTest.cpp b/velox/dwio/dwrf/test/ColumnWriterTest.cpp
index 1918ff045316..0447a7f9d884 100644
--- a/velox/dwio/dwrf/test/ColumnWriterTest.cpp
+++ b/velox/dwio/dwrf/test/ColumnWriterTest.cpp
@@ -1215,22 +1215,41 @@ TEST_F(ColumnWriterTest, TestMapWriterInt64Key) {
   testMapWriterNumericKey<int64_t>(/* useFlatMap */ true, /* useStruct */ true);
 }
 
+TEST_F(ColumnWriterTest, TestMapWriterDuplicatedInt64Key) {
+  using T = int64_t;
+  using b = MapBuilder<T, T>;
+
+  auto pool = memory::memoryManager()->addLeafPool();
+  auto batch = b::create(
+      *pool, {typename b::row{typename b::pair{5, 3}, typename b::pair{5, 4}}});
+
+  EXPECT_THAT(
+      ([&]() { testMapWriter<T, T>(*pool, batch, /* useFlatMap */ true); }),
+      Throws<
+          facebook::velox::dwio::common::exception::LoggedException>(Property(
+          &facebook::velox::dwio::common::exception::LoggedException::message,
+          HasSubstr("Duplicated key in map: 5"))));
+}
+
 TEST_F(ColumnWriterTest, TestMapWriterInt32Key) {
   testMapWriterNumericKey<int32_t>(/* useFlatMap */ false);
   testMapWriterNumericKey<int32_t>(/* useFlatMap */ true);
-  testMapWriterNumericKey<int32_t>(/* useFlatMap */ true, /* useStruct */ true);
+  testMapWriterNumericKey<int32_t>(
+      /* useFlatMap */ true, /* useStruct */ true);
 }
 
 TEST_F(ColumnWriterTest, TestMapWriterInt16Key) {
   testMapWriterNumericKey<int16_t>(/* useFlatMap */ false);
   testMapWriterNumericKey<int16_t>(/* useFlatMap */ true);
-  testMapWriterNumericKey<int16_t>(/* useFlatMap */ true, /* useStruct */ true);
+  testMapWriterNumericKey<int16_t>(
+      /* useFlatMap */ true, /* useStruct */ true);
 }
 
 TEST_F(ColumnWriterTest, TestMapWriterInt8Key) {
   testMapWriterNumericKey<int8_t>(/* useFlatMap */ false);
   testMapWriterNumericKey<int8_t>(/* useFlatMap */ true);
-  testMapWriterNumericKey<int8_t>(/* useFlatMap */ true, /* useStruct */ true);
+  testMapWriterNumericKey<int8_t>(
+      /* useFlatMap */ true, /* useStruct */ true);
 }
 
 TEST_F(ColumnWriterTest, TestMapWriterStringKey) {
@@ -1250,6 +1269,26 @@ TEST_F(ColumnWriterTest, TestMapWriterStringKey) {
       *pool_, batch, /* useFlatMap */ true, true, /* useStruct */ true);
 }
 
+TEST_F(ColumnWriterTest, TestMapWriterDuplicatedStringKey) {
+  using keyType = StringView;
+  using valueType = StringView;
+  using b = MapBuilder<keyType, valueType>;
+
+  auto pool = memory::memoryManager()->addLeafPool();
+  auto batch = b::create(
+      *pool_,
+      {b::row{b::pair{"2", "5"}, b::pair{"2", "8"}}}); // Duplicated key: 2
+
+  EXPECT_THAT(
+      ([&]() {
+        testMapWriter<keyType, valueType>(*pool_, batch, /* useFlatMap */ true);
+      }),
+      Throws<
+          facebook::velox::dwio::common::exception::LoggedException>(Property(
+          &facebook::velox::dwio::common::exception::LoggedException::message,
+          HasSubstr("Duplicated key in map: 2"))));
+}
+
 TEST_F(ColumnWriterTest, TestMapWriterDifferentNumericKeyValue) {
   using keyType = float;
   using valueType = int32_t;
diff --git a/velox/dwio/dwrf/writer/FlatMapColumnWriter.h b/velox/dwio/dwrf/writer/FlatMapColumnWriter.h
index 6d265fb032c5..6e50a0e435ac 100644
--- a/velox/dwio/dwrf/writer/FlatMapColumnWriter.h
+++ b/velox/dwio/dwrf/writer/FlatMapColumnWriter.h
@@ -120,7 +120,13 @@ class ValueWriter {
 
   void addOffset(uint64_t offset, uint64_t inMapIndex) {
     if (UNLIKELY(inMapBuffer_[inMapIndex])) {
-      DWIO_RAISE("Duplicate key in map");
+      std::string duplicatedKey = "<unknown>";
+      if (keyInfo_.has_byteskey()) {
+        duplicatedKey = keyInfo_.byteskey();
+      } else if (keyInfo_.has_intkey()) {
+        duplicatedKey = std::to_string(keyInfo_.intkey());
+      }
+      DWIO_RAISE("Duplicated key in map: ", duplicatedKey);
     }
 
     ranges_.add(offset, offset + 1);