[DYOD] Maintain UCCs in face of potentially changing data #2599

j-hellenberg · 2023-08-02T16:12:01Z

UCCs (Unique Column Combinations) indicate the uniqueness across all entries of a column (or a set of columns). UCCs can be given by the schema (a primary key guarantees uniqueness), but also happen "incidentally" in real-world data.

Because UCCs can be used for query optimization, we want to detect these "incidental" UCCs as well, because, for optimization purposes, we can use them just as we would use a primary key constraint.

Before this PR, hyrise was already capable to detect such UCCs. It did so by actually generating UNIQUE constraints on a table, which are translated into UCCs during query optimization. However, this discovery happened under the assumption that the data of the table never changes. This means, if the data of the table were to change and violate the previously discovered "incidental" UCC by creating a duplicate value in the targeted column, this using the UCC for query optimization would lead to incorrect query results.

Therefore, in this MR, we assign a validation commit ID to the TableKeyConstraint (UCC) such that it may be only used for optimization if the table has not seen any changes since this commit ID (note that every modifying transaction increments the global commit ID). We will then regularly revalidate the UCC and see if we can expand the constraint to larger commit IDs.

For optimization purposes, we also make use of the MVCC (Multi-Version Concurrency Control) data of chunks, which includes but is not limited to the commit IDs of when any data was last inserted to or deleted from the chunk (note that hyrise models modifications as delete+insert).

SanJSp

Hey all, I reviewed your code in the name of Team 3.
First of all: Good job with the implementation!

I checked mainly for hyrise coding standards, which were all pretty well followed. I already started to look inside your comments to find stuff I could pick on 😁 So I think this is a pretty good sign. Likewise, I've tried to add some comments regarding const in between, sometimes I was not too sure if it's applicable. Feel free to try it out and if it works implement it, if not, so be it 😊

I will take a look at the other PR as well 😊

src/lib/storage/constraints/table_key_constraint.hpp

src/lib/storage/constraints/table_key_constraint.cpp

src/lib/storage/table.hpp

src/lib/storage/table.cpp

src/test/plugins/ucc_discovery_plugin_test.cpp

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

… become invalid Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

…id using MVCC data Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

…Cs becoming invalid and vice versa Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

dey4ss

I have a few suggestions, but nothing deal-breaking. Nice :)

dey4ss · 2023-09-28T14:44:03Z

src/lib/storage/constraints/table_key_constraint.hpp

