fix(nimble): Keep nulls in dictionary reads without AVX2 (#18621)

Summary:

Reading a Nimble string column that keeps its dictionary encoding dropped every null on any platform where `process::hasAvx2()` is false, which is all of aarch64. A 200-row column holding `always_the_same` with a null every 7th row came back as a `DictionaryVector` carrying no null flags, so row 0 read as `always_the_same` instead of null.

The dense dictionary-index path wrote nulls only into the reader's read-range bitmap. It then relied on `returnReaderNulls_` to hand that bitmap back from `resultNulls()`. `setReturnNullsMode` clears that flag whenever `useBulkPath()` is false, and `process::hasAvx2()` makes that permanent off AVX2. `resultNulls()` then returns the output-indexed buffer instead, which nothing on this path filled. It now copies the read-range nulls into that buffer, the way the sparse row-set path already did.

Reviewed By: vandreykiv

Differential Revision: D116833280

Author: Yuhta

velox/dwio/nimble/encodings/CMakeLists.txt

@@ -83,6 +83,7 @@ add_library(
   SparseBoolEncoding.h
   SimdForBitpackEncoding.h
   SharedDictionaryEncoding.h
+  SharedDictionaryCatalog.h
   SharedDictionaryBuilder.h
   SentinelEncoding.h
   RLEEncoding.h
@@ -134,10 +135,7 @@ target_include_directories(
   nimble_shared_dictionary_fb
   INTERFACE ${PROJECT_BINARY_DIR} ${FLATBUFFERS_INCLUDE_DIR}
 )
-add_dependencies(
-  nimble_shared_dictionary_fb
-  nimble_shared_dictionary_schema_fb
-)
+add_dependencies(nimble_shared_dictionary_fb nimble_shared_dictionary_schema_fb)
 
 target_link_libraries(
   nimble_encodings

velox/dwio/nimble/encodings/common/Encoding.h

@@ -89,22 +89,22 @@ struct ReadWithVisitorParams {
   // Create the reader nulls buffer if not already exists and return pointer to
   // the raw buffer.  When it is created, it is created with the full length
   // across potential multiple chunks.
-  std::function<uint64_t*()> makeReaderNulls;
+  std::function<uint64_t*()> makeReaderNulls{};
 
   // Resolves which buffer `SelectiveColumnReader::resultNulls()' returns for
   // this read by setting the `returnReaderNulls_' flag (and `anyNulls_').
   // Resolves the flag only; allocates no buffer.  Must be called after decoding
   // nulls in `NullableEncoding'.
-  std::function<void()> setReturnNullsMode;
+  std::function<void()> setReturnNullsMode{};
 
   // Create the result nulls if not already exists.  Similar to
   // `makeReaderNulls', we create one single buffer for all the results nulls
   // across potential multiple chunks during one read.
-  std::function<void()> prepareResultNulls;
+  std::function<void()> prepareResultNulls{};
 
   // Number of rows scanned so far.  Contains rows scanned in previous chunks
   // during this read call as well.
-  vector_size_t numScanned;
+  vector_size_t numScanned{};
 };
 
 class Encoding {
@@ -162,7 +162,7 @@ class Encoding {
 
     /// Direct alphabet for SharedDictionary encodings when the read path has
     /// already resolved the dictionary bound to this value stream.
-    std::shared_ptr<const SharedDictionaryAlphabet> sharedDictionaryAlphabet;
+    std::shared_ptr<const SharedDictionaryAlphabet> sharedDictionaryAlphabet{};
 
     velox::io::IoCounter* decompressCounter() const {
       return decodingStats != nullptr ? &decodingStats->decompressCPUTimeNanos
@@ -527,6 +527,25 @@ void readDenseMaterializedIndices(
       /*sourceBegin=*/valueOutputOffset,
       rawOuterNonNullRows,
       rawOutputValues);
+
+  // Nulls were materialized into the reader's read-range bitmap only.
+  // `resultNulls()' hands that bitmap back while `returnReaderNulls_' holds,
+  // but `setReturnNullsMode' clears the flag whenever `useBulkPath()' is false
+  // -- notably on a platform without AVX2, where every read takes that branch.
+  // `resultNulls()' then returns the output-indexed `resultNulls_', which
+  // nothing on this path writes, so the output would silently lose its nulls.
+  // Copy the read-range nulls across, mirroring what
+  // `readSparseMaterializedIndices' does for the sparse row set.
+  if (!visitor.reader().returnReaderNulls()) {
+    auto* rawResultNulls = visitor.reader().rawResultNulls();
+    NIMBLE_CHECK_NOT_NULL(
+        rawResultNulls,
+        "prepareResultNulls must allocate result nulls before the dense index "
+        "path writes them");
+    velox::bits::copyBits(
+        rawNulls, readOffset, rawResultNulls, valueOutputOffset, numReadRows);
+    visitor.reader().setHasNulls();
+  }
   visitor.addNumValues(numReadRows);
   visitor.setRowIndex(visitor.numRows());
 }

velox/dwio/nimble/encodings/tests/ReadWithVisitorTest.cpp

@@ -4238,9 +4238,16 @@ TEST_P(ReadWithVisitorNonLegacyTest, readDenseMaterializedIndicesWithNulls) {
 
   ASSERT_EQ(reader->numValues(), 0);
 
-  // Call the helper with nulls.
+  // Call the helper with nulls. prepareResultNulls must mirror what
+  // ChunkedDecoder wires up in production: the dense index path materializes
+  // nulls into the read-range bitmap only, so it needs an allocated,
+  // output-indexed result-nulls buffer to copy them into whenever
+  // returnReaderNulls_ is false -- as it is here, the scan spec carries a
+  // filter.
   ReadWithVisitorParams params{.numScanned = 0};
-  params.prepareResultNulls = [] {};
+  params.prepareResultNulls = [&] {
+    reader->prepareNulls(rows, /*hasNulls=*/true, /*extraRows=*/8);
+  };
   detail::readDenseMaterializedIndices(
       *encoding,
       visitor,

velox/dwio/nimble/serializer/benchmarks/DeserializerBenchmark.cpp

@@ -33,9 +33,9 @@ constexpr size_t kNumColumns = 100;
 constexpr size_t kProjectionStride = 20;
 
 struct BenchmarkState {
-  std::string serialized;
-  std::shared_ptr<const Type> schema;
-  std::vector<Deserializer::Subfield> selectedSubfields;
+  std::string serialized{};
+  std::shared_ptr<const Type> schema{};
+  std::vector<Deserializer::Subfield> selectedSubfields{};
 };
 
 BenchmarkState prepareBenchmark() {

velox/dwio/nimble/serializer/benchmarks/StreamSlicerBenchmark.cpp

@@ -94,14 +94,14 @@ DEFINE_bool(
 using Subfield = velox::common::Subfield;
 
 struct BenchmarkState {
-  std::shared_ptr<velox::memory::MemoryPool> pool;
-  std::string serialized;
-  std::shared_ptr<const Type> schema;
-  std::shared_ptr<const Type> projectedSchema;
-  std::vector<Subfield> selectedSubfields;
-  std::unique_ptr<Projector> projector;
-  std::unique_ptr<Deserializer> deserializer;
-  std::string encodingName;
+  std::shared_ptr<velox::memory::MemoryPool> pool{};
+  std::string serialized{};
+  std::shared_ptr<const Type> schema{};
+  std::shared_ptr<const Type> projectedSchema{};
+  std::vector<Subfield> selectedSubfields{};
+  std::unique_ptr<Projector> projector{};
+  std::unique_ptr<Deserializer> deserializer{};
+  std::string encodingName{};
 };
 
 velox::vector_size_t numRows() {

velox/dwio/nimble/tablet/CMakeLists.txt

@@ -190,6 +190,7 @@ add_library(
   TabletReader.cpp
   TabletReader.h
   StripeGroup.h
+  SharedDictionaryReader.h
   FileLayout.h
 )
 target_link_libraries(

velox/dwio/nimble/tablet/TabletReaderCache.h

@@ -140,7 +140,7 @@ class TabletReaderCache {
 
     /// Executor for background IO during TabletReader construction
     /// (e.g., parallel metadata loading).
-    std::shared_ptr<folly::Executor> executor;
+    std::shared_ptr<folly::Executor> executor{};
 
     /// Observe the lifetime of each cached tablet, which is the only way to
     /// reach the metadata and index IO it does. onCreate runs once the entry
@@ -151,8 +151,8 @@ class TabletReaderCache {
     ///
     /// A throw from onCreate propagates to the caller. A throw from onRelease
     /// cannot: it is invoked from a destructor, so it is logged and swallowed.
-    std::function<void(const CachedTabletReader&)> onCreate;
-    std::function<void(const CachedTabletReader&)> onRelease;
+    std::function<void(const CachedTabletReader&)> onCreate{};
+    std::function<void(const CachedTabletReader&)> onRelease{};
 
     std::string toString() const {
       return fmt::format(

velox/dwio/nimble/tools/CMakeLists.txt

@@ -29,10 +29,7 @@ target_include_directories(
   nimble_external_dictionary_fb
   INTERFACE ${PROJECT_BINARY_DIR} ${FLATBUFFERS_INCLUDE_DIR}
 )
-add_dependencies(
-  nimble_external_dictionary_fb
-  nimble_external_dictionary_schema_fb
-)
+add_dependencies(nimble_external_dictionary_fb nimble_external_dictionary_schema_fb)
 
 add_library(nimble_dump_lib NimbleDumpLib.cpp NimbleDumpLib.h)
 target_link_libraries(
@@ -73,7 +70,11 @@ target_link_libraries(
   velox_memory
 )
 
-add_library(nimble_external_dictionary_builder ExternalDictionaryBuilder.cpp)
+add_library(
+  nimble_external_dictionary_builder
+  ExternalDictionaryBuilder.cpp
+  ExternalDictionaryBuilder.h
+)
 target_link_libraries(
   nimble_external_dictionary_builder
   nimble_external_dictionary_fb

velox/dwio/nimble/tools/ExternalDictionaryBuilder.h

@@ -57,10 +57,10 @@ class ExternalDictionaryBuilder {
 
     /// Optional forced alphabet encoding. When unset, readFactors or default
     /// encoding selection picks the alphabet encoding.
-    std::optional<EncodingType> alphabetEncoding;
+    std::optional<EncodingType> alphabetEncoding{};
 
     /// Optional parsed manual read factors used by encoding selection.
-    std::vector<std::pair<EncodingType, float>> readFactors;
+    std::vector<std::pair<EncodingType, float>> readFactors{};
   };
 
   explicit ExternalDictionaryBuilder(velox::memory::MemoryPool* pool);

velox/dwio/nimble/tools/tests/CMakeLists.txt

@@ -24,10 +24,7 @@ target_link_libraries(
   gtest_main
 )
 
-add_executable(
-  external_dictionary_builder_tests
-  ExternalDictionaryBuilderTest.cpp
-)
+add_executable(external_dictionary_builder_tests ExternalDictionaryBuilderTest.cpp)
 add_test(external_dictionary_builder_tests external_dictionary_builder_tests)
 target_link_libraries(
   external_dictionary_builder_tests