From 399f815a89a55b8b94c221c291c3ed3f4ee2f198 Mon Sep 17 00:00:00 2001 From: Vladimir Krivopalov Date: Thu, 25 Oct 2018 17:07:20 -0700 Subject: [PATCH 1/3] schema: Add helper method returning the count of columns of specified kind. Signed-off-by: Vladimir Krivopalov --- schema.cc | 15 +++++++++++++++ schema.hh | 1 + 2 files changed, 16 insertions(+) diff --git a/schema.cc b/schema.cc index 9c9ee67c22..3bdd994bdd 100644 --- a/schema.cc +++ b/schema.cc @@ -1121,6 +1121,21 @@ schema::has_static_columns() const { return !static_columns().empty(); } +column_count_type +schema::columns_count(column_kind kind) const { + switch (kind) { + case column_kind::partition_key: + return partition_key_size(); + case column_kind::clustering_key: + return clustering_key_size(); + case column_kind::static_column: + return static_columns_count(); + case column_kind::regular_column: + return regular_columns_count(); + default: + std::abort(); + } +} column_count_type schema::partition_key_size() const { return column_offset(column_kind::clustering_key); diff --git a/schema.hh b/schema.hh index 9c9e2612f4..e0e613bf60 100644 --- a/schema.hh +++ b/schema.hh @@ -701,6 +701,7 @@ public: bool is_last_partition_key(const column_definition& def) const; bool has_multi_cell_collections() const; bool has_static_columns() const; + column_count_type columns_count(column_kind kind) const; column_count_type partition_key_size() const; column_count_type clustering_key_size() const; column_count_type static_columns_count() const; From 44043cfd4437aa852129ee4a285539ac5b217175 Mon Sep 17 00:00:00 2001 From: Vladimir Krivopalov Date: Thu, 25 Oct 2018 17:08:03 -0700 Subject: [PATCH 2/3] sstables: Honour the column kind when writing missing columns in 'mc' format. Previously, we've been writing the wrong missing columns indices for static rows because write_missing_columns() explicitly used regular columns internally. Now, it takes the proper column kind into account. Fixes #3892 Signed-off-by: Vladimir Krivopalov --- sstables/m_format_write_helpers.cc | 16 ++++++++++------ sstables/m_format_write_helpers.hh | 2 +- sstables/sstables.cc | 2 +- 3 files changed, 12 insertions(+), 8 deletions(-) diff --git a/sstables/m_format_write_helpers.cc b/sstables/m_format_write_helpers.cc index ea4b2d3de7..2ace0e297f 100644 --- a/sstables/m_format_write_helpers.cc +++ b/sstables/m_format_write_helpers.cc @@ -204,6 +204,7 @@ class missing_columns_input_range : public input_range_base { private: const schema& _schema; + const column_kind _kind; const row& _row; mutable uint64_t _current_value = 0; mutable column_id _current_id = 0; @@ -216,12 +217,15 @@ private: } _mode; public: - missing_columns_input_range(const schema& s, const row& row) + missing_columns_input_range(const schema& s, column_kind kind, const row& row) : _schema(s) + , _kind(kind) , _row(row) { + assert(kind == column_kind::static_column || kind == column_kind::regular_column); + auto row_size = _row.size(); - auto total_size = _schema.regular_columns_count(); + auto total_size = _schema.columns_count(_kind); _current_id = row_size < total_size ? 0 : total_size; _mode = (total_size < 64) ? encoding_mode::small : @@ -230,7 +234,7 @@ public: } bool next() const { - auto total_size = _schema.regular_columns_count(); + auto total_size = _schema.columns_count(_kind); if (_current_id == total_size) { // No more values to encode return false; @@ -285,12 +289,12 @@ public: explicit operator bool() const { - return (_current_id < _schema.regular_columns_count()); + return (_current_id < _schema.columns_count(_kind)); } }; -void write_missing_columns(file_writer& out, const schema& s, const row& row) { - for (const auto value: missing_columns_input_range{s, row}) { +void write_missing_columns(file_writer& out, const schema& s, column_kind kind, const row& row) { + for (const auto value: missing_columns_input_range{s, kind, row}) { write_vint(out, value); } } diff --git a/sstables/m_format_write_helpers.hh b/sstables/m_format_write_helpers.hh index c706503680..ff2945ef7e 100644 --- a/sstables/m_format_write_helpers.hh +++ b/sstables/m_format_write_helpers.hh @@ -75,7 +75,7 @@ void write_clustering_prefix(file_writer& out, const schema& s, const clustering_key_prefix& prefix, ephemerally_full_prefix is_ephemerally_full); // Writes encoded information about missing columns in the given row -void write_missing_columns(file_writer& out, const schema& s, const row& row); +void write_missing_columns(file_writer& out, const schema& s, column_kind kind, const row& row); // Helper functions for writing delta-encoded time-related values void write_delta_timestamp(file_writer& out, api::timestamp_type timestamp, const encoding_stats& enc_stats); diff --git a/sstables/sstables.cc b/sstables/sstables.cc index e94babbd29..572cc87d0b 100644 --- a/sstables/sstables.cc +++ b/sstables/sstables.cc @@ -3209,7 +3209,7 @@ void sstable_writer_m::write_cells(file_writer& writer, column_kind kind, const // This differs from Origin where all updated columns are tracked and the set of filled columns of a row // is compared with the set of all columns filled in the memtable. So our encoding may be less optimal in some cases // but still valid. - write_missing_columns(writer, _schema, row_body); + write_missing_columns(writer, _schema, kind, row_body); row_body.for_each_cell([this, &writer, kind, &properties, has_complex_deletion] (column_id id, const atomic_cell_or_collection& c) { auto&& column_definition = _schema.column_at(kind, id); if (!column_definition.is_atomic()) { From 9843343ad841829aebfa69d1e5a453ac66a6445d Mon Sep 17 00:00:00 2001 From: Vladimir Krivopalov Date: Thu, 25 Oct 2018 17:16:31 -0700 Subject: [PATCH 3/3] tests: Add test for a static row with missing columns (SStables 3.x.). This is a test case for #3892. Signed-off-by: Vladimir Krivopalov --- tests/sstable_3_x_test.cc | 28 ++++++++++++++++++ .../mc-1-big-Data.db | Bin 0 -> 44 bytes .../mc-1-big-Digest.crc32 | 1 + .../mc-1-big-Filter.db | Bin 0 -> 16 bytes .../mc-1-big-Index.db | Bin 0 -> 8 bytes .../mc-1-big-Statistics.db | Bin 0 -> 4739 bytes 6 files changed, 29 insertions(+) create mode 100644 tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Data.db create mode 100644 tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Digest.crc32 create mode 100644 tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Filter.db create mode 100644 tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Index.db create mode 100644 tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Statistics.db diff --git a/tests/sstable_3_x_test.cc b/tests/sstable_3_x_test.cc index 69750b9247..2b08706fdb 100644 --- a/tests/sstable_3_x_test.cc +++ b/tests/sstable_3_x_test.cc @@ -4639,3 +4639,31 @@ SEASTAR_THREAD_TEST_CASE(test_regular_and_shadowable_deletion) { validate_read(s, tmp.path, {mut1, mut2}); } +SEASTAR_THREAD_TEST_CASE(test_write_static_row_with_missing_columns) { + auto abj = defer([] { await_background_jobs().get(); }); + sstring table_name = "static_row_with_missing_columns"; + // CREATE TABLE static_row (pk int, ck int, st1 int static, st2 int static, rc int, PRIMARY KEY (pk, ck)) WITH compression = {'sstable_compression': ''}; + schema_builder builder("sst3", table_name); + builder.with_column("pk", int32_type, column_kind::partition_key); + builder.with_column("ck", int32_type, column_kind::clustering_key); + builder.with_column("st1", int32_type, column_kind::static_column); + builder.with_column("st2", int32_type, column_kind::static_column); + builder.with_column("rc", int32_type); + builder.set_compressor_params(compression_parameters()); + schema_ptr s = builder.build(schema_builder::compact_storage::no); + + lw_shared_ptr mt = make_lw_shared(s); + + // INSERT INTO static_row (pk, ck, st1, rc) VALUES (0, 1, 2, 3); + auto key = partition_key::from_deeply_exploded(*s, {0}); + mutation mut{s, key}; + clustering_key ckey = clustering_key::from_deeply_exploded(*s, { 1 }); + mut.partition().apply_insert(*s, ckey, write_timestamp); + mut.set_static_cell("st1", data_value{2}, write_timestamp); + mut.set_cell(ckey, "rc", data_value{3}, write_timestamp); + mt->apply(mut); + + tmpdir tmp = write_and_compare_sstables(s, mt, table_name); + validate_read(s, tmp.path, {mut}); +} + diff --git a/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Data.db b/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Data.db new file mode 100644 index 0000000000000000000000000000000000000000..6932a66f34631c1edad48a918caa5571f7a335f2 GIT binary patch literal 44 ncmZQzVE}^q|Ns9tfEYl~z{tVC1ZFX*fG9?GSq2Uu&CCb@q3;Ca literal 0 HcmV?d00001 diff --git a/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Digest.crc32 b/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Digest.crc32 new file mode 100644 index 0000000000..e90a9206fe --- /dev/null +++ b/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Digest.crc32 @@ -0,0 +1 @@ +712215703 \ No newline at end of file diff --git a/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Filter.db b/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Filter.db new file mode 100644 index 0000000000000000000000000000000000000000..157a4de7cff17365433da137f823747e58804d15 GIT binary patch literal 16 XcmZQzU|?lnU|?imQefz4U|;|M1jqp~ literal 0 HcmV?d00001 diff --git a/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Index.db b/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Index.db new file mode 100644 index 0000000000000000000000000000000000000000..f9376371f57ea150ba2e6002d905fabebd52132f GIT binary patch literal 8 LcmZQzVE_XF03-kf literal 0 HcmV?d00001 diff --git a/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Statistics.db b/tests/sstables/3.x/uncompressed/write_static_row_with_missing_columns/mc-1-big-Statistics.db new file mode 100644 index 0000000000000000000000000000000000000000..e6cb0f3fb0faa16a19804e0eef43438e7059e338 GIT binary patch literal 4739 zcmeI$eM}Q)90%}wX@Meiw4fqJ;tTk~mddDzCMr`)piUzKDms^WltyslCD)>e7>#59 zkOgs>GL0|WOo%}vaZUv^`iBaeb3@58wq+QWR9sxPs__M;vC00p7SKm(>SLDteuB6F1~z|c4MR@w zn4ws(u^8v0$@Uk-6RC=7HO;;`!-ogYzjy#yqC^VrKTI;GI*CgMYY^N-fPe56@R=zwu`pmlLXC ze#q_b8)#ftZCFID-_Q%k8&*+TP2=Hho+s1>Th=4$F^L^;KSdXsZ_;>-Z11Fw%}9p% zJl^>13mTtzV(&fbDRXkZp`P*U+6C0JZ_rxc527crS$$6TXN6S`(N~ZZlw3WnB+$^ov!(oICAAzohotE z2Mzzy<2U|hx<`-SbSvkKndFaE>MQB{V8Zdt4hxAVem`KQ*DL8^w!NOj-&*ZWru9I& z_3ut2iD&lS?bu4Z{7~yjI-d8u^V8{kF3+{YA!e*p_(gHbCC)0y474GL)U*;uw%{DY9a8ulYQLq zFIxQaeXS=HkMx2a@#SY0{~I*edl}VUrcy9>xcA~+LwBu>)eEy;{q?2DQl_Y`66GU{ iz1(e0E7(@)6i1&b$u9TXqfckqX`{~;CDk$d?Ee9!8k$c4 literal 0 HcmV?d00001