complete BlockBasedTableFactory::ValidateOptions block_align validation

ajkr · Apr 26, 2024 · b77b6f6 · b77b6f6
1 parent 2d29011
commit b77b6f6
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 10 deletions.
diff --git a/table/block_based/block_based_table_builder.cc b/table/block_based/block_based_table_builder.cc
@@ -628,10 +628,9 @@ struct BlockBasedTableBuilder::Rep {
     }
 
     if (alignment > 0 && compression_type != kNoCompression) {
-      // With better sanitization in upper layers
-      // (`BlockBasedTableFactory::ValidateOptions()` and
-      // `CompactionPicker::CompactFiles()`), we would not need to handle this
-      // case here and could change it to an assertion instead.
+      // With better sanitization in `CompactionPicker::CompactFiles()`, we
+      // would not need to handle this case here and could change it to an
+      // assertion instead.
       SetStatus(Status::InvalidArgument(
           "Enable block_align, but compression enabled"));
     }

diff --git a/table/block_based/block_based_table_factory.cc b/table/block_based/block_based_table_factory.cc
@@ -619,6 +619,21 @@ Status BlockBasedTableFactory::ValidateOptions(
         "Enable block_align, but compression "
         "enabled");
   }
+  if (table_options_.block_align &&
+      cf_opts.compression != kDisableCompressionOption &&
+      cf_opts.bottommost_compression != kNoCompression) {
+    return Status::InvalidArgument(
+        "Enable block_align, but bottommost_compression enabled");
+  }
+  if (table_options_.block_align) {
+    for (auto level_compression : cf_opts.compression_per_level) {
+      if (level_compression != kDisableCompressionOption &&
+          level_compression != kNoCompression) {
+        return Status::InvalidArgument(
+            "Enable block_align, but compression_per_level enabled");
+      }
+    }
+  }
   if (table_options_.block_align &&
       (table_options_.block_size & (table_options_.block_size - 1))) {
     return Status::InvalidArgument(

diff --git a/table/table_test.cc b/table/table_test.cc
@@ -5860,6 +5860,7 @@ TEST_P(BlockBasedTableTest, SeekMetaBlocks) {
 TEST_P(BlockBasedTableTest, BadOptions) {
   ROCKSDB_NAMESPACE::Options options;
   options.compression = kNoCompression;
+  options.create_if_missing = true;
   BlockBasedTableOptions bbto = GetBlockBasedTableOptions();
   bbto.block_size = 4000;
   bbto.block_align = true;
@@ -5868,13 +5869,29 @@ TEST_P(BlockBasedTableTest, BadOptions) {
       test::PerThreadDBPath("block_based_table_bad_options_test");
   options.table_factory.reset(NewBlockBasedTableFactory(bbto));
   ASSERT_OK(DestroyDB(kDBPath, options));
-  ROCKSDB_NAMESPACE::DB* db;
-  ASSERT_NOK(ROCKSDB_NAMESPACE::DB::Open(options, kDBPath, &db));
 
-  bbto.block_size = 4096;
-  options.compression = kSnappyCompression;
-  options.table_factory.reset(NewBlockBasedTableFactory(bbto));
-  ASSERT_NOK(ROCKSDB_NAMESPACE::DB::Open(options, kDBPath, &db));
+  std::unique_ptr<DB> db;
+  {
+    ROCKSDB_NAMESPACE::DB* _db;
+    ASSERT_NOK(ROCKSDB_NAMESPACE::DB::Open(options, kDBPath, &_db));
+
+    bbto.block_size = 4096;
+    options.compression = kSnappyCompression;
+    options.table_factory.reset(NewBlockBasedTableFactory(bbto));
+    ASSERT_NOK(ROCKSDB_NAMESPACE::DB::Open(options, kDBPath, &_db));
+
+    options.compression = kNoCompression;
+    options.bottommost_compression = kSnappyCompression;
+    ASSERT_NOK(ROCKSDB_NAMESPACE::DB::Open(options, kDBPath, &_db));
+
+    options.bottommost_compression = kNoCompression;
+    options.compression_per_level.emplace_back(kSnappyCompression);
+    ASSERT_NOK(ROCKSDB_NAMESPACE::DB::Open(options, kDBPath, &_db));
+
+    options.compression_per_level.clear();
+    ASSERT_OK(ROCKSDB_NAMESPACE::DB::Open(options, kDBPath, &_db));
+    db.reset(_db);
+  }
 }
 
 TEST_F(BBTTailPrefetchTest, TestTailPrefetchStats) {