You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@impala.apache.org by ta...@apache.org on 2017/12/15 23:20:03 UTC

[1/3] impala git commit: IMPALA-6222: Add details to error msg on failure to get min reservation

Repository: impala
Updated Branches:
  refs/heads/master 3cbbaf3b3 -> bfbcd1fe8


IMPALA-6222: Add details to error msg on failure to get min reservation

This patch adds the following details to the error message encountered
on failure to get minimum memory reservation:
- which ReservationTracker hit its limit
- top 5 admitted queries that are consuming the most memory under the
ReservationTracker that hit its limit

Testing:
- added tests to reservation-tracker-test.cc that verify the error
message returned for different cases.
- tested "initial reservation failed" condition manually to verify
the error message returned.

Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Reviewed-on: http://gerrit.cloudera.org:8080/8781
Reviewed-by: Bikramjeet Vig <bi...@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/de299259
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/de299259
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/de299259

Branch: refs/heads/master
Commit: de29925912703ba73139ca9b34ab0e28712af45e
Parents: 3cbbaf3
Author: Bikramjeet Vig <bi...@cloudera.com>
Authored: Mon Dec 4 15:55:04 2017 -0800
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Thu Dec 14 22:34:30 2017 +0000

----------------------------------------------------------------------
 .../bufferpool/reservation-tracker-test.cc      | 69 +++++++++++++++-----
 .../runtime/bufferpool/reservation-tracker.cc   | 52 ++++++++++++---
 be/src/runtime/bufferpool/reservation-tracker.h | 33 +++++++---
 be/src/runtime/initial-reservations.cc          |  8 +--
 be/src/runtime/mem-tracker.cc                   | 34 +++++++++-
 be/src/runtime/mem-tracker.h                    | 13 ++++
 common/thrift/generate_error_codes.py           |  6 +-
 7 files changed, 176 insertions(+), 39 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/de299259/be/src/runtime/bufferpool/reservation-tracker-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/reservation-tracker-test.cc b/be/src/runtime/bufferpool/reservation-tracker-test.cc
index 3fc0e0b..b6717f4 100644
--- a/be/src/runtime/bufferpool/reservation-tracker-test.cc
+++ b/be/src/runtime/bufferpool/reservation-tracker-test.cc
@@ -21,6 +21,8 @@
 
 #include "runtime/bufferpool/reservation-tracker.h"
 #include "runtime/bufferpool/reservation-util.h"
+#include "runtime/test-env.h"
+#include "service/fe-support.h"
 #include "common/init.h"
 #include "common/object-pool.h"
 #include "runtime/mem-tracker.h"
@@ -32,9 +34,15 @@ namespace impala {
 
 class ReservationTrackerTest : public ::testing::Test {
  public:
-  virtual void SetUp() {}
+  virtual void SetUp() {
+    test_env_.reset(new TestEnv());
+    ASSERT_OK(test_env_->Init());
+    ASSERT_OK(test_env_->CreateQueryState(0, nullptr, &runtime_state_));
+  }
 
   virtual void TearDown() {
+    runtime_state_ = nullptr;
+    test_env_.reset();
     root_.Close();
     obj_pool_.Clear();
   }
@@ -51,7 +59,8 @@ class ReservationTrackerTest : public ::testing::Test {
 
   ReservationTracker root_;
 
-  scoped_ptr<RuntimeProfile> profile_;
+  scoped_ptr<TestEnv> test_env_;
+  RuntimeState* runtime_state_;
 };
 
 const int64_t ReservationTrackerTest::MIN_BUFFER_LEN;
@@ -226,9 +235,9 @@ TEST_F(ReservationTrackerTest, MemTrackerIntegrationTwoLevel) {
   // the child MemTracker. We add various limits at different places to enable testing
   // of different code paths.
   root_.InitRootTracker(NewProfile(), MIN_BUFFER_LEN * 100);
-  MemTracker root_mem_tracker;
-  MemTracker child_mem_tracker1(-1, "Child 1", &root_mem_tracker);
-  MemTracker child_mem_tracker2(MIN_BUFFER_LEN * 50, "Child 2", &root_mem_tracker);
+  MemTracker* root_mem_tracker = ExecEnv::GetInstance()->process_mem_tracker();
+  MemTracker child_mem_tracker1(-1, "Child 1", root_mem_tracker);
+  MemTracker child_mem_tracker2(MIN_BUFFER_LEN * 50, "Child 2", root_mem_tracker);
   ReservationTracker child_reservations1, child_reservations2;
   child_reservations1.InitChildTracker(
       NewProfile(), &root_, &child_mem_tracker1, 500 * MIN_BUFFER_LEN);
@@ -239,51 +248,76 @@ TEST_F(ReservationTrackerTest, MemTrackerIntegrationTwoLevel) {
   ASSERT_TRUE(child_reservations1.IncreaseReservation(MIN_BUFFER_LEN));
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations1.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker1.consumption());
-  ASSERT_EQ(MIN_BUFFER_LEN, root_mem_tracker.consumption());
+  ASSERT_EQ(MIN_BUFFER_LEN, root_mem_tracker->consumption());
   ASSERT_EQ(MIN_BUFFER_LEN, root_.GetChildReservations());
 
   // Check that a buffer reservation from the other child is accounted correctly.
   ASSERT_TRUE(child_reservations2.IncreaseReservation(MIN_BUFFER_LEN));
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations2.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker2.consumption());
-  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker.consumption());
+  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker->consumption());
   ASSERT_EQ(2 * MIN_BUFFER_LEN, root_.GetChildReservations());
 
   // Check that hitting the MemTracker limit leaves things in a consistent state.
