Repository navigation
feat(clp-s): Add support for losslessly compressing floating-point numbers; Bump archive format version to v0.4.1. #1176
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 89 commits
2c0a7e3
91b499c
41eee1a
1a3ef60
e4bca46
1c2d47e
32a5375
f6f58be
6bc94cc
8724e11
1f33f98
15e4346
e5717ef
a4cbd89
30c9b08
c56275e
0090052
38d5ae4
87a131f
287b43e
283ce61
b0d083d
3e18f00
bee4fca
71f2fb6
e6ac492
42347c1
284027c
3870058
eff80ce
03506bc
4da57f3
e8db268
5a385c6
c43884d
b6b364a
e72ea13
c0982e7
464f3b6
5425f84
cfc3740
98e0390
b5c1987
091a3fc
731bf83
1d3fd9f
bf9cfb0
18e5aaf
af19cc6
ab38a34
73da7f3
fcea117
7ed2c91
bf9b6cb
0197735
c1bf459
eb9a711
ef79776
05fd52c
1c2a209
d4ef78b
86ed676
c1b2960
c38d22a
0d4b607
0e15df7
6813e1e
200a320
dafb471
f9725db
7c0d698
a2a31b0
b997d17
b8e2d50
2f70218
3894a4d
b37df69
89e9a1f
903e6c2
46983ae
0aca1e2
7da0c1d
5fbdd07
cda2121
f6e1024
2f25beb
66a7e8e
1504898
c0b7e84
c0d6f4c
29fcbf2
777f126
bb46862
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -226,6 +226,8 @@ set( | |
| DictionaryWriter.cpp | ||
| DictionaryWriter.hpp | ||
| ErrorCode.hpp | ||
| FloatFormatEncoding.cpp | ||
| FloatFormatEncoding.hpp | ||
| JsonFileIterator.cpp | ||
|
Comment on lines
+235
to
237
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick Adding FloatFormatEncoding to writer sources: SGTM Good to see the encoding module compiled into the writer. If this module will be shared broadly (writer, reader, json constructor, etc.), consider extracting it into a tiny common target (e.g., clp_s::float_format) and linking both writer/reader to it to avoid repeated compilation. Not urgent given existing project patterns. 🤖 Prompt for AI Agents |
||
| JsonFileIterator.hpp | ||
| JsonParser.cpp | ||
|
|
@@ -268,6 +270,7 @@ if(CLP_BUILD_CLP_S_ARCHIVEWRITER) | |
| msgpack-cxx | ||
| nlohmann_json::nlohmann_json | ||
| simdjson::simdjson | ||
| ystdlib::error_handling | ||
| PRIVATE | ||
| Boost::url | ||
| clp_s::clp_dependencies | ||
|
|
@@ -293,6 +296,8 @@ set( | |
| DictionaryEntry.hpp | ||
| DictionaryReader.hpp | ||
| ErrorCode.hpp | ||
| FloatFormatEncoding.cpp | ||
| FloatFormatEncoding.hpp | ||
| JsonSerializer.hpp | ||
|
Comment on lines
+305
to
307
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick Adding FloatFormatEncoding to reader sources: SGTM Mirrors the writer change; necessary for decode/restore path. Same optional note as above: consider a small shared target if this module expands or is reused elsewhere. 🤖 Prompt for AI Agents |
||
| PackedStreamReader.cpp | ||
| PackedStreamReader.hpp | ||
|
|
@@ -329,6 +334,7 @@ if(CLP_BUILD_CLP_S_ARCHIVEREADER) | |
| clp_s::io | ||
| msgpack-cxx | ||
| nlohmann_json::nlohmann_json | ||
| ystdlib::error_handling | ||
| PRIVATE | ||
| Boost::url | ||
| clp_s::clp_dependencies | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,14 @@ | ||
| #ifndef CLP_S_COLUMNREADER_HPP | ||
| #define CLP_S_COLUMNREADER_HPP | ||
|
|
||
| #include <cstddef> | ||
| #include <cstdint> | ||
|
anlowee marked this conversation as resolved.
Resolved
|
||
| #include <string> | ||
| #include <variant> | ||
|
|
||
| #include "BufferViewReader.hpp" | ||
| #include "DictionaryReader.hpp" | ||
| #include "FloatFormatEncoding.hpp" | ||
| #include "SchemaTree.hpp" | ||
| #include "TimestampDictionaryReader.hpp" | ||
| #include "Utils.hpp" | ||
|
|
@@ -147,6 +150,66 @@ class FloatColumnReader : public BaseColumnReader { | |
| UnalignedMemSpan<double> m_values; | ||
| }; | ||
|
|
||
| class FormattedFloatColumnReader : public BaseColumnReader { | ||
| public: | ||
| // Constructor | ||
| explicit FormattedFloatColumnReader(int32_t id) : BaseColumnReader(id) {} | ||
|
|
||
| // Destructor | ||
| ~FormattedFloatColumnReader() override = default; | ||
|
|
||
| // Methods inherited from BaseColumnReader | ||
| void load(BufferViewReader& reader, uint64_t num_messages) override; | ||
|
|
||
| NodeType get_type() override { return NodeType::FormattedFloat; } | ||
|
|
||
| std::variant<int64_t, double, std::string, uint8_t> extract_value( | ||
| uint64_t cur_message | ||
| ) override; | ||
|
|
||
| /** | ||
| * Appends the floating point value to the buffer in its original format by decoding the stored | ||
| * format information. | ||
| * | ||
| * @param cur_message | ||
| * @param buffer | ||
| */ | ||
| void extract_string_value_into_buffer(uint64_t cur_message, std::string& buffer) override; | ||
|
anlowee marked this conversation as resolved.
Resolved
|
||
|
|
||
|
coderabbitai[bot] marked this conversation as resolved.
Resolved
|
||
| private: | ||
| UnalignedMemSpan<double> m_values; | ||
| UnalignedMemSpan<float_format_t> m_formats; | ||
| }; | ||
|
Comment on lines
+153
to
+182
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) API matches intent; ensure cpp uses float_format_t for the span (see cpp diff). Header declares UnalignedMemSpan<float_format_t>; keep the implementation in sync (replace uint16_t in ColumnReader.cpp). 🤖 Prompt for AI Agents
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe consider replace that
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chainI'll check the implementation file to see if there's indeed an inconsistency between the header declaration and the implementation. @anlowee You're absolutely right! There's a type inconsistency that should be fixed. The header correctly declares Using
The implementation should use 🏁 Script executed: Length of output: 348 Perfect! I found the exact inconsistency you and CodeRabbit identified. On line 61 of m_formats = reader.read_unaligned_span<uint16_t>(num_messages);But the header declares m_formats = reader.read_unaligned_span<float_format_t>(num_messages);This fix will:
The change is straightforward - just replace 🧠 Learnings used |
||
|
|
||
|
anlowee marked this conversation as resolved.
Resolved
|
||
| class DictionaryFloatColumnReader : public BaseColumnReader { | ||
| public: | ||
| // Constructor | ||
| explicit DictionaryFloatColumnReader( | ||
| int32_t id, | ||
| std::shared_ptr<VariableDictionaryReader> var_dict | ||
| ) | ||
| : BaseColumnReader(id), | ||
| m_var_dict{std::move(var_dict)} {} | ||
|
|
||
| // Destructor | ||
| ~DictionaryFloatColumnReader() override = default; | ||
|
|
||
| // Methods inherited from BaseColumnReader | ||
| void load(BufferViewReader& reader, uint64_t num_messages) override; | ||
|
|
||
| NodeType get_type() override { return NodeType::DictionaryFloat; } | ||
|
|
||
| std::variant<int64_t, double, std::string, uint8_t> extract_value( | ||
| uint64_t cur_message | ||
| ) override; | ||
|
|
||
| void extract_string_value_into_buffer(uint64_t cur_message, std::string& buffer) override; | ||
|
|
||
| private: | ||
| std::shared_ptr<VariableDictionaryReader> m_var_dict; | ||
| UnalignedMemSpan<variable_dictionary_id_t> m_var_dict_ids; | ||
| }; | ||
|
anlowee marked this conversation as resolved.
Resolved
|
||
|
|
||
| class BooleanColumnReader : public BaseColumnReader { | ||
| public: | ||
| // Constructor | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -7,6 +7,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "../clp/Defs.h" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "DictionaryWriter.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "FileWriter.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "FloatFormatEncoding.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
anlowee marked this conversation as resolved.
Resolved
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "ParsedMessage.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "TimestampDictionaryWriter.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| #include "ZstdCompressor.hpp" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -98,6 +99,44 @@ class FloatColumnWriter : public BaseColumnWriter { | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::vector<double> m_values; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class FormattedFloatColumnWriter : public BaseColumnWriter { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Constructor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| explicit FormattedFloatColumnWriter(int32_t id) : BaseColumnWriter(id) {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Destructor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ~FormattedFloatColumnWriter() override = default; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Methods inherited from BaseColumnWriter | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| size_t add_value(ParsedMessage::variable_t& value) override; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void store(ZstdCompressor& compressor) override; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::vector<double> m_values; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::vector<float_format_t> m_formats; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class DictionaryFloatColumnWriter : public BaseColumnWriter { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Constructor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| DictionaryFloatColumnWriter(int32_t id, std::shared_ptr<VariableDictionaryWriter> var_dict) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| : BaseColumnWriter(id), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| m_var_dict(std::move(var_dict)) {} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Destructor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ~DictionaryFloatColumnWriter() override = default; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Methods inherited from BaseColumnWriter | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| size_t add_value(ParsedMessage::variable_t& value) override; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| void store(ZstdCompressor& compressor) override; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::shared_ptr<VariableDictionaryWriter> m_var_dict; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| std::vector<clp::variable_dictionary_id_t> m_var_dict_ids; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+102
to
+138
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧹 Nitpick (assertive) Introduce a named alias for the 16-bit format descriptor for clarity and future refactors A small alias improves readability and prevents scattering the underlying type across the codebase. It also documents intent at the point of use. Apply this diff: class FormattedFloatColumnWriter : public BaseColumnWriter {
public:
// Constructor
explicit FormattedFloatColumnWriter(int32_t id) : BaseColumnWriter(id) {}
@@
private:
- std::vector<double> m_values;
- std::vector<uint16_t> m_formats;
+ using format_descriptor_t = uint16_t; // 16-bit format descriptor; see FloatFormatEncoding.hpp
+ std::vector<double> m_values;
+ std::vector<format_descriptor_t> m_formats;
};📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class BooleanColumnWriter : public BaseColumnWriter { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| public: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Constructor | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.