Skip to content

Commit 56fedd4

Browse files
author
Krishna Pai
committed
fix(nimble): Make the shared dictionary export build and pass checks
#18640 brought D116132982's shared dictionary work across byte-identical to fbcode, which is right for the export but leaves it not compiling here and failing pre-commit. These are the repairs, kept separate so the export itself stays a clean mirror. Two struct members arrived without default initializers while their neighbours have them. Velox builds -Wextra -Werror and fbcode does not promote -Wmissing-field-initializers, so the omissions are invisible internally and fatal here: - Encoding::Options::sharedDictionaryAlphabet, 196 instances across ConstantEncodingTest, DictionaryEncodingTest, EncodingTest, FsstEncodingTest and HuffmanEncodingViewTest. Set to nullptr, matching bufferPool, encodingBufferPool and decodingStats beside it. - SharedDictionaryConfig::alphabetEncodings, 5 instances in SharedDictionaryConfigTest. Given `{}`, matching dictionaryId and useExternalAlphabet beside it. ExternalSharedDictionaryBuilder::resetImpl passed a Kind enum straight to NIMBLE_UNSUPPORTED. Velox bundles fmt v11, which will not format an enum implicitly: error: 'type_is_unformattable_for<SharedDictionaryBuilder<int>::Kind, char>' has incomplete type instantiated for four value types. SharedDictionaryBuilder::kindString already exists for this, so it is used, which also turns the message from an integer into "External shared dictionary builder does not support reset()". Four headers reached this tree without CMake entries, so check-header-ownership failed: SharedDictionaryCatalog.h, SharedDictionaryReader.h, ExternalDictionaryBuilder.h and SharedDictionaryConfig.h. Each is listed alongside the .cpp already in its target. fbcode builds with BUCK, so nothing internal enforces this. All 1651 headers are tracked again. Touching those files makes gersemi reformat two add_dependencies blocks that arrived unformatted; that is the hook's doing, not a manual edit. Velox is GitHub-first, so the next import carries these into fbcode.
1 parent a9c90e0 commit 56fedd4

7 files changed

Lines changed: 19 additions & 13 deletions

File tree

velox/dwio/nimble/encodings/CMakeLists.txt

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,7 @@ add_library(
3232
PrefixEncoding.cpp
3333
RLEEncoding.cpp
3434
SharedDictionaryCatalog.cpp
35+
SharedDictionaryCatalog.h
3536
SharedDictionaryEncoding.cpp
3637
SparseBoolEncoding.cpp
3738
TrivialEncoding.cpp
@@ -134,10 +135,7 @@ target_include_directories(
134135
nimble_shared_dictionary_fb
135136
INTERFACE ${PROJECT_BINARY_DIR} ${FLATBUFFERS_INCLUDE_DIR}
136137
)
137-
add_dependencies(
138-
nimble_shared_dictionary_fb
139-
nimble_shared_dictionary_schema_fb
140-
)
138+
add_dependencies(nimble_shared_dictionary_fb nimble_shared_dictionary_schema_fb)
141139

142140
target_link_libraries(
143141
nimble_encodings

velox/dwio/nimble/encodings/SharedDictionaryBuilder.h

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -289,7 +289,8 @@ class ExternalSharedDictionaryBuilder final
289289

290290
void resetImpl() final {
291291
NIMBLE_UNSUPPORTED(
292-
"{} shared dictionary builder does not support reset().", this->kind());
292+
"{} shared dictionary builder does not support reset().",
293+
SharedDictionaryBuilder<T>::kindString(this->kind()));
293294
}
294295

295296
private:

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

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -162,7 +162,8 @@ class Encoding {
162162

163163
/// Direct alphabet for SharedDictionary encodings when the read path has
164164
/// already resolved the dictionary bound to this value stream.
165-
std::shared_ptr<const SharedDictionaryAlphabet> sharedDictionaryAlphabet;
165+
std::shared_ptr<const SharedDictionaryAlphabet> sharedDictionaryAlphabet =
166+
nullptr;
166167

167168
velox::io::IoCounter* decompressCounter() const {
168169
return decodingStats != nullptr ? &decodingStats->decompressCPUTimeNanos

velox/dwio/nimble/tablet/CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -186,6 +186,7 @@ add_library(
186186
nimble_tablet_reader
187187
FileLayout.cpp
188188
SharedDictionaryReader.cpp
189+
SharedDictionaryReader.h
189190
StripeGroup.cpp
190191
TabletReader.cpp
191192
TabletReader.h

velox/dwio/nimble/tools/CMakeLists.txt

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,7 @@ target_include_directories(
2929
nimble_external_dictionary_fb
3030
INTERFACE ${PROJECT_BINARY_DIR} ${FLATBUFFERS_INCLUDE_DIR}
3131
)
32-
add_dependencies(
33-
nimble_external_dictionary_fb
34-
nimble_external_dictionary_schema_fb
35-
)
32+
add_dependencies(nimble_external_dictionary_fb nimble_external_dictionary_schema_fb)
3633

3734
add_library(nimble_dump_lib NimbleDumpLib.cpp NimbleDumpLib.h)
3835
target_link_libraries(
@@ -73,7 +70,11 @@ target_link_libraries(
7370
velox_memory
7471
)
7572

76-
add_library(nimble_external_dictionary_builder ExternalDictionaryBuilder.cpp)
73+
add_library(
74+
nimble_external_dictionary_builder
75+
ExternalDictionaryBuilder.cpp
76+
ExternalDictionaryBuilder.h
77+
)
7778
target_link_libraries(
7879
nimble_external_dictionary_builder
7980
nimble_external_dictionary_fb

velox/dwio/nimble/velox/CMakeLists.txt

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,11 @@
1414
add_library(nimble_velox_common SchemaUtils.cpp SchemaUtils.h)
1515
target_link_libraries(nimble_velox_common nimble_common velox_type Folly::folly fmt::fmt)
1616

17-
add_library(nimble_velox_shared_dictionary_config SharedDictionaryConfig.cpp)
17+
add_library(
18+
nimble_velox_shared_dictionary_config
19+
SharedDictionaryConfig.cpp
20+
SharedDictionaryConfig.h
21+
)
1822
target_link_libraries(
1923
nimble_velox_shared_dictionary_config
2024
nimble_common

velox/dwio/nimble/velox/SharedDictionaryConfig.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ struct SharedDictionaryConfig {
3737
/// Resolves a provided alphabet instead of building one from written values.
3838
bool useExternalAlphabet{false};
3939
/// Candidate encodings for alphabets stored in this file.
40-
std::vector<EncodingType> alphabetEncodings;
40+
std::vector<EncodingType> alphabetEncodings{};
4141
};
4242

4343
/// Shared dictionary settings for one regular column value stream.

0 commit comments

Comments
 (0)