-  ASSERT_FALSE(child_reservations2.IncreaseReservation(MIN_BUFFER_LEN * 50));
+  Status error_status;
+  string expected_error_str =
+      "Could not allocate memory while trying to increase reservation.";
+  ASSERT_FALSE(
+      child_reservations2.IncreaseReservation(MIN_BUFFER_LEN * 50, &error_status));
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations1.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker1.consumption());
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations2.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker2.consumption());
-  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker.consumption());
+  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker->consumption());
   ASSERT_EQ(2 * MIN_BUFFER_LEN, root_.GetChildReservations());
+  ASSERT_TRUE(error_status.msg().msg().find(expected_error_str) != string::npos);
 
   // Check that hitting the ReservationTracker's local limit leaves things in a
   // consistent state.
-  ASSERT_FALSE(child_reservations2.IncreaseReservation(MIN_BUFFER_LEN * 75));
+  string top_5_query_msg =
+      "The top 5 queries that allocated memory under this tracker are";
+  expected_error_str = Substitute(
+      "Failed to increase reservation by $0 because it would "
+      "exceed the applicable reservation limit for the \"$1\" ReservationTracker",
+      PrettyPrinter::Print(MIN_BUFFER_LEN * 75, TUnit::BYTES),
+      child_mem_tracker2.label());
+  ASSERT_FALSE(
+      child_reservations2.IncreaseReservation(MIN_BUFFER_LEN * 75, &error_status));
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations1.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker1.consumption());
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations2.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker2.consumption());
-  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker.consumption());
+  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker->consumption());
   ASSERT_EQ(2 * MIN_BUFFER_LEN, root_.GetChildReservations());
+  ASSERT_TRUE(error_status.msg().msg().find(expected_error_str) != string::npos);
+  // No queries registered under this tracker.
+  ASSERT_TRUE(error_status.msg().msg().find(top_5_query_msg) == string::npos);
 
   // Check that hitting the ReservationTracker's parent's limit after the
   // MemTracker consumption is incremented leaves things in a consistent state.
-  ASSERT_FALSE(child_reservations1.IncreaseReservation(MIN_BUFFER_LEN * 100));
+  expected_error_str = Substitute(
+      "Failed to increase reservation by $0 because it would "
+      "exceed the applicable reservation limit for the \"$1\" ReservationTracker",
+      PrettyPrinter::Print(MIN_BUFFER_LEN * 100, TUnit::BYTES),
+      root_mem_tracker->label());
+  ASSERT_FALSE(
+      child_reservations1.IncreaseReservation(MIN_BUFFER_LEN * 100, &error_status));
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations1.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker1.consumption());
   ASSERT_EQ(MIN_BUFFER_LEN, child_reservations2.GetReservation());
   ASSERT_EQ(MIN_BUFFER_LEN, child_mem_tracker2.consumption());
-  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker.consumption());
+  ASSERT_EQ(2 * MIN_BUFFER_LEN, root_mem_tracker->consumption());
   ASSERT_EQ(2 * MIN_BUFFER_LEN, root_.GetChildReservations());
+  ASSERT_TRUE(error_status.msg().msg().find(expected_error_str) != string::npos);
+  // A dummy query is registered under the Process tracker by the test env.
+  ASSERT_TRUE(error_status.msg().msg().find(top_5_query_msg) != string::npos);
 
   // Check that released memory is decremented from all trackers correctly.
   child_reservations1.Close();
   child_reservations2.Close();
   ASSERT_EQ(0, child_mem_tracker1.consumption());
   ASSERT_EQ(0, child_mem_tracker2.consumption());
-  ASSERT_EQ(0, root_mem_tracker.consumption());
+  ASSERT_EQ(0, root_mem_tracker->consumption());
   ASSERT_EQ(0, root_.GetUsedReservation());
   child_mem_tracker1.Close();
   child_mem_tracker2.Close();
@@ -501,4 +535,9 @@ TEST_F(ReservationTrackerTest, ReservationUtil) {
 
 }
 
-IMPALA_TEST_MAIN();
+int main(int argc, char **argv) {
+  ::testing::InitGoogleTest(&argc, argv);
+  impala::InitCommonRuntime(argc, argv, true, impala::TestInfo::BE_TEST);
+  impala::InitFeSupport(false);
+  return RUN_ALL_TESTS();
+}

http://git-wip-us.apache.org/repos/asf/impala/blob/de299259/be/src/runtime/bufferpool/reservation-tracker.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/reservation-tracker.cc b/be/src/runtime/bufferpool/reservation-tracker.cc
index b40f8b5..1c84912 100644
--- a/be/src/runtime/bufferpool/reservation-tracker.cc
+++ b/be/src/runtime/bufferpool/reservation-tracker.cc
@@ -22,6 +22,7 @@
 
 #include "common/object-pool.h"
 #include "gutil/strings/substitute.h"
+#include "runtime/exec-env.h"
 #include "runtime/mem-tracker.h"
 #include "util/dummy-runtime-profile.h"
 #include "util/runtime-profile-counters.h"
@@ -129,18 +130,18 @@ void ReservationTracker::Close() {
   initialized_ = false;
 }
 
