Merge pull request #1266 from poojanilangekar/attribute-order-fix

Fixes the attribute reordering in update statements

Author: pmenon

src/codegen/operator/update_translator.cpp

@@ -41,13 +41,14 @@ UpdateTranslator::UpdateTranslator(const planner::UpdatePlan &update_plan,
       UpdaterProxy::GetType(GetCodeGen()));
 }
 
-bool IsTarget(const TargetList &target_list, uint32_t index) {
-  for (const auto &target : target_list) {
-    if (target.first == index) {
-      return true;
+oid_t GetTargetIndex(const TargetList &target_list, uint32_t index) {
+  oid_t target_size = target_list.size();
+  for (oid_t i = 0; i < target_size; i++) {
+    if (target_list[i].first == index) {
+      return i;
     }
   }
-  return false;
+  return INVALID_OID;
 }
 
 void UpdateTranslator::InitializeState() {
@@ -93,13 +94,13 @@ void UpdateTranslator::Consume(ConsumerContext &, RowBatch::Row &row) const {
 
   // Collect all the column values
   std::vector<codegen::Value> values;
-  for (uint32_t i = 0, target_id = 0; i < column_num; i++) {
+  for (uint32_t i = 0; i < column_num; i++) {
     codegen::Value val;
-    if (IsTarget(target_list, i)) {
+    uint32_t target_index = GetTargetIndex(target_list, i);
+    if (target_index != INVALID_OID) {
       // Set the value for the update
-      const auto &derived_attribute = target_list[target_id].second;
+      const auto &derived_attribute = target_list[target_index].second;
       val = row.DeriveValue(codegen, *derived_attribute.expr);
-      target_id++;
     } else {
       val = row.DeriveValue(codegen, ais[i]);
     }

test/sql/update_sql_test.cpp

@@ -527,5 +527,111 @@ TEST_F(UpdateSQLTests, MultiTileGroupUpdateSQLTest) {
   txn_manager.CommitTransaction(txn);
 }
 
+TEST_F(UpdateSQLTests, AttributeOrderUpdateSQLTest) {
+  // This test updates the attributes of the table in different orders.
+  // It ensure that the updates in the  data_table are in the correct
+  // order irrespective of the order of attributes in the update statement.
+
+  LOG_DEBUG("Bootstrapping...");
+
+  auto catalog = catalog::Catalog::GetInstance();
+  auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
+  auto txn = txn_manager.BeginTransaction();
+  catalog->CreateDatabase(DEFAULT_DB_NAME, txn);
+  txn_manager.CommitTransaction(txn);
+
+  LOG_DEBUG("Bootstrapping completed!");
+
+  txn = txn_manager.BeginTransaction();
+  // Create a table first
+  LOG_DEBUG("Creating a table...");
+  LOG_DEBUG("Query: CREATE TABLE test(a INT, b INT)");
+
+  TestingSQLUtil::ExecuteSQLQuery("CREATE TABLE test(a INT, b INT);");
+  txn_manager.CommitTransaction(txn);
+
+  LOG_DEBUG("Table created!");
+
+  txn = txn_manager.BeginTransaction();
+  // Insert a tuple into table
+  LOG_DEBUG("Inserting a tuple...");
+
+  LOG_DEBUG("Query: INSERT INTO test VALUES (1, 100)");
+  TestingSQLUtil::ExecuteSQLQuery("INSERT INTO test VALUES (1, 100);");
+
+  txn_manager.CommitTransaction(txn);
+  LOG_DEBUG("Tuple inserted!");
+
+  txn = txn_manager.BeginTransaction();
+  // Update a tuple in the table in the order (b, a)
+  LOG_DEBUG("Updating a tuple...");
+  LOG_DEBUG("Query: UPDATE test SET b = b * 2, a = a * 2");
+
+  std::vector<ResultValue> result;
+  std::vector<FieldInfo> tuple_descriptor;
+  std::string error_message;
+  int rows_affected;
+
+  TestingSQLUtil::ExecuteSQLQuery("UPDATE test SET b = b * 2, a = a * 2;",
+                                  result, tuple_descriptor, rows_affected,
+                                  error_message);
+
+  txn_manager.CommitTransaction(txn);
+  // Check the return value
+  EXPECT_EQ(1, rows_affected);
+  LOG_DEBUG("Tuple Update successful!");
+
+  // Check the updated values.
+  txn = txn_manager.BeginTransaction();
+  // Update a tuple in the table in the order (b, a)
+  LOG_DEBUG("Selecting the updated tuple...");
+  LOG_DEBUG("Query: SELECT a, b FROM test;");
+
+  TestingSQLUtil::ExecuteSQLQuery("SELECT a, b FROM test;", result,
+                                  tuple_descriptor, rows_affected,
+                                  error_message);
+
+  txn_manager.CommitTransaction(txn);
+  // Check the return value
+  EXPECT_EQ(TestingSQLUtil::GetResultValueAsString(result, 0), "2");
+  EXPECT_EQ(TestingSQLUtil::GetResultValueAsString(result, 1), "200");
+  LOG_DEBUG("Attributes updated in the correct order!");
+
+  txn = txn_manager.BeginTransaction();
+  // Update a tuple in the table in the order (a, b)
+  LOG_DEBUG("Updating a tuple again...");
+  LOG_DEBUG("Query: UPDATE test SET a = a * 2, b = b * 2");
+
+  TestingSQLUtil::ExecuteSQLQuery("UPDATE test SET a = a * 2, b = b * 2;",
+                                  result, tuple_descriptor, rows_affected,
+                                  error_message);
+
+  txn_manager.CommitTransaction(txn);
+  // Check the return value
+  EXPECT_EQ(1, rows_affected);
+  LOG_DEBUG("Tuple Update successful, agin!");
+
+  // Check the updated values.
+  txn = txn_manager.BeginTransaction();
+  // Update a tuple in the table in the order (b, a)
+  LOG_DEBUG("Selecting the updated tuple...");
+  LOG_DEBUG("Query: SELECT a, b FROM test;");
+
+  TestingSQLUtil::ExecuteSQLQuery("SELECT a, b FROM test;", result,
+                                  tuple_descriptor, rows_affected,
+                                  error_message);
+
+  txn_manager.CommitTransaction(txn);
+  // Check the return value
+  EXPECT_EQ(TestingSQLUtil::GetResultValueAsString(result, 0), "4");
+  EXPECT_EQ(TestingSQLUtil::GetResultValueAsString(result, 1), "400");
+  LOG_DEBUG("Attributes updated in the correct order, again!");
+
+  // free the database just created
+  txn = txn_manager.BeginTransaction();
+  catalog::Catalog::GetInstance()->DropDatabaseWithName(DEFAULT_DB_NAME, txn);
+  txn_manager.CommitTransaction(txn);
+}
+
 }  // namespace test
 }  // namespace peloton