@@ -21,11 +21,22 @@ class TableKeyConstraint final : public AbstractTableConstraint {
   * voilating the set semantics of the constraint.
   */
  TableKeyConstraint(const std::set<ColumnID>& columns, const KeyConstraintType key_type);
+  TableKeyConstraint(const std::set<ColumnID>& columns, const KeyConstraintType key_type, const CommitID last_validated_on);


As noted in your other PR, just use a default parameter here:

TableKeyConstraint(const std::set<ColumnID>& columns, const KeyConstraintType key_type, const CommitID last_validated_on = INVALID_COMMIT_ID);

and explain the semantics of the special default value.

Also, is there a reason not to use MAX_COMMIT_ID (and maybe move it's definition to types.hpp?)

dey4ss · 2023-09-28T14:55:49Z

src/lib/storage/constraints/table_key_constraint.hpp

+  CommitID last_validated_on() const;
+
+  /**
+   * Whether this key constraint can become invalid if the table data changes.


We try to fill up 120 chars.

dey4ss · 2023-09-28T14:58:47Z

src/lib/storage/constraints/table_key_constraint.hpp

+  KeyConstraintType _key_type;
+
+  /**
+   * Commit ID during which this constraint was last validated. Note that the constraint will still be valid during


on which snapshot?

dey4ss · 2023-09-28T15:03:07Z

src/lib/storage/table.cpp

+  const auto chunk_count = this->chunk_count();
+  // Iterate through the chunks backwards as inserts are more likely to happen in later chunks, potentially enabling us
+  // to return faster.
+  // Subtract 1 from the chunk_id only inside the loop to avoid underflows.


Rather start with chunk_count - 1 then and loop to >= 0?

dey4ss · 2023-09-28T15:05:00Z

src/lib/storage/table.cpp

+  _table_key_constraints.erase(constraint);
+}
+
+bool Table::constraint_guaranteed_to_be_valid(const TableKeyConstraint& table_key_constraint) const {


I see why this is a member of Table, but I'd prefer this to be a helper function in a separate file taking the table and the constraint as arguments.

dey4ss · 2023-09-28T15:27:10Z

src/plugins/ucc_discovery_plugin.cpp

+  // Check all chunks for duplicates that we haven't yet in the first loop above.
+  // Note that the loop below will only contain these chunks, as we have excluded the others when executing the GetTable
+  // operator.


Bit weird formulation. Also, fill 120 chars.

dey4ss · 2023-09-28T15:29:07Z

src/plugins/ucc_discovery_plugin.cpp

+  // operator.
+  const auto validated_chunk_count = table_view->chunk_count();
+  for (auto chunk_id = ChunkID{0}; chunk_id < validated_chunk_count; ++chunk_id) {
+    // No need to do null checks here as we already did so above


Ah, you mean chunk nullptr and not NULL values. There's no need to check after the GetTable operator (it only forwards chunks with actual data).

dey4ss · 2023-09-28T15:31:42Z

src/test/plugins/ucc_discovery_plugin_test.cpp

@@ -73,6 +76,29 @@ class UccDiscoveryPluginTest : public BaseTest {
    ChunkEncoder::encode_all_chunks(table, chunk_encoding_spec);
  }

+  void _duplicate_table(const std::string table_name) {


take a reference? Not measurable in this test case though.

dey4ss · 2023-09-28T15:36:09Z

src/test/plugins/ucc_discovery_plugin_test.cpp

+  // Delete row of _table_A that had a duplicate value regarding column 1 such that column 1 is unique afterwards.
+  _delete_row(_table_A, 3);
+
+  // We are only interested in column 1, since it was not unique before the deletion but should be now


pick here: end w/ full stop.

dey4ss · 2023-09-28T15:36:58Z

src/test/plugins/ucc_discovery_plugin_test.cpp

+  const auto& constraints_B = _table_A->soft_key_constraints();
+  EXPECT_FALSE(constraints_B.contains({{ColumnID{0}}, KeyConstraintType::UNIQUE}));
+}
+


Pick here: I'd love to see some more test cases, e.g., no CommitID ipdate for schema-given UCC, Updates (since it's delete + insert w/ and w/o duplicate), validation while transactions are active (insert/delete ops are executed, but not committed), ...

dey4ss · 2024-04-30T17:09:24Z

Put this on hold until end of August. We plan to incorporate this feature, but there is currently no capacity for it. Will work on it when back from the internship.

j-hellenberg marked this pull request as ready for review August 2, 2023 16:12

j-hellenberg added the FullCI Run all CI tests (slow, but required for merge) label Aug 2, 2023

This was referenced Aug 2, 2023

[DYOD] Maintain UCCs in face of potentially changing data #2591

Closed

[DYOD] Only use valid UCCs in optimization and limit cacheability of plans optimized using UCCs #2600

Draft

SanJSp self-requested a review August 4, 2023 16:36

SanJSp reviewed Aug 4, 2023

View reviewed changes

Base automatically changed from dey4ss/set_cids to master August 17, 2023 12:26

j-hellenberg and others added 8 commits August 22, 2023 11:20

Add validation commit ID to table key constraint

289edd7

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Test that only table key constraints with a non-default commit ID can…

cbd7cd4

… become invalid Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Enable deletion of a SoftKeyConstraint from a table

a67cce3

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Add method for checking if TableKeyConstraint is guaranteed to be val…

af81986

…id using MVCC data Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Enable UCC discovery plugin to do MVCC-aware (re)validation of UCCs

616d8ef

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Test that after data update, UCC discovery plugin recognizes valid UC…

3ff952c

…Cs becoming invalid and vice versa Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Add contributor notice :)

4c40234

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

Address review comments

b3b721c

Co-authored-by: kalathen <kalathen@jyu.fi> Co-authored-by: Margarete <margarete.dippel@student.hpi.de>

j-hellenberg force-pushed the j-hellenberg/maintain_data_dependencies branch from d214251 to b3b721c Compare August 22, 2023 09:21

dey4ss reviewed Sep 28, 2023

View reviewed changes

dey4ss closed this Apr 30, 2024

dey4ss reopened this Apr 30, 2024

dey4ss marked this pull request as draft April 30, 2024 17:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DYOD] Maintain UCCs in face of potentially changing data #2599

[DYOD] Maintain UCCs in face of potentially changing data #2599

j-hellenberg commented Aug 2, 2023 •

edited by dey4ss

SanJSp left a comment

dey4ss left a comment

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss Sep 28, 2023

dey4ss commented Apr 30, 2024

[DYOD] Maintain UCCs in face of potentially changing data #2599

Are you sure you want to change the base?

[DYOD] Maintain UCCs in face of potentially changing data #2599

Conversation

j-hellenberg commented Aug 2, 2023 • edited by dey4ss

SanJSp left a comment

Choose a reason for hiding this comment

dey4ss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dey4ss commented Apr 30, 2024

j-hellenberg commented Aug 2, 2023 •

edited by dey4ss