-bool ReservationTracker::IncreaseReservation(int64_t bytes) {
+bool ReservationTracker::IncreaseReservation(int64_t bytes, Status* error_status) {
   lock_guard<SpinLock> l(lock_);
-  return IncreaseReservationInternalLocked(bytes, false, false);
+  return IncreaseReservationInternalLocked(bytes, false, false, error_status);
 }
 
-bool ReservationTracker::IncreaseReservationToFit(int64_t bytes) {
+bool ReservationTracker::IncreaseReservationToFit(int64_t bytes, Status* error_status) {
   lock_guard<SpinLock> l(lock_);
-  return IncreaseReservationInternalLocked(bytes, true, false);
+  return IncreaseReservationInternalLocked(bytes, true, false, error_status);
 }
 
-bool ReservationTracker::IncreaseReservationInternalLocked(
-    int64_t bytes, bool use_existing_reservation, bool is_child_reservation) {
+bool ReservationTracker::IncreaseReservationInternalLocked(int64_t bytes,
+    bool use_existing_reservation, bool is_child_reservation, Status* error_status) {
   DCHECK(initialized_);
   int64_t reservation_increase =
       use_existing_reservation ? max<int64_t>(0, bytes - unused_reservation()) : bytes;
@@ -156,18 +157,53 @@ bool ReservationTracker::IncreaseReservationInternalLocked(
     // Should be good enough. If the probability is 0.0, this never triggers. If it is 1.0
     // it always triggers.
     granted = false;
+    if (error_status != nullptr) {
+      *error_status = Status::Expected(
+          Substitute("Debug random failure mode is turned on: Reservation of $0 denied.",
+              PrettyPrinter::Print(bytes, TUnit::BYTES)));
+    }
   } else if (reservation_ + reservation_increase > reservation_limit_) {
     granted = false;
+    if (error_status != nullptr) {
+      MemTracker* mem_tracker = mem_tracker_;
+      if (mem_tracker == nullptr) {
+        // The ReservationTracker at the root does not have a reference to the top
+        // level(process) MemTracker.
+        mem_tracker = ExecEnv::GetInstance()->process_mem_tracker();
+      }
+      string error_msg = Substitute(
+          "Failed to increase reservation by $0 because it would exceed the applicable "
+          "reservation limit for the \"$1\" ReservationTracker: reservation_limit=$2 "
+          "reservation=$3 used_reservation=$4 child_reservations=$5",
+          PrettyPrinter::Print(bytes, TUnit::BYTES),
+          mem_tracker == nullptr ? "Process" : mem_tracker->label(),
+          PrettyPrinter::Print(reservation_limit_, TUnit::BYTES),
+          PrettyPrinter::Print(reservation_, TUnit::BYTES),
+          PrettyPrinter::Print(used_reservation_, TUnit::BYTES),
+          PrettyPrinter::Print(child_reservations_, TUnit::BYTES));
+      string top_n_queries = mem_tracker->LogTopNQueries(5);
+      if (!top_n_queries.empty()) {
+        error_msg = Substitute(
+            "$0\nThe top 5 queries that allocated memory under this tracker are:\n$1",
+            error_msg, top_n_queries);
+      }
+      *error_status = Status::Expected(error_msg);
+    }
   } else {
     if (parent_ == nullptr) {
       granted = true;
     } else {
       lock_guard<SpinLock> l(parent_->lock_);
-      granted =
-          parent_->IncreaseReservationInternalLocked(reservation_increase, true, true);
+      granted = parent_->IncreaseReservationInternalLocked(
+          reservation_increase, true, true, error_status);
     }
     if (granted && !TryConsumeFromMemTracker(reservation_increase)) {
       granted = false;
+      if (error_status != nullptr) {
+        *error_status = mem_tracker_->MemLimitExceeded(nullptr,
+            "Could not allocate memory while trying to increase reservation.",
+            reservation_increase);
+      }
       // Roll back changes to ancestors if MemTracker update fails.
       parent_->DecreaseReservation(reservation_increase, true);
     }

http://git-wip-us.apache.org/repos/asf/impala/blob/de299259/be/src/runtime/bufferpool/reservation-tracker.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/bufferpool/reservation-tracker.h b/be/src/runtime/bufferpool/reservation-tracker.h
index 4126eb7..337e12b 100644
--- a/be/src/runtime/bufferpool/reservation-tracker.h
+++ b/be/src/runtime/bufferpool/reservation-tracker.h
@@ -117,15 +117,20 @@ class ReservationTracker {
   /// ancestors' reservations if needed to fit the increased reservation.
   /// Returns true if the reservation increase is granted, or false if not granted.
   /// If the reservation is not granted, no modifications are made to the state of
-  /// any ReservationTrackers.
-  bool IncreaseReservation(int64_t bytes) WARN_UNUSED_RESULT;
+  /// any ReservationTrackers and if 'error_status' is non-null, it returns an
+  /// appropriate status message in it.
+  bool IncreaseReservation(int64_t bytes, Status* error_status = nullptr)
+      WARN_UNUSED_RESULT;
 
   /// Tries to ensure that 'bytes' of unused reservation is available. If not already
   /// available, tries to increase the reservation such that the unused reservation is
   /// exactly equal to 'bytes'. Uses any unused reservation on ancestors and increase
   /// ancestors' reservations if needed to fit the increased reservation.
-  /// Returns true if the reservation increase was successful or not necessary.
-  bool IncreaseReservationToFit(int64_t bytes) WARN_UNUSED_RESULT;
+  /// Returns true if the reservation increase was successful or not necessary. Otherwise
+  /// returns false and if 'error_status' is non-null, it returns an appropriate status
+  /// message in it.
+  bool IncreaseReservationToFit(int64_t bytes, Status* error_status = nullptr)
+      WARN_UNUSED_RESULT;
 
   /// Decrease reservation by 'bytes' on this tracker and all ancestors. This tracker's
   /// reservation must be at least 'bytes' before calling this method.
@@ -190,10 +195,18 @@ class ReservationTracker {
   /// Internal helper for IncreaseReservation(). If 'use_existing_reservation' is true,
   /// increase by the minimum amount so that 'bytes' fits in the reservation, otherwise
   /// just increase by 'bytes'. If 'is_child_reservation' is true, also increase
-  /// 'child_reservations_' by 'bytes'.
+  /// 'child_reservations_' by 'bytes'. If 'error_status' is not null and reservation
+  /// increase fails then an appropriate status message is returned in it.
   /// 'lock_' must be held by caller.
-  bool IncreaseReservationInternalLocked(
-      int64_t bytes, bool use_existing_reservation, bool is_child_reservation);
+  /// Example error message if a reservation tracker hits its limit:
+  /// Failed to increase reservation by 2.12 GB because it would exceed the applicable
+  /// reservation limit for the "Process" ReservationTracker: reservation_limit=2.00 GB
+  /// reservation=0 used_reservation=0 child_reservations=0
+  /// The top 5 queries that allocated memory under this tracker are:
+  /// Query(20449659107d67ce:2e9058b500000000): Reservation=0 ReservationLimit=6.67 GB
+  /// OtherMemory=0 Total=0 Peak=0
+  bool IncreaseReservationInternalLocked(int64_t bytes, bool use_existing_reservation,
+      bool is_child_reservation, Status* error_status = nullptr);
 
   /// Increase consumption on linked MemTracker to reflect an increase in reservation
   /// of 'reservation_increase'. For the topmost link, return false if this failed
@@ -248,12 +261,16 @@ class ReservationTracker {
   /// based on a post-order traversal of the tree, with children visited in order of the
   /// memory address of the ReservationTracker object. The following rules can be applied
   /// to determine the relative positions of two trackers t1 and t2 in the lock order:
-  /// * If t1 is a descendent of t2, t1's lock must be acquired before t2's lock (i.e.
+  /// * If t1 is a descendant of t2, t1's lock must be acquired before t2's lock (i.e.
   ///   locks are acquired bottom-up).
   /// * If neither t1 or t2 is a descendant of the other, they must be in subtrees of
   ///   under a common ancestor. If the memory address of t1's subtree's root is less
   ///   than the memory address of t2's subtree's root, t1's lock must be acquired before
   ///   t2's lock. This check is implemented in lock_sibling_subtree_first().
+  /// Since MemTracker::child_trackers_lock_ objects are acquired in a top-down lock
+  /// order, if a MemTracker::child_trackers_lock_ is acquired while holding a lock_, any
+  /// more calls to acquire a lock_ should not be made to avoid any deadlock that might
+  /// occur due to ReservationTracker's bottom-up lock order.
   SpinLock lock_;
 
   /// True if the tracker is initialized.

http://git-wip-us.apache.org/repos/asf/impala/blob/de299259/be/src/runtime/initial-reservations.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/initial-reservations.cc b/be/src/runtime/initial-reservations.cc
index 39810ae..0fe00fe 100644
--- a/be/src/runtime/initial-reservations.cc
+++ b/be/src/runtime/initial-reservations.cc
@@ -50,12 +50,12 @@ InitialReservations::InitialReservations(ObjectPool* obj_pool,
 Status InitialReservations::Init(
     const TUniqueId& query_id, int64_t query_min_reservation) {
   DCHECK_EQ(0, initial_reservations_.GetReservation()) << "Already inited";
-  if (!initial_reservations_.IncreaseReservation(query_min_reservation)) {
+  Status reservation_status;
+  if (!initial_reservations_.IncreaseReservation(
+          query_min_reservation, &reservation_status)) {
     return Status(TErrorCode::MINIMUM_RESERVATION_UNAVAILABLE,
         PrettyPrinter::Print(query_min_reservation, TUnit::BYTES), FLAGS_hostname,
-        FLAGS_be_port, PrintId(query_id),
-        ExecEnv::GetInstance()->process_mem_tracker()->LogUsage(
-            MemTracker::PROCESS_MEMTRACKER_LIMITED_DEPTH));
+        FLAGS_be_port, PrintId(query_id), reservation_status.GetDetail());
   }
   VLOG_QUERY << "Successfully claimed initial reservations ("
             << PrettyPrinter::Print(query_min_reservation, TUnit::BYTES) << ") for"

http://git-wip-us.apache.org/repos/asf/impala/blob/de299259/be/src/runtime/mem-tracker.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/mem-tracker.cc b/be/src/runtime/mem-tracker.cc
index 4162e8a..98f45db 100644
--- a/be/src/runtime/mem-tracker.cc
+++ b/be/src/runtime/mem-tracker.cc
@@ -34,6 +34,8 @@
 #include "common/names.h"
 
 using boost::algorithm::join;
+using std::priority_queue;
+using std::greater;
 using namespace strings;
 
 namespace impala {
@@ -301,7 +303,6 @@ string MemTracker::LogUsage(int max_recursive_depth, const string& prefix,
        << new_prefix << "Untracked Memory: Total="
        << PrettyPrinter::Print(untracked_bytes, TUnit::BYTES);
   }
-
   return ss.str();
 }
 
@@ -319,6 +320,37 @@ string MemTracker::LogUsage(int max_recursive_depth, const string& prefix,
   return join(usage_strings, "\n");
 }
 
+string MemTracker::LogTopNQueries(int limit) {
+  if (limit == 0) return "";
+  if (this->is_query_mem_tracker_) return LogUsage(0);
+  priority_queue<pair<int64_t, string>, vector<pair<int64_t, string>>,
+      std::greater<pair<int64_t, string>>>
+      min_pq;
+  GetTopNQueries(min_pq, limit);
+  vector<string> usage_strings(min_pq.size());
+  while (!min_pq.empty()) {
+    usage_strings.push_back(min_pq.top().second);
+    min_pq.pop();
+  }
+  std::reverse(usage_strings.begin(), usage_strings.end());
+  return join(usage_strings, "\n");
+}
+
+void MemTracker::GetTopNQueries(
+    priority_queue<pair<int64_t, string>, vector<pair<int64_t, string>>,
+        greater<pair<int64_t, string>>>& min_pq,
+    int limit) {
+  lock_guard<SpinLock> l(child_trackers_lock_);
+  for (MemTracker* tracker : child_trackers_) {
+    if (!tracker->is_query_mem_tracker_) {
+      tracker->GetTopNQueries(min_pq, limit);
+    } else {
+      min_pq.push(pair<int64_t, string>(tracker->consumption(), tracker->LogUsage(0)));
+      if (min_pq.size() > limit) min_pq.pop();
+    }
+  }
+}
+
 MemTracker* MemTracker::GetQueryMemTracker() {
   MemTracker* tracker = this;
   while (tracker != nullptr && !tracker->is_query_mem_tracker_) {

http://git-wip-us.apache.org/repos/asf/impala/blob/de299259/be/src/runtime/mem-tracker.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/mem-tracker.h b/be/src/runtime/mem-tracker.h
index 539f973..fb1cd90 100644
--- a/be/src/runtime/mem-tracker.h
+++ b/be/src/runtime/mem-tracker.h
@@ -22,6 +22,7 @@
 #include <stdint.h>
 #include <map>
 #include <memory>
+#include <queue>
 #include <vector>
 #include <boost/thread/mutex.hpp>
 #include <boost/unordered_map.hpp>
@@ -344,6 +345,10 @@ class MemTracker {
   /// are not performance sensitive
   static const int UNLIMITED_DEPTH = INT_MAX;
 
+  /// Logs the usage of 'limit' number of queries based on maximum total memory
+  /// consumption.
+  std::string LogTopNQueries(int limit);
+
   /// Log the memory usage when memory limit is exceeded and return a status object with
   /// details of the allocation which caused the limit to be exceeded.
   /// If 'failed_allocation_size' is greater than zero, logs the allocation size. If
@@ -382,6 +387,14 @@ class MemTracker {
   static std::string LogUsage(int max_recursive_depth, const std::string& prefix,
       const std::list<MemTracker*>& trackers, int64_t* logged_consumption);
 
+  /// Helper function for LogTopNQueries that iterates through the MemTracker hierarchy
+  /// and populates 'min_pq' with 'limit' number of elements (that contain state related
+  /// to query MemTrackers) based on maximum total memory consumption.
+  void GetTopNQueries(
+      std::priority_queue<pair<int64_t, string>,
+          vector<pair<int64_t, string>>, std::greater<pair<int64_t, string>>>& min_pq,
+      int limit);
+
   /// If an ancestor of this tracker is a query MemTracker, return that tracker.
   /// Otherwise return NULL.
   MemTracker* GetQueryMemTracker();

http://git-wip-us.apache.org/repos/asf/impala/blob/de299259/common/thrift/generate_error_codes.py
----------------------------------------------------------------------
diff --git a/common/thrift/generate_error_codes.py b/common/thrift/generate_error_codes.py
index 5aec1ce..bdeb1ed 100755
--- a/common/thrift/generate_error_codes.py
+++ b/common/thrift/generate_error_codes.py
@@ -323,9 +323,9 @@ error_codes = (
    "Failed to verify generated IR function $0, see log for more details."),
 
   ("MINIMUM_RESERVATION_UNAVAILABLE", 106, "Failed to get minimum memory reservation of "
-     "$0 on daemon $1:$2 for query $3 because it would exceed an applicable memory "
-     "limit. Memory is likely oversubscribed. Reducing query concurrency or configuring "
-     "admission control may help avoid this error. Memory usage:\\n$4"),
+     "$0 on daemon $1:$2 for query $3 due to following error: $4Memory is likely "
+     "oversubscribed. Reducing query concurrency or configuring admission control may "
+     "help avoid this error."),
 
   ("ADMISSION_REJECTED", 107, "Rejected query from pool $0: $1"),
 


[2/3] impala git commit: IMPALA-6114: Require type equality for NumericLiteral::localEquals().

Posted by ta...@apache.org.
IMPALA-6114: Require type equality for NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals.

A test case has been added to AnalyzeStmtsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Reviewed-on: http://gerrit.cloudera.org:8080/8448
Reviewed-by: Alex Behm <al...@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/f4eb0012
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/f4eb0012
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/f4eb0012

Branch: refs/heads/master
Commit: f4eb00123ff6be770bf0fb75f71b6468cb5e7085
Parents: de29925
Author: Zoram Thanga <zo...@cloudera.com>
Authored: Wed Nov 1 14:43:47 2017 -0700
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Dec 15 01:28:13 2017 +0000

----------------------------------------------------------------------
 .../main/java/org/apache/impala/analysis/NumericLiteral.java | 8 +++++++-
 .../java/org/apache/impala/analysis/AnalyzeStmtsTest.java    | 4 ++++
 2 files changed, 11 insertions(+), 1 deletion(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/f4eb0012/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java b/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
index 80d2347..b720778 100644
--- a/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
+++ b/fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
@@ -132,7 +132,13 @@ public class NumericLiteral extends LiteralExpr {
 
   @Override
   public boolean localEquals(Expr that) {
-    return super.localEquals(that) && ((NumericLiteral) that).value_.equals(value_);
+    if (!super.localEquals(that)) return false;
+
+    NumericLiteral tmp = (NumericLiteral) that;
+    if (!tmp.value_.equals(value_)) return false;
+    // Analyzed Numeric literals of different types are distinct.
+    if ((isAnalyzed() && tmp.isAnalyzed()) && (!getType().equals(tmp.getType()))) return false;
+    return true;
   }
 
   @Override

http://git-wip-us.apache.org/repos/asf/impala/blob/f4eb0012/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
index 133b6e2..f9d5cf5 100644
--- a/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
+++ b/fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
@@ -2052,6 +2052,10 @@ public class AnalyzeStmtsTest extends AnalyzerTest {
     AnalyzesOk("select tinyint_col, count(distinct int_col),"
         + "min(distinct smallint_col), max(distinct string_col) "
         + "from functional.alltypesagg group by 1");
+
+    // IMPALA-6114: Test that numeric literals having the same value, but different types are
+    // considered distinct.
+    AnalyzesOk("select distinct cast(0 as decimal(14)), 0 from functional.alltypes");
   }
 
   @Test


[3/3] impala git commit: IMPALA-4664: Unexpected string conversion in Shell

Posted by ta...@apache.org.
IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Reviewed-on: http://gerrit.cloudera.org:8080/8762
Reviewed-by: Zoltan Borok-Nagy <bo...@cloudera.com>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Tested-by: Impala Public Jenkins


Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/bfbcd1fe
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/bfbcd1fe
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/bfbcd1fe

Branch: refs/heads/master
Commit: bfbcd1fe86e4161b9419b3a0c653d0327d3aacdb
Parents: f4eb001
Author: Jinchul <ji...@gmail.com>
Authored: Tue Dec 5 23:14:24 2017 +0900
Committer: Impala Public Jenkins <im...@gerrit.cloudera.org>
Committed: Fri Dec 15 21:32:20 2017 +0000

----------------------------------------------------------------------
 LICENSE.txt                            |   1 +
 bin/rat_exclude_files.txt              |   2 +
 shell/impala_shell.py                  | 100 ++++++++++++++++------------
 tests/shell/shell_case_sensitive.cmds  |   3 +
 tests/shell/shell_case_sensitive2.cmds |   1 +
 tests/shell/test_shell_interactive.py  |  60 ++++++++++++++---
 6 files changed, 118 insertions(+), 49 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/impala/blob/bfbcd1fe/LICENSE.txt
----------------------------------------------------------------------
diff --git a/LICENSE.txt b/LICENSE.txt
index d4c4307..1f6e222 100644
--- a/LICENSE.txt
+++ b/LICENSE.txt
@@ -398,6 +398,7 @@ www/DataTables* and www/datatables*: MIT license
 
 shell/pkg_resources.py: Python Software License V2
 Parts of be/src/runtime/string-search.h: Python Software License V2
+Parts of shell/impala_shell.py: Python Software License V2
 
   Copyright (c) 2001 - 2016 Python Software Foundation; All Rights Reserved
 

http://git-wip-us.apache.org/repos/asf/impala/blob/bfbcd1fe/bin/rat_exclude_files.txt
----------------------------------------------------------------------
diff --git a/bin/rat_exclude_files.txt b/bin/rat_exclude_files.txt
index 30ebc26..5bb13f0 100644
--- a/bin/rat_exclude_files.txt
+++ b/bin/rat_exclude_files.txt
@@ -117,6 +117,8 @@ tests/shell/impalarc_with_query_options
 tests/shell/impalarc_with_warnings
 tests/shell/shell.cmds
 tests/shell/shell2.cmds
+tests/shell/shell_case_sensitive.cmds
+tests/shell/shell_case_sensitive2.cmds
 tests/shell/shell_error.cmds
 tests/shell/test_close_queries.sql
 tests/shell/test_file_comments.sql

http://git-wip-us.apache.org/repos/asf/impala/blob/bfbcd1fe/shell/impala_shell.py
----------------------------------------------------------------------
diff --git a/shell/impala_shell.py b/shell/impala_shell.py
index 36f8420..93e10fc 100755
--- a/shell/impala_shell.py
+++ b/shell/impala_shell.py
@@ -175,6 +175,7 @@ class ImpalaShell(object, cmd.Cmd):
     self._populate_command_list()
 
     self.imp_client = None;
+    self.orig_cmd = None
 
     # Tracks query handle of the last query executed. Used by the 'profile' command.
     self.last_query_handle = None;
@@ -300,6 +301,18 @@ class ImpalaShell(object, cmd.Cmd):
     for x in self.VALID_SHELL_OPTIONS:
       print "\t%s: %s" % (x, self.__dict__[self.VALID_SHELL_OPTIONS[x][1]])
 
+  def _create_beeswax_query(self, args):
+    """Original command should be stored before running the method. The method is usually
+    used in do_* methods and the command is kept at precmd()."""
+    command = self.orig_cmd
+    self.orig_cmd = None
+    if not command:
+      print_to_stderr("Unexpected error: Failed to execute query due to command "\
+                      "is missing")
+      sys.exit(1)
+    return self.imp_client.create_beeswax_query("%s %s" % (command, args),
+                                                 self.set_query_options)
+
   def do_shell(self, args):
     """Run a command on the shell
     Usage: shell <cmd>
@@ -323,19 +336,16 @@ class ImpalaShell(object, cmd.Cmd):
     return regexp.sub(ImpalaShell.COMMENTS_BEFORE_SET_REPLACEMENT, line, 1)
 
   def sanitise_input(self, args):
-    """Convert the command to lower case, so it's recognized"""
     # A command terminated by a semi-colon is legal. Check for the trailing
     # semi-colons and strip them from the end of the command.
     if not self.interactive:
       # Strip all the non-interactive commands of the delimiter.
       args = self._remove_comments_before_set(args)
       tokens = args.strip().split(' ')
-      tokens[0] = tokens[0].lower()
       return ' '.join(tokens).rstrip(ImpalaShell.CMD_DELIM)
     # Handle EOF if input is interactive
     tokens = args.strip().split(' ')
-    tokens[0] = tokens[0].lower()
-    if tokens[0] == 'eof':
+    if tokens[0].lower() == 'eof':
       if not self.partial_cmd:
         # The first token is the command.
         # If it's EOF, call do_quit()
@@ -350,14 +360,9 @@ class ImpalaShell(object, cmd.Cmd):
         # been cancelled.
         print '\n'
         return str()
-    # The first token is converted into lower case to route it to the
-    # appropriate command handler. This only applies to the first line of user input.
-    # Modifying tokens in subsequent lines may change the semantics of the command,
-    # so do not modify the text.
     args = self._check_for_command_completion(args)
     args = self._remove_comments_before_set(args)
     tokens = args.strip().split(' ')
-    tokens[0] = tokens[0].lower()
     args = ' '.join(tokens).strip()
     return args.rstrip(ImpalaShell.CMD_DELIM)
 
@@ -561,7 +566,29 @@ class ImpalaShell(object, cmd.Cmd):
     if line == None:
       return CmdStatus.ERROR
     else:
-      return cmd.Cmd.onecmd(self, line)
+      # This code is based on the code from the standard Python library package cmd.py:
+      # https://github.com/python/cpython/blob/master/Lib/cmd.py#L192
+      # One change is lowering command before getting a function. The lowering
+      # is necessary to find a proper function and here is a right place
+      # because the lowering command in front of the finding can avoid a
+      # side effect.
+      command, arg, line = self.parseline(line)
+      if not line:
+        return self.emptyline()
+      if command is None:
+        return self.default(line)
+      self.lastcmd = line
+      if line == 'EOF' :
+        self.lastcmd = ''
+      if command == '':
+        return self.default(line)
+      else:
+        try:
+          func = getattr(self, 'do_' + command.lower())
+          self.orig_cmd = command
+        except AttributeError:
+          return self.default(line)
+        return func(arg)
 
   def postcmd(self, status, args):
     # status conveys to shell how the shell should continue execution
@@ -843,25 +870,21 @@ class ImpalaShell(object, cmd.Cmd):
       return db_table_name
 
   def do_alter(self, args):
-    query = self.imp_client.create_beeswax_query("alter %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query)
 
   def do_create(self, args):
     # We want to print the webserver link only for CTAS queries.
     print_web_link = "select" in args
-    query = self.imp_client.create_beeswax_query("create %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query, print_web_link=print_web_link)
 
   def do_drop(self, args):
-    query = self.imp_client.create_beeswax_query("drop %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query)
 
   def do_load(self, args):
-    query = self.imp_client.create_beeswax_query("load %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query)
 
   def do_profile(self, args):
@@ -877,8 +900,7 @@ class ImpalaShell(object, cmd.Cmd):
 
   def do_select(self, args):
     """Executes a SELECT... query, fetching all rows"""
-    query = self.imp_client.create_beeswax_query("select %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query, print_web_link=True)
 
   def do_compute(self, args):
@@ -886,8 +908,7 @@ class ImpalaShell(object, cmd.Cmd):
     Impala shell cannot get child query handle so it cannot
     query live progress for COMPUTE STATS query. Disable live
     progress/summary callback for COMPUTE STATS query."""
-    query = self.imp_client.create_beeswax_query("compute %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     (prev_print_progress, prev_print_summary) = self.print_progress, self.print_summary
     (self.print_progress, self.print_summary) = False, False;
     try:
@@ -1085,14 +1106,12 @@ class ImpalaShell(object, cmd.Cmd):
 
   def do_values(self, args):
     """Executes a VALUES(...) query, fetching all rows"""
-    query = self.imp_client.create_beeswax_query("values %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query)
 
   def do_with(self, args):
     """Executes a query with a WITH clause, fetching all rows"""
-    query = self.imp_client.create_beeswax_query("with %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     # Set posix=True and add "'" to escaped quotes
     # to deal with escaped quotes in string literals
     lexer = shlex.shlex(query.query.lstrip(), posix=True)
@@ -1106,8 +1125,7 @@ class ImpalaShell(object, cmd.Cmd):
 
   def do_use(self, args):
     """Executes a USE... query"""
-    query = self.imp_client.create_beeswax_query("use %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     if self._execute_stmt(query) is CmdStatus.SUCCESS:
       self.current_db = args
     else:
@@ -1115,41 +1133,41 @@ class ImpalaShell(object, cmd.Cmd):
 
   def do_show(self, args):
     """Executes a SHOW... query, fetching all rows"""
-    query = self.imp_client.create_beeswax_query("show %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query)
 
   def do_describe(self, args):
     """Executes a DESCRIBE... query, fetching all rows"""
-    query = self.imp_client.create_beeswax_query("describe %s" % args,
-                                                 self.set_query_options)
+    # original command should be overridden because the server cannot
+    # recognize "desc" as a keyword. Thus, given command should be
+    # replaced with "describe" here.
+    self.orig_cmd = "describe"
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query)
 
   def do_desc(self, args):
     return self.do_describe(args)
 
-  def __do_dml(self, stmt, args):
+  def __do_dml(self, args):
     """Executes a DML query"""
-    query = self.imp_client.create_beeswax_query("%s %s" % (stmt, args),
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query, is_dml=True, print_web_link=True)
 
   def do_upsert(self, args):
-    return self.__do_dml("upsert", args)
+    return self.__do_dml(args)
 
   def do_update(self, args):
-    return self.__do_dml("update", args)
+    return self.__do_dml(args)
 
   def do_delete(self, args):
-    return self.__do_dml("delete", args)
+    return self.__do_dml(args)
 
   def do_insert(self, args):
-    return self.__do_dml("insert", args)
+    return self.__do_dml(args)
 
   def do_explain(self, args):
     """Explain the query execution plan"""
-    query = self.imp_client.create_beeswax_query("explain %s" % args,
-                                                 self.set_query_options)
+    query = self._create_beeswax_query(args)
     return self._execute_stmt(query)
 
   def do_history(self, args):

http://git-wip-us.apache.org/repos/asf/impala/blob/bfbcd1fe/tests/shell/shell_case_sensitive.cmds
----------------------------------------------------------------------
diff --git a/tests/shell/shell_case_sensitive.cmds b/tests/shell/shell_case_sensitive.cmds
new file mode 100644
index 0000000..afe4a88
--- /dev/null
+++ b/tests/shell/shell_case_sensitive.cmds
@@ -0,0 +1,3 @@
+uSe FUNCTIONAL;
+ShOw TABLES;
+SoUrCe shell_case_sensitive2.cmds;

http://git-wip-us.apache.org/repos/asf/impala/blob/bfbcd1fe/tests/shell/shell_case_sensitive2.cmds
----------------------------------------------------------------------
diff --git a/tests/shell/shell_case_sensitive2.cmds b/tests/shell/shell_case_sensitive2.cmds
new file mode 100644
index 0000000..b2df97f
--- /dev/null
+++ b/tests/shell/shell_case_sensitive2.cmds
@@ -0,0 +1 @@
+SeLeCt VERSION();

http://git-wip-us.apache.org/repos/asf/impala/blob/bfbcd1fe/tests/shell/test_shell_interactive.py
----------------------------------------------------------------------
diff --git a/tests/shell/test_shell_interactive.py b/tests/shell/test_shell_interactive.py
index aa5db9f..a104809 100755
--- a/tests/shell/test_shell_interactive.py
+++ b/tests/shell/test_shell_interactive.py
@@ -308,11 +308,11 @@ class TestImpalaShellInteractive(object):
       os.chdir("%s/tests/shell/" % os.environ['IMPALA_HOME'])
       # IMPALA-5416: Test that a command following 'source' won't be run twice.
       result = run_impala_shell_interactive("source shell.cmds;select \"second command\";")
-      assert "Query: use FUNCTIONAL" in result.stderr
-      assert "Query: show TABLES" in result.stderr
+      assert "Query: USE FUNCTIONAL" in result.stderr
+      assert "Query: SHOW TABLES" in result.stderr
       assert "alltypes" in result.stdout
       # This is from shell2.cmds, the result of sourcing a file from a sourced file.
-      assert "select VERSION()" in result.stderr
+      assert "SELECT VERSION()" in result.stderr
       assert "version()" in result.stdout
       assert len(re.findall("'second command'", result.stdout)) == 1
       # IMPALA-5416: Test that two source commands on a line won't crash the shell.
@@ -325,13 +325,13 @@ class TestImpalaShellInteractive(object):
   def test_source_file_with_errors(self):
     full_path = "%s/tests/shell/shell_error.cmds" % os.environ['IMPALA_HOME']
     result = run_impala_shell_interactive("source %s;" % full_path)
-    assert "Could not execute command: use UNKNOWN_DATABASE" in result.stderr
-    assert "Query: use FUNCTIONAL" not in result.stderr
+    assert "Could not execute command: USE UNKNOWN_DATABASE" in result.stderr
+    assert "Query: USE FUNCTIONAL" not in result.stderr
 
     result = run_impala_shell_interactive("source %s;" % full_path, '-c')
-    assert "Could not execute command: use UNKNOWN_DATABASE" in result.stderr
-    assert "Query: use FUNCTIONAL" in result.stderr
-    assert "Query: show TABLES" in result.stderr
+    assert "Could not execute command: USE UNKNOWN_DATABASE" in result.stderr
+    assert "Query: USE FUNCTIONAL" in result.stderr
+    assert "Query: SHOW TABLES" in result.stderr
     assert "alltypes" in result.stdout
 
   @pytest.mark.execute_serially
@@ -390,6 +390,50 @@ class TestImpalaShellInteractive(object):
     assert "DEBUG_ACTION" in development_part
     assert "ABORT_ON_DEFAULT_LIMIT_EXCEEDED" in result.stdout[deprecated_part_start_idx:]
 
+  def check_command_case_sensitivity(self, command, expected):
+    shell = ImpalaShell()
+    shell.send_cmd(command)
+    assert expected in shell.get_result().stderr
+
+  @pytest.mark.execute_serially
+  def test_unexpected_conversion_for_literal_string_to_lowercase(self):
+    # IMPALA-4664: Impala shell can accidentally convert certain literal
+    # strings to lowercase. Impala shell splits each command into tokens
+    # and then converts the first token to lowercase to figure out how it
+    # should execute the command. The splitting is done by spaces only.
+    # Thus, if the user types a TAB after the SELECT, the first token after
+    # the split becomes the SELECT plus whatever comes after it.
+    result = run_impala_shell_interactive("select'MUST_HAVE_UPPER_STRING'")
+    assert re.search('MUST_HAVE_UPPER_STRING', result.stdout)
+    result = run_impala_shell_interactive("select\t'MUST_HAVE_UPPER_STRING'")
+    assert re.search('MUST_HAVE_UPPER_STRING', result.stdout)
+    result = run_impala_shell_interactive("select\n'MUST_HAVE_UPPER_STRING'")
+    assert re.search('MUST_HAVE_UPPER_STRING', result.stdout)
+
+  @pytest.mark.execute_serially
+  def test_case_sensitive_command(self):
+    # IMPALA-2640: Make a given command case-sensitive
+    cwd = os.getcwd()
+    try:
+      self.check_command_case_sensitivity("sElEcT VERSION()", "Query: sElEcT")
+      self.check_command_case_sensitivity("sEt VaR:FoO=bOo", "Variable FOO")
+      self.check_command_case_sensitivity("sHoW tables", "Query: sHoW")
+      # Change working dir so that SOURCE command in shell_case_sensitive.cmds can
+      # find shell_case_sensitive2.cmds.
+      os.chdir("%s/tests/shell/" % os.environ['IMPALA_HOME'])
+      result = run_impala_shell_interactive(
+        "sOuRcE shell_case_sensitive.cmds; SeLeCt 'second command'")
+      print result.stderr
+      assert "Query: uSe FUNCTIONAL" in result.stderr
+      assert "Query: ShOw TABLES" in result.stderr
+      assert "alltypes" in result.stdout
+      # This is from shell_case_sensitive2.cmds, the result of sourcing a file
+      # from a sourced file.
+      print result.stderr
+      assert "SeLeCt 'second command'" in result.stderr
+    finally:
+      os.chdir(cwd)
+
 def run_impala_shell_interactive(input_lines, shell_args=None):
   """Runs a command in the Impala shell interactively."""
   # if argument "input_lines" is a string, makes it into a list