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 2018/10/04 21:52:41 UTC
[1/4] impala git commit: IMPALA-6634: [DOCS] Whitespace behavior in
string to timestamp conversion
Repository: impala
Updated Branches:
refs/heads/master e1d1b4f14 -> 93606e604
IMPALA-6634: [DOCS] Whitespace behavior in string to timestamp conversion
Noted that leading and trailing whitespaces are ignored when a string is
implicitly or explicitly converted to a timestamp.
Change-Id: Id5e485f0dccd2e6e1351d6d8194ec94c753f60b3
Reviewed-on: http://gerrit.cloudera.org:8080/11567
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/ccec2416
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/ccec2416
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/ccec2416
Branch: refs/heads/master
Commit: ccec241637e78a1432653330a45540127eb3e7df
Parents: e1d1b4f
Author: Alex Rodoni <ar...@cloudera.com>
Authored: Tue Oct 2 15:29:58 2018 -0700
Committer: Alex Rodoni <ar...@cloudera.com>
Committed: Wed Oct 3 02:16:41 2018 +0000
----------------------------------------------------------------------
docs/shared/impala_common.xml | 37 +++++++++++++++++++-----------------
docs/topics/impala_literals.xml | 4 ++--
2 files changed, 22 insertions(+), 19 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/ccec2416/docs/shared/impala_common.xml
----------------------------------------------------------------------
diff --git a/docs/shared/impala_common.xml b/docs/shared/impala_common.xml
index c827ec1..ff0f449 100644
--- a/docs/shared/impala_common.xml
+++ b/docs/shared/impala_common.xml
@@ -1356,33 +1356,36 @@ drop database temp;
values such as <codeph>'1966-07-30'</codeph>, <codeph>'08:30:00'</codeph>,
or <codeph>'1985-09-25 17:45:30.005'</codeph>.
</p>
- <p>Leading zeroes are not required in the numbers representing the date
+ <p>
+ Leading zeroes are not required in the numbers representing the date
component, such as month and date, or the time component, such as
hour, minute, and second. For example, Impala accepts both
<codeph>"2018-1-1 01:02:03"</codeph> and
- <codeph>"2018-01-01 1:2:3"</codeph> as valid.</p>
+ <codeph>"2018-01-01 1:2:3"</codeph> as valid.
+ </p>
+ <p>
+ In <codeph>STRING</codeph> to <codeph>TIMESTAMP</codeph> conversions,
+ leading and trailing white spaces, such as a space, a tab, a newline, or
+ a carriage return, are ignored. For example, Impala treats the following
+ as equivalent: '1999-12-01 01:02:03 ', ' 1999-12-01 01:02:03',
+ '1999-12-01 01:02:03\r\n\t'.
+ </p>
<p id="cast_string_to_timestamp">
- When you convert or cast a <codeph>STRING</codeph> literal to <codeph>TIMESTAMP</codeph>,
- you can use the following separators between the date part and the time part:
+ When you convert or cast a
+ <codeph>STRING</codeph> literal to <codeph>TIMESTAMP</codeph>, you can
+ use the following separators between the date part and the time part:
<ul>
<li>
- <p>
- One or more space characters
- </p>
-
- <p>
- Example: <codeph>CAST ('2001-01-09 01:05:01' AS TIMESTAMP)</codeph>
+ <p> One or more space characters </p>
+ <p> Example: <codeph>CAST ('2001-01-09 01:05:01' AS
+ TIMESTAMP)</codeph>
</p>
</li>
-
<li>
- <p>
- The character âTâ
- </p>
-
- <p>
- Example: <codeph>CAST ('2001-01-09T01:05:01' AS TIMESTAMP)</codeph>
+ <p> The character âTâ </p>
+ <p> Example: <codeph>CAST ('2001-01-09T01:05:01' AS
+ TIMESTAMP)</codeph>
</p>
</li>
</ul>
http://git-wip-us.apache.org/repos/asf/impala/blob/ccec2416/docs/topics/impala_literals.xml
----------------------------------------------------------------------
diff --git a/docs/topics/impala_literals.xml b/docs/topics/impala_literals.xml
index bdfda25..6c5b823 100644
--- a/docs/topics/impala_literals.xml
+++ b/docs/topics/impala_literals.xml
@@ -295,8 +295,8 @@ select case bool_col when true then 'yes' when false 'no' else 'null' end from t
<conbody>
- <p conref="../shared/impala_common.xml#common/timestamp_conversions"/>
-
+ <p conref="../shared/impala_common.xml#common/timestamp_conversions"
+ conrefend="../shared/impala_common.xml#common/cast_string_to_timestamp"/>
<p>
You can also use <codeph>INTERVAL</codeph> expressions to add or subtract from timestamp literal values,
such as <codeph>CAST('1966-07-30' AS TIMESTAMP) + INTERVAL 5 YEARS + INTERVAL 3 DAYS</codeph>. See
[4/4] impala git commit: IMPALA-7581: timeout backend tests after 2
hours
Posted by ta...@apache.org.
IMPALA-7581: timeout backend tests after 2 hours
This will abort the process if a backend test is taking too long, which
we assume is because of a hang. This makes job failures easier to triage
and may make it easier to debug failures if we collect coredumps or
minidumps.
Also disable the death tests for ASAN under the theory that the
probability of the hang is higher than a regular DEBUG build.
Testing:
Reduced the timeout to 5s and confirmed that it worked.
Change-Id: I2e4ef9cb0549ead0bae57b11489f6a4d9b44ef95
Reviewed-on: http://gerrit.cloudera.org:8080/11533
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Tim Armstrong <ta...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/93606e60
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/93606e60
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/93606e60
Branch: refs/heads/master
Commit: 93606e6046054526c093975894efc1eef8a53bc1
Parents: d301600
Author: Tim Armstrong <ta...@cloudera.com>
Authored: Wed Sep 26 17:43:51 2018 -0700
Committer: Tim Armstrong <ta...@cloudera.com>
Committed: Thu Oct 4 21:51:50 2018 +0000
----------------------------------------------------------------------
be/src/common/init.cc | 22 ++++++++++++++++++++--
be/src/testutil/death-test-util.h | 5 ++++-
be/src/util/test-info.h | 1 +
3 files changed, 25 insertions(+), 3 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/93606e60/be/src/common/init.cc
----------------------------------------------------------------------
diff --git a/be/src/common/init.cc b/be/src/common/init.cc
index 62a33b1..6c02dbf 100644
--- a/be/src/common/init.cc
+++ b/be/src/common/init.cc
@@ -115,6 +115,13 @@ static unique_ptr<impala::Thread> memory_maintenance_thread;
// time slept. If that exceeds PAUSE_WARN_THRESHOLD_MS, a warning is logged.
static unique_ptr<impala::Thread> pause_monitor;
+// Thread only used in backend tests to implement a test timeout.
+static unique_ptr<impala::Thread> be_timeout_thread;
+
+// Timeout after 2 hours - backend tests should generally run in minutes or tens of
+// minutes at worst.
+static const int64_t BE_TEST_TIMEOUT_S = 60L * 60L * 2L;
+
[[noreturn]] static void LogMaintenanceThread() {
while (true) {
sleep(FLAGS_logbufsecs);
@@ -236,10 +243,21 @@ void impala::InitCommonRuntime(int argc, char** argv, bool init_jvm,
&LogMaintenanceThread, &log_maintenance_thread);
if (!thread_spawn_status.ok()) CLEAN_EXIT_WITH_ERROR(thread_spawn_status.GetDetail());
- thread_spawn_status = Thread::Create("common", "pause-monitor",
- &PauseMonitorLoop, &pause_monitor);
+ thread_spawn_status =
+ Thread::Create("common", "pause-monitor", &PauseMonitorLoop, &pause_monitor);
if (!thread_spawn_status.ok()) CLEAN_EXIT_WITH_ERROR(thread_spawn_status.GetDetail());
+ // Implement timeout for backend tests.
+ if (impala::TestInfo::is_be_test()) {
+ thread_spawn_status = Thread::Create("common", "be-test-timeout-thread",
+ []() {
+ SleepForMs(BE_TEST_TIMEOUT_S * 1000L);
+ LOG(FATAL) << "Backend test timed out after " << BE_TEST_TIMEOUT_S << "s";
+ },
+ &be_timeout_thread);
+ if (!thread_spawn_status.ok()) CLEAN_EXIT_WITH_ERROR(thread_spawn_status.GetDetail());
+ }
+
PeriodicCounterUpdater::Init();
LOG(INFO) << impala::GetVersionString();
http://git-wip-us.apache.org/repos/asf/impala/blob/93606e60/be/src/testutil/death-test-util.h
----------------------------------------------------------------------
diff --git a/be/src/testutil/death-test-util.h b/be/src/testutil/death-test-util.h
index 474025b..7a4b3f9 100644
--- a/be/src/testutil/death-test-util.h
+++ b/be/src/testutil/death-test-util.h
@@ -24,7 +24,7 @@
// Wrapper around gtest's ASSERT_DEBUG_DEATH that prevents coredumps and minidumps
// being generated as the result of the death test.
-#ifndef NDEBUG
+#if !defined(NDEBUG) && !defined(ADDRESS_SANITIZER) && !defined(THREAD_SANITIZER)
#define IMPALA_ASSERT_DEBUG_DEATH(fn, msg) \
do { \
ScopedCoredumpDisabler disable_coredumps; \
@@ -37,6 +37,9 @@
// death tests that work in both debug and release builds. To avoid this problem, update
// our wrapper macro to simply omit the death test expression in release builds, where we
// can't actually test DCHECKs anyway.
+// Also disable the death tests in ASAN and TSAN builds where we suspect there is a
+// higher risk of hangs because of races with other threads holding locks during fork() -
+// see IMPALA-7581.
#define IMPALA_ASSERT_DEBUG_DEATH(fn, msg)
#endif
http://git-wip-us.apache.org/repos/asf/impala/blob/93606e60/be/src/util/test-info.h
----------------------------------------------------------------------
diff --git a/be/src/util/test-info.h b/be/src/util/test-info.h
index c2d1a5f..b1ec64d 100644
--- a/be/src/util/test-info.h
+++ b/be/src/util/test-info.h
@@ -33,6 +33,7 @@ class TestInfo {
/// Called in InitCommonRuntime().
static void Init(Mode mode) { mode_ = mode; }
+ static bool is_be_test() { return mode_ == BE_TEST; }
static bool is_fe_test() { return mode_ == FE_TEST; }
static bool is_test() { return mode_ == BE_TEST || mode_ == FE_TEST; }
[3/4] impala git commit: Revert "IMPALA-7595: Revert "IMPALA-7521:
Speed up sub-second unix time->TimestampValue conversions""
Posted by ta...@apache.org.
Revert "IMPALA-7595: Revert "IMPALA-7521: Speed up sub-second unix time->TimestampValue conversions""
IMPALA-7595 added proper handling for invalid time-of-day values
in Parquet, so the DCHECK mentioned in IMPALA-7595 will no longer
be hit. This means that IMPALA-7521 can be committed again without
causing problems.
This reverts commit f8b472ee6442e31a867a6dd6aaac22cc44291d41.
Change-Id: Ibab04bc6ad09db331220312ed21d90622fdfc41b
Reviewed-on: http://gerrit.cloudera.org:8080/11573
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/d301600a
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/d301600a
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/d301600a
Branch: refs/heads/master
Commit: d301600a85f399863d89c281a9a1dc3091d52fcc
Parents: e6bbe4e
Author: Csaba Ringhofer <cs...@cloudera.com>
Authored: Wed Oct 3 14:51:15 2018 +0200
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Thu Oct 4 03:40:44 2018 +0000
----------------------------------------------------------------------
.../benchmarks/convert-timestamp-benchmark.cc | 183 ++++++++++++++++++-
be/src/exec/data-source-scan-node.cc | 2 +-
be/src/exec/hdfs-orc-scanner.cc | 1 +
be/src/exprs/decimal-operators-ir.cc | 1 +
be/src/exprs/expr-test.cc | 6 +-
be/src/runtime/timestamp-test.cc | 129 +++++++++++--
be/src/runtime/timestamp-value.cc | 43 +----
be/src/runtime/timestamp-value.h | 64 +++----
be/src/runtime/timestamp-value.inline.h | 50 ++++-
9 files changed, 371 insertions(+), 108 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/benchmarks/convert-timestamp-benchmark.cc
----------------------------------------------------------------------
diff --git a/be/src/benchmarks/convert-timestamp-benchmark.cc b/be/src/benchmarks/convert-timestamp-benchmark.cc
index 89ba857..dd40f2f 100644
--- a/be/src/benchmarks/convert-timestamp-benchmark.cc
+++ b/be/src/benchmarks/convert-timestamp-benchmark.cc
@@ -101,9 +101,28 @@ UnixTimeToLocalPtime: Function iters/ms 10%ile 50%ile 90%ile 10%
UnixTimeToUtcPtime: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
(relative) (relative) (relative)
---------------------------------------------------------------------------------------------------------
- (glibc) 18.8 19.2 19.5 1X 1X 1X
- (Google/CCTZ) 8 8.14 8.27 0.425X 0.425X 0.424X
- (fast path) 29.5 30.2 30.8 1.57X 1.58X 1.58X
+ (glibc) 17 17.6 17.9 1X 1X 1X
+ (Google/CCTZ) 6.45 6.71 6.81 0.379X 0.382X 0.38X
+ (fast path) 25.1 26 26.4 1.47X 1.48X 1.48X
+ (day split) 48.6 50.3 51.3 2.85X 2.87X 2.86X
+
+UtcFromUnixTimeMicros: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
+ (relative) (relative) (relative)
+---------------------------------------------------------------------------------------------------------
+ (sec split (old)) 17.9 18.7 19.1 1X 1X 1X
+ (day split) 111 116 118 6.21X 6.19X 6.19X
+
+FromUnixTimeNanos: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
+ (relative) (relative) (relative)
+---------------------------------------------------------------------------------------------------------
+ (sec split (old)) 18.7 19.5 19.8 1X 1X 1X
+ (sec split (new)) 104 108 110 5.58X 5.55X 5.57X
+
+FromSubsecondUnixTime: Function iters/ms 10%ile 50%ile 90%ile 10%ile 50%ile 90%ile
+ (relative) (relative) (relative)
+---------------------------------------------------------------------------------------------------------
+ (old) 18.7 18.7 18.7 1X 1X 1X
+ (new) 73.5 74.1 74.1 3.94X 3.96X 3.96X
Number of threads: 8
@@ -236,14 +255,17 @@ void bail_if_results_dont_match(const vector<const vector<T>*>& test_result_vec)
auto b = test_result_vec.begin();
for (auto it = b + 1; it != test_result_vec.end(); ++it) {
- if (**b != **it) {
- cerr << "Results don't match.";
- exit(1);
+ auto& references = **b;
+ auto& results = **it;
+ for (int i = 0; i < references.size(); ++i) {
+ if (references[i] != results[i]) {
+ cerr << "Results don't match: " << references[i] << " vs " << results[i] << endl;
+ exit(1);
+ }
}
}
}
-
//
// Test UtcToUnixTime (boost is expected to be the fastest, followed by CCTZ and glibc)
//
@@ -540,6 +562,68 @@ boost::posix_time::ptime cctz_optimized_unix_time_to_utc_ptime(const time_t& uni
return cctz_unix_time_to_utc_ptime(unix_time);
}
+boost::posix_time::ptime split_unix_time_to_utc_ptime(const time_t& unix_time) {
+ int64_t time = unix_time;
+ int32_t days = TimestampValue::SplitTime<24*60*60>(&time);
+
+ return boost::posix_time::ptime(
+ boost::gregorian::date(1970, 1, 1) + boost::gregorian::date_duration(days),
+ boost::posix_time::nanoseconds(time*NANOS_PER_SEC));
+}
+
+TimestampValue sec_split_utc_from_unix_time_micros(const int64_t& unix_time_micros) {
+ int64_t ts_seconds = unix_time_micros / MICROS_PER_SEC;
+ int64_t micros_part = unix_time_micros - (ts_seconds * MICROS_PER_SEC);
+ boost::posix_time::ptime temp = cctz_optimized_unix_time_to_utc_ptime(ts_seconds);
+ temp += boost::posix_time::microseconds(micros_part);
+ return TimestampValue(temp);
+}
+
+TimestampValue day_split_utc_from_unix_time_micros(const int64_t& unix_time_micros) {
+ static const boost::gregorian::date EPOCH(1970,1,1);
+ int64_t micros = unix_time_micros;
+ int32_t days = TimestampValue::SplitTime<24LL*60*60*MICROS_PER_SEC>(µs);
+
+ return TimestampValue(
+ EPOCH + boost::gregorian::date_duration(days),
+ boost::posix_time::nanoseconds(micros*1000));
+}
+
+struct SplitNanoAndSecond {
+ int64_t seconds;
+ int64_t nanos;
+};
+
+TimestampValue old_split_utc_from_unix_time_nanos(const SplitNanoAndSecond& unix_time) {
+ boost::posix_time::ptime temp =
+ cctz_optimized_unix_time_to_utc_ptime(unix_time.seconds);
+ temp += boost::posix_time::nanoseconds(unix_time.nanos);
+ return TimestampValue(temp);
+}
+
+TimestampValue new_split_utc_from_unix_time_nanos(const SplitNanoAndSecond& unix_time) {
+ // The TimestampValue version is used as it is hard to reproduce the same logic without
+ // accessing private members.
+ return TimestampValue::FromUnixTimeNanos(unix_time.seconds, unix_time.nanos,
+ TimezoneDatabase::GetUtcTimezone());
+}
+
+TimestampValue from_subsecond_unix_time_old(const double& unix_time) {
+ const double ONE_BILLIONTH = 0.000000001;
+ const time_t unix_time_whole = unix_time;
+ boost::posix_time::ptime temp =
+ cctz_optimized_unix_time_to_utc_ptime(unix_time_whole);
+ temp += boost::posix_time::nanoseconds((unix_time - unix_time_whole) / ONE_BILLIONTH);
+ return TimestampValue(temp);
+}
+
+TimestampValue from_subsecond_unix_time_new(const double& unix_time) {
+ const double ONE_BILLIONTH = 0.000000001;
+ int64_t unix_time_whole = unix_time;
+ int64_t nanos = (unix_time - unix_time_whole) / ONE_BILLIONTH;
+ return TimestampValue::FromUnixTimeNanos(
+ unix_time_whole, nanos, TimezoneDatabase::GetUtcTimezone());
+}
//
// Test ToUtc (CCTZ is expected to be faster than boost)
@@ -749,6 +833,10 @@ int main(int argc, char* argv[]) {
time_t,
boost::posix_time::ptime,
fastpath_unix_time_to_utc_ptime> fastpath_unix_time_to_utc_ptime_data = time_data;
+ TestData<
+ time_t,
+ boost::posix_time::ptime,
+ split_unix_time_to_utc_ptime> split_unix_time_to_utc_ptime_data = time_data;
glibc_unix_time_to_utc_ptime_data.add_to_benchmark(bm_unix_time_to_utc_ptime,
"(glibc)");
@@ -756,12 +844,91 @@ int main(int argc, char* argv[]) {
"(Google/CCTZ)");
fastpath_unix_time_to_utc_ptime_data.add_to_benchmark(bm_unix_time_to_utc_ptime,
"(fast path)");
+ split_unix_time_to_utc_ptime_data.add_to_benchmark(bm_unix_time_to_utc_ptime,
+ "(day split)");
cout << bm_unix_time_to_utc_ptime.Measure() << endl;
bail_if_results_dont_match(vector<const vector<boost::posix_time::ptime>*>{
&glibc_unix_time_to_utc_ptime_data.result(),
&cctz_unix_time_to_utc_ptime_data.result(),
- &fastpath_unix_time_to_utc_ptime_data.result()});
+ &fastpath_unix_time_to_utc_ptime_data.result(),
+ &split_unix_time_to_utc_ptime_data.result()});
+
+ // Benchmark UtcFromUnixTimeMicros improvement in IMPALA-7417.
+ vector<time_t> microsec_data;
+ for (int i = 0; i < tsvalue_data.size(); ++i) {
+ const TimestampValue& tsvalue = tsvalue_data[i];
+ time_t unix_time;
+ tsvalue.ToUnixTime(TimezoneDatabase::GetUtcTimezone(), &unix_time);
+ int micros = (i * 1001) % MICROS_PER_SEC; // add some sub-second part
+ microsec_data.push_back(unix_time * MICROS_PER_SEC + micros);
+ }
+
+ Benchmark bm_utc_from_unix_time_micros("UtcFromUnixTimeMicros");
+ TestData<int64_t, TimestampValue, sec_split_utc_from_unix_time_micros>
+ sec_split_utc_from_unix_time_micros_data = microsec_data;
+ TestData<int64_t, TimestampValue, day_split_utc_from_unix_time_micros>
+ day_split_utc_from_unix_time_micros_data = microsec_data;
+
+ sec_split_utc_from_unix_time_micros_data.add_to_benchmark(bm_utc_from_unix_time_micros,
+ "(sec split (old))");
+ day_split_utc_from_unix_time_micros_data.add_to_benchmark(bm_utc_from_unix_time_micros,
+ "(day split)");
+ cout << bm_utc_from_unix_time_micros.Measure() << endl;
+
+ bail_if_results_dont_match(vector<const vector<TimestampValue>*>{
+ &sec_split_utc_from_unix_time_micros_data.result(),
+ &day_split_utc_from_unix_time_micros_data.result()});
+
+ // Benchmark FromUnixTimeNanos improvement in IMPALA-7417.
+ vector<SplitNanoAndSecond> nanosec_data;
+ for (int i = 0; i < tsvalue_data.size(); ++i) {
+ const TimestampValue& tsvalue = tsvalue_data[i];
+ time_t unix_time;
+ tsvalue.ToUnixTime(TimezoneDatabase::GetUtcTimezone(), &unix_time);
+ int nanos = (i * 1001) % NANOS_PER_SEC; // add some sub-second part
+ nanosec_data.push_back(SplitNanoAndSecond {unix_time, nanos} );
+ }
+
+ Benchmark bm_utc_from_unix_time_nanos("FromUnixTimeNanos");
+ TestData<SplitNanoAndSecond, TimestampValue, old_split_utc_from_unix_time_nanos>
+ old_split_utc_from_unix_time_nanos_data = nanosec_data;
+ TestData<SplitNanoAndSecond, TimestampValue, new_split_utc_from_unix_time_nanos>
+ new_split_utc_from_unix_time_nanos_data = nanosec_data;
+
+ old_split_utc_from_unix_time_nanos_data.add_to_benchmark(bm_utc_from_unix_time_nanos,
+ "(sec split (old))");
+ new_split_utc_from_unix_time_nanos_data.add_to_benchmark(bm_utc_from_unix_time_nanos,
+ "(sec split (new))");
+ cout << bm_utc_from_unix_time_nanos.Measure() << endl;
+
+ bail_if_results_dont_match(vector<const vector<TimestampValue>*>{
+ &old_split_utc_from_unix_time_nanos_data.result(),
+ &new_split_utc_from_unix_time_nanos_data.result()});
+
+ // Benchmark FromSubsecondUnixTime before and after IMPALA-7417.
+ vector<double> double_data;
+ for (int i = 0; i < tsvalue_data.size(); ++i) {
+ const TimestampValue& tsvalue = tsvalue_data[i];
+ time_t unix_time;
+ tsvalue.ToUnixTime(TimezoneDatabase::GetUtcTimezone(), &unix_time);
+ double nanos = (i * 1001) % NANOS_PER_SEC; // add some sub-second part
+ double_data.push_back((double)unix_time + nanos / NANOS_PER_SEC);
+ }
+
+ Benchmark from_subsecond_unix_time("FromSubsecondUnixTime");
+ TestData<double, TimestampValue, from_subsecond_unix_time_old>
+ from_subsecond_unix_time_old_data = double_data;
+ TestData<double, TimestampValue, from_subsecond_unix_time_new>
+ from_subsecond_unix_time_new_data = double_data;
+
+ from_subsecond_unix_time_old_data.add_to_benchmark(from_subsecond_unix_time, "(old)");
+ from_subsecond_unix_time_new_data.add_to_benchmark(from_subsecond_unix_time, "(new)");
+ cout << from_subsecond_unix_time.Measure() << endl;
+
+ bail_if_results_dont_match(vector<const vector<TimestampValue>*>{
+ &from_subsecond_unix_time_old_data.result(),
+ &from_subsecond_unix_time_new_data.result()});
// If number of threads is specified, run multithreaded tests.
int num_of_threads = (argc < 2) ? 0 : atoi(argv[1]);
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/exec/data-source-scan-node.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/data-source-scan-node.cc b/be/src/exec/data-source-scan-node.cc
index fa11d05..10f0c7d 100644
--- a/be/src/exec/data-source-scan-node.cc
+++ b/be/src/exec/data-source-scan-node.cc
@@ -29,7 +29,7 @@
#include "runtime/runtime-state.h"
#include "runtime/row-batch.h"
#include "runtime/string-value.h"
-#include "runtime/timestamp-value.h"
+#include "runtime/timestamp-value.inline.h"
#include "runtime/tuple-row.h"
#include "util/jni-util.h"
#include "util/periodic-counter-updater.h"
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/exec/hdfs-orc-scanner.cc
----------------------------------------------------------------------
diff --git a/be/src/exec/hdfs-orc-scanner.cc b/be/src/exec/hdfs-orc-scanner.cc
index 55b3b71..fa36660 100644
--- a/be/src/exec/hdfs-orc-scanner.cc
+++ b/be/src/exec/hdfs-orc-scanner.cc
@@ -24,6 +24,7 @@
#include "runtime/exec-env.h"
#include "runtime/io/request-context.h"
#include "runtime/runtime-filter.inline.h"
+#include "runtime/timestamp-value.inline.h"
#include "runtime/tuple-row.h"
#include "util/decompress.h"
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/exprs/decimal-operators-ir.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/decimal-operators-ir.cc b/be/src/exprs/decimal-operators-ir.cc
index 0a41f97..c987b9d 100644
--- a/be/src/exprs/decimal-operators-ir.cc
+++ b/be/src/exprs/decimal-operators-ir.cc
@@ -26,6 +26,7 @@
#include "exprs/scalar-expr.h"
#include "exprs/timezone_db.h"
#include "runtime/decimal-value.inline.h"
+#include "runtime/timestamp-value.inline.h"
#include "util/decimal-util.h"
#include "util/string-parser.h"
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/exprs/expr-test.cc
----------------------------------------------------------------------
diff --git a/be/src/exprs/expr-test.cc b/be/src/exprs/expr-test.cc
index 7168b03..c926ff0 100644
--- a/be/src/exprs/expr-test.cc
+++ b/be/src/exprs/expr-test.cc
@@ -172,13 +172,15 @@ class ScopedTimeZoneOverride {
// Enable FLAGS_use_local_tz_for_unix_timestamp_conversions for the duration of the scope.
class ScopedLocalUnixTimestampConversionOverride {
+ bool original_;
public:
ScopedLocalUnixTimestampConversionOverride() {
+ original_ = FLAGS_use_local_tz_for_unix_timestamp_conversions;
FLAGS_use_local_tz_for_unix_timestamp_conversions = true;
}
~ScopedLocalUnixTimestampConversionOverride() {
- FLAGS_use_local_tz_for_unix_timestamp_conversions = false;
+ FLAGS_use_local_tz_for_unix_timestamp_conversions = original_;
}
};
@@ -6548,6 +6550,7 @@ TEST_F(ExprTest, TimestampFunctions) {
// Test that now() and current_timestamp() are reasonable.
{
ScopedTimeZoneOverride time_zone("PST8PDT");
+ ScopedLocalUnixTimestampConversionOverride use_local;
const Timezone& local_tz = time_zone.GetTimezone();
const TimestampValue start_time =
@@ -6573,6 +6576,7 @@ TEST_F(ExprTest, TimestampFunctions) {
// Test cast(unix_timestamp() as timestamp).
{
ScopedTimeZoneOverride time_zone("PST8PDT");
+ ScopedLocalUnixTimestampConversionOverride use_local;
const Timezone& local_tz = time_zone.GetTimezone();
// UNIX_TIMESTAMP() has second precision so the comparison start time is shifted back
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/runtime/timestamp-test.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/timestamp-test.cc b/be/src/runtime/timestamp-test.cc
index 3f3f01d..4e5b4b9 100644
--- a/be/src/runtime/timestamp-test.cc
+++ b/be/src/runtime/timestamp-test.cc
@@ -233,6 +233,66 @@ void TestTimestampTokens(vector<TimestampToken>* toks, int year, int month,
} while (next_permutation(toks->begin(), toks->end()));
}
+// Checks that FromSubsecondUnixTime gives the correct result. Conversion to double
+// is lossy so the result is expected to be within a certain range of 'expected'.
+// The fraction part of 'nanos' can express sub-nanoseconds - the current logic is to
+// truncate to the next whole nanosecond (towards 0).
+void TestFromDoubleUnixTime(
+ int64_t seconds, int64_t millis, double nanos, const TimestampValue& expected) {
+ auto tz = TimezoneDatabase::GetUtcTimezone();
+ TimestampValue from_double = TimestampValue::FromSubsecondUnixTime(
+ 1.0 * seconds + 0.001 * millis + nanos / NANOS_PER_SEC, tz);
+
+ if (!expected.HasDate()) EXPECT_FALSE(from_double.HasDate());
+ else {
+ // Conversion to double is lossy so the timestamp can be a bit different.
+ int64_t expected_rounded_to_micros, from_double_rounded_to_micros;
+ EXPECT_TRUE(expected.UtcToUnixTimeMicros(&expected_rounded_to_micros));
+ EXPECT_TRUE(from_double.UtcToUnixTimeMicros(&from_double_rounded_to_micros));
+ // The difference can be more than a microsec in case of timestamps far from 1970.
+ int64_t MARGIN_OF_ERROR = 8;
+ EXPECT_LT(abs(expected_rounded_to_micros - from_double_rounded_to_micros),
+ MARGIN_OF_ERROR);
+ }
+}
+
+// Checks that all sub-second From*UnixTime gives the same result and that the result
+// is the same as 'expected'.
+// If 'expected' is nullptr then the result is expected to be invalid (out of range).
+void TestFromSubSecondFunctions(int64_t seconds, int64_t millis, const char* expected) {
+ auto tz = TimezoneDatabase::GetUtcTimezone();
+
+ TimestampValue from_millis =
+ TimestampValue::UtcFromUnixTimeMillis(seconds * 1000 + millis);
+ if (expected == nullptr) EXPECT_FALSE(from_millis.HasDate());
+ else EXPECT_EQ(expected, from_millis.ToString());
+
+ EXPECT_EQ(from_millis, TimestampValue::UtcFromUnixTimeMicros(
+ seconds * MICROS_PER_SEC + millis * 1000));
+ EXPECT_EQ(from_millis, TimestampValue::FromUnixTimeMicros(
+ seconds * MICROS_PER_SEC + millis * 1000, tz));
+
+ // Check the same timestamp shifted with some sub-nanosecs.
+ vector<double> sub_nanosec_offsets =
+ {0.0, 0.1, 0.9, 0.000001, 0.999999, 2.2250738585072020e-308};
+ for (double sub_nanos: sub_nanosec_offsets) {
+ TestFromDoubleUnixTime(seconds, millis, sub_nanos, from_millis);
+ TestFromDoubleUnixTime(seconds, millis, -sub_nanos, from_millis);
+ }
+
+ // Test FromUnixTimeNanos with shifted sec + subsec pairs.
+ vector<int64_t> signs = {-1, 1};
+ vector<int64_t> offsets = {0, 1, 2, 60, 60*60, 24*60*60};
+ for (int64_t sign: signs) {
+ for (int64_t offset: offsets) {
+ int64_t shifted_seconds = seconds + sign * offset;
+ int64_t shifted_nanos = (millis - 1000 * sign * offset) * 1000 * 1000;
+ EXPECT_EQ(from_millis,
+ TimestampValue::FromUnixTimeNanos(shifted_seconds, shifted_nanos, tz));
+ }
+ }
+}
+
TEST(TimestampTest, Basic) {
// Fix current time to determine the behavior parsing 2-digit year format
// Set it to 03/01 to test 02/29 edge cases.
@@ -671,12 +731,9 @@ TEST(TimestampTest, Basic) {
// Sub-second FromUnixTime functions incorrectly accepted the last second of 1399
// as valid, because validation logic checked the nearest second rounded towards 0
// (IMPALA-5664).
- EXPECT_FALSE(TimestampValue::FromSubsecondUnixTime(
- MIN_DATE_AS_UNIX_TIME - 0.1, utc_tz).HasDate());
- EXPECT_FALSE(TimestampValue::UtcFromUnixTimeMicros(
- MIN_DATE_AS_UNIX_TIME * MICROS_PER_SEC - 2000).HasDate());
- EXPECT_FALSE(TimestampValue::FromUnixTimeNanos(
- MIN_DATE_AS_UNIX_TIME, -NANOS_PER_MICRO * 100, utc_tz).HasDate());
+ TestFromSubSecondFunctions(MIN_DATE_AS_UNIX_TIME, -100, nullptr);
+ TestFromSubSecondFunctions(MIN_DATE_AS_UNIX_TIME, 100,
+ "1400-01-01 00:00:00.100000000");
// Test the max supported date that can be represented in seconds.
const int64_t MAX_DATE_AS_UNIX_TIME = 253402300799;
@@ -719,20 +776,9 @@ TEST(TimestampTest, Basic) {
EXPECT_FALSE(too_late.HasTime());
// Checking sub-second FromUnixTime functions near 10000.01.01
- EXPECT_TRUE(TimestampValue::FromSubsecondUnixTime(
- MAX_DATE_AS_UNIX_TIME + 0.99, utc_tz).HasDate());
- EXPECT_FALSE(TimestampValue::FromSubsecondUnixTime(
- MAX_DATE_AS_UNIX_TIME + 1, utc_tz).HasDate());
-
- EXPECT_TRUE(TimestampValue::UtcFromUnixTimeMicros(
- MAX_DATE_AS_UNIX_TIME * MICROS_PER_SEC + MICROS_PER_SEC - 1).HasDate());
- EXPECT_FALSE(TimestampValue::UtcFromUnixTimeMicros(
- MAX_DATE_AS_UNIX_TIME * MICROS_PER_SEC + MICROS_PER_SEC).HasDate());
-
- EXPECT_TRUE(TimestampValue::FromUnixTimeNanos(
- MAX_DATE_AS_UNIX_TIME, NANOS_PER_SEC - 1, utc_tz).HasDate());
- EXPECT_FALSE(TimestampValue::FromUnixTimeNanos(
- MAX_DATE_AS_UNIX_TIME, NANOS_PER_SEC, utc_tz).HasDate());
+ TestFromSubSecondFunctions(MAX_DATE_AS_UNIX_TIME, MILLIS_PER_SEC - 1,
+ "9999-12-31 23:59:59.999000000");
+ TestFromSubSecondFunctions(MAX_DATE_AS_UNIX_TIME, MILLIS_PER_SEC, nullptr);
// Regression tests for IMPALA-1676, Unix times overflow int32 during year 2038
EXPECT_EQ("2038-01-19 03:14:08",
@@ -807,6 +853,49 @@ TEST(TimestampTest, Basic) {
TimestampValue::FromSubsecondUnixTime(0.008, utc_tz).ToString());
}
+// Test subsecond unix time conversion for non edge cases.
+TEST(TimestampTest, SubSecond) {
+ // Test with millisec precision.
+ TestFromSubSecondFunctions(0, 0, "1970-01-01 00:00:00");
+ TestFromSubSecondFunctions(0, 100, "1970-01-01 00:00:00.100000000");
+ TestFromSubSecondFunctions(0, 1100, "1970-01-01 00:00:01.100000000");
+ TestFromSubSecondFunctions(0, 24*60*60*1000, "1970-01-02 00:00:00");
+ TestFromSubSecondFunctions(0, 2*24*60*60*1000 + 100, "1970-01-03 00:00:00.100000000");
+
+ TestFromSubSecondFunctions(0, -100, "1969-12-31 23:59:59.900000000");
+ TestFromSubSecondFunctions(0, -1100, "1969-12-31 23:59:58.900000000");
+ TestFromSubSecondFunctions(0, -24*60*60*1000, "1969-12-31 00:00:00");
+ TestFromSubSecondFunctions(0, -2*24*60*60*1000 + 100, "1969-12-30 00:00:00.100000000");
+
+ TestFromSubSecondFunctions(-1, 0, "1969-12-31 23:59:59");
+ TestFromSubSecondFunctions(-1, 100, "1969-12-31 23:59:59.100000000");
+ TestFromSubSecondFunctions(-1, 1100, "1970-01-01 00:00:00.100000000");
+ TestFromSubSecondFunctions(-1, 24*60*60*1000, "1970-01-01 23:59:59");
+ TestFromSubSecondFunctions(-1, 2*24*60*60*1000 + 100, "1970-01-02 23:59:59.100000000");
+
+ TestFromSubSecondFunctions(-1, -100, "1969-12-31 23:59:58.900000000");
+ TestFromSubSecondFunctions(-1, -1100, "1969-12-31 23:59:57.900000000");
+ TestFromSubSecondFunctions(-1, -24*60*60*1000, "1969-12-30 23:59:59");
+ TestFromSubSecondFunctions(-1, -2*24*60*60*1000 + 100, "1969-12-29 23:59:59.100000000");
+
+ // A few test with sub-millisec precision.
+ auto tz = TimezoneDatabase::GetUtcTimezone();
+ EXPECT_EQ("1970-01-01 00:00:00.000001000",
+ TimestampValue::UtcFromUnixTimeMicros(1).ToString());
+ EXPECT_EQ("1969-12-31 23:59:59.999999000",
+ TimestampValue::UtcFromUnixTimeMicros(-1).ToString());
+
+ EXPECT_EQ("1970-01-01 00:00:00.000001000",
+ TimestampValue::FromUnixTimeMicros(1, tz).ToString());
+ EXPECT_EQ("1969-12-31 23:59:59.999999000",
+ TimestampValue::FromUnixTimeMicros(-1, tz).ToString());
+
+ EXPECT_EQ("1970-01-01 00:00:00.000000001",
+ TimestampValue::FromUnixTimeNanos(0, 1, tz).ToString());
+ EXPECT_EQ("1969-12-31 23:59:59.999999999",
+ TimestampValue::FromUnixTimeNanos(0, -1, tz).ToString());
+}
+
}
IMPALA_TEST_MAIN();
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/runtime/timestamp-value.cc
----------------------------------------------------------------------
diff --git a/be/src/runtime/timestamp-value.cc b/be/src/runtime/timestamp-value.cc
index a3631e5..14c57b9 100644
--- a/be/src/runtime/timestamp-value.cc
+++ b/be/src/runtime/timestamp-value.cc
@@ -142,7 +142,7 @@ void TimestampValue::LocalToUtc(const Timezone& local_tz) {
SetToInvalidDateTime();
} else {
int64_t nanos = time_.fractional_seconds();
- *this = UnixTimeToUtcPtime(TimePointToUnixTime(from_cl.pre));
+ *this = UtcFromUnixTimeTicks<1>(TimePointToUnixTime(from_cl.pre));
// Time-zone conversion rules don't affect fractional seconds, leave them intact.
time_ += nanoseconds(nanos);
}
@@ -152,10 +152,8 @@ ostream& operator<<(ostream& os, const TimestampValue& timestamp_value) {
return os << timestamp_value.ToString();
}
-/// Return a ptime representation of the given Unix time (seconds since the Unix epoch).
-/// The time zone of the resulting ptime is 'local_tz'. This is called by UnixTimeToPtime.
-ptime TimestampValue::UnixTimeToLocalPtime(time_t unix_time,
- const Timezone& local_tz) {
+TimestampValue TimestampValue::UnixTimeToLocal(
+ time_t unix_time, const Timezone& local_tz) {
cctz::time_point<cctz::sys_seconds> from_tp = UnixTimeToTimePoint(unix_time);
cctz::civil_second to_cs = cctz::convert(from_tp, local_tz);
// boost::gregorian::date() throws boost::gregorian::bad_year if year is not in the
@@ -163,44 +161,17 @@ ptime TimestampValue::UnixTimeToLocalPtime(time_t unix_time,
if (UNLIKELY(IsDateOutOfRange(to_cs))) {
return ptime(not_a_date_time);
} else {
- return ptime(
+ return TimestampValue(
boost::gregorian::date(to_cs.year(), to_cs.month(), to_cs.day()),
boost::posix_time::time_duration(to_cs.hour(), to_cs.minute(), to_cs.second()));
}
}
-/// Return a ptime representation of the given Unix time (seconds since the Unix epoch).
-/// The time zone of the resulting ptime is UTC.
-/// In order to avoid a serious performance degredation using CCTZ, this function uses
-/// boost to convert the time_t to a ptime. Unfortunately, because the boost conversion
-/// relies on time_duration to represent the time_t and internally
-/// time_duration stores nanosecond precision ticks, the 'fast path' conversion using
-/// boost can only handle a limited range of dates (appx years 1677-2622, while Impala
-/// supports years 1400-9999). For dates outside this range, the conversion will instead
-/// use the CCTZ function convert which supports those dates. This is called by
-/// UnixTimeToPtime.
-ptime TimestampValue::UnixTimeToUtcPtime(time_t unix_time) {
- // Minimum Unix time that can be converted with from_time_t: 1677-Sep-21 00:12:44
- const int64_t MIN_BOOST_CONVERT_UNIX_TIME = -9223372036;
- // Maximum Unix time that can be converted with from_time_t: 2262-Apr-11 23:47:16
- const int64_t MAX_BOOST_CONVERT_UNIX_TIME = 9223372036;
- if (LIKELY(unix_time >= MIN_BOOST_CONVERT_UNIX_TIME &&
- unix_time <= MAX_BOOST_CONVERT_UNIX_TIME)) {
- try {
- return from_time_t(unix_time);
- } catch (std::exception&) {
- return ptime(not_a_date_time);
- }
- }
-
- return UnixTimeToLocalPtime(unix_time, TimezoneDatabase::GetUtcTimezone());
-}
-
-ptime TimestampValue::UnixTimeToPtime(time_t unix_time, const Timezone& local_tz) {
+TimestampValue TimestampValue::FromUnixTime(time_t unix_time, const Timezone& local_tz) {
if (FLAGS_use_local_tz_for_unix_timestamp_conversions) {
- return UnixTimeToLocalPtime(unix_time, local_tz);
+ return UnixTimeToLocal(unix_time, local_tz);
} else {
- return UnixTimeToUtcPtime(unix_time);
+ return UtcFromUnixTimeTicks<1>(unix_time);
}
}
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/runtime/timestamp-value.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/timestamp-value.h b/be/src/runtime/timestamp-value.h
index 840a5ae..6d87d9b 100644
--- a/be/src/runtime/timestamp-value.h
+++ b/be/src/runtime/timestamp-value.h
@@ -97,22 +97,15 @@ class TimestampValue {
/// Return the corresponding timestamp in the 'local_tz' time zone if
/// FLAGS_use_local_tz_for_unix_timestamp_conversions is true. Otherwise, return the
/// corresponding timestamp in UTC.
- static TimestampValue FromUnixTime(time_t unix_time, const Timezone& local_tz) {
- return TimestampValue(UnixTimeToPtime(unix_time, local_tz));
- }
+ static TimestampValue FromUnixTime(time_t unix_time, const Timezone& local_tz);
/// Same as FromUnixTime() above, but adds the specified number of nanoseconds to the
/// resulting TimestampValue. Handles negative nanoseconds and the case where
/// abs(nanos) >= 1e9.
static TimestampValue FromUnixTimeNanos(time_t unix_time, int64_t nanos,
- const Timezone& local_tz) {
- boost::posix_time::ptime temp = UnixTimeToPtime(unix_time, local_tz);
- temp += boost::posix_time::nanoseconds(nanos);
- return TimestampValue(temp);
- }
+ const Timezone& local_tz);
- /// Return the corresponding timestamp in 'local_tz' time zone for the Unix time
- /// specified in microseconds.
+ /// Same as FromUnixTime(), but expects the time in microseconds.
static TimestampValue FromUnixTimeMicros(int64_t unix_time_micros,
const Timezone& local_tz);
@@ -120,17 +113,15 @@ class TimestampValue {
/// microseconds.
static TimestampValue UtcFromUnixTimeMicros(int64_t unix_time_micros);
+ /// Return the corresponding timestamp in UTC for the Unix time specified in
+ /// milliseconds.
+ static TimestampValue UtcFromUnixTimeMillis(int64_t unix_time_millis);
+
/// Returns a TimestampValue in 'local_tz' time zone where the integer part of the
/// specified 'unix_time' specifies the number of seconds (see above), and the
/// fractional part is converted to nanoseconds and added to the resulting
/// TimestampValue.
- static TimestampValue FromSubsecondUnixTime(double unix_time,
- const Timezone& local_tz) {
- const time_t unix_time_whole = unix_time;
- boost::posix_time::ptime temp = UnixTimeToPtime(unix_time_whole, local_tz);
- temp += boost::posix_time::nanoseconds((unix_time - unix_time_whole) / ONE_BILLIONTH);
- return TimestampValue(temp);
- }
+ static TimestampValue FromSubsecondUnixTime(double unix_time, const Timezone& local_tz);
/// Returns a TimestampValue converted from a TimestampVal. The caller must ensure
/// the TimestampVal does not represent a NULL.
@@ -245,7 +236,7 @@ class TimestampValue {
void LocalToUtc(const Timezone& local_tz);
void set_date(const boost::gregorian::date d) { date_ = d; Validate(); }
- void set_time(const boost::posix_time::time_duration t) { time_ = t; }
+ void set_time(const boost::posix_time::time_duration t) { time_ = t; Validate(); }
const boost::gregorian::date& date() const { return date_; }
const boost::posix_time::time_duration& time() const { return time_; }
@@ -286,6 +277,19 @@ class TimestampValue {
return HashUtil::Hash(&date_, sizeof(date_), hash);
}
+ /// Divides 'ticks' with 'GRANULARITY' (truncated towards negative infinity) and
+ /// sets 'ticks' to the remainder.
+ template <int64_t GRANULARITY>
+ inline static int64_t SplitTime(int64_t* ticks) {
+ int64_t result = *ticks / GRANULARITY;
+ *ticks %= GRANULARITY;
+ if (*ticks < 0) {
+ result--;
+ *ticks += GRANULARITY;
+ }
+ return result;
+ }
+
static const char* LLVM_CLASS_NAME;
private:
@@ -294,7 +298,6 @@ class TimestampValue {
/// Used when converting a time with fractional seconds which are stored as in integer
/// to a Unix time stored as a double.
static const double ONE_BILLIONTH;
-
/// Boost ptime leaves a gap in the structure, so we swap the order to make it
/// 12 contiguous bytes. We then must convert to and from the boost ptime data type.
/// See IMP-87 for more information on why using ptime with the 4 byte gap is
@@ -313,24 +316,21 @@ class TimestampValue {
}
/// Sets both date and time to invalid if date is outside the valid range.
+ /// Time's validity is only checked in debug builds.
+ /// TODO: This could be also checked in release, but I am a bit afraid that it would
+ /// affect performance and probably break some scenarios that are
+ /// currently working more or less correctly.
inline void Validate() {
if (HasDate() && UNLIKELY(!IsValidDate(date_))) SetToInvalidDateTime();
+ else if (HasTime()) DCHECK(IsValidTime(time_));
}
- /// Return a ptime representation of the given Unix time (seconds since the Unix epoch).
- /// The time zone of the resulting ptime is determined by
- /// FLAGS_use_local_tz_for_unix_timestamp_conversions. If the flag is true, the value
- /// will be in the 'local_tz' time zone. If the flag is false, the value will be in UTC.
- static boost::posix_time::ptime UnixTimeToPtime(time_t unix_time,
- const Timezone& local_tz);
-
- /// Same as the above, but the time zone of the resulting ptime is always in the
- /// 'local_tz' time zone.
- static boost::posix_time::ptime UnixTimeToLocalPtime(time_t unix_time,
- const Timezone& local_tz);
+ /// Converts 'unix_time' (in UTC seconds) to TimestampValue in timezone 'local_tz'.
+ static TimestampValue UnixTimeToLocal(time_t unix_time, const Timezone& local_tz);
- /// Same as the above, but the time zone of the resulting ptime is always in UTC.
- static boost::posix_time::ptime UnixTimeToUtcPtime(time_t unix_time);
+ /// Converts 'unix_time_ticks'/TICKS_PER_SEC seconds to TimestampValue.
+ template <int32_t TICKS_PER_SEC>
+ static TimestampValue UtcFromUnixTimeTicks(int64_t unix_time_ticks);
};
/// This function must be called 'hash_value' to be picked up by boost.
http://git-wip-us.apache.org/repos/asf/impala/blob/d301600a/be/src/runtime/timestamp-value.inline.h
----------------------------------------------------------------------
diff --git a/be/src/runtime/timestamp-value.inline.h b/be/src/runtime/timestamp-value.inline.h
index e049147..9c4b156 100644
--- a/be/src/runtime/timestamp-value.inline.h
+++ b/be/src/runtime/timestamp-value.inline.h
@@ -30,23 +30,53 @@
namespace impala {
+template <int32_t TICKS_PER_SEC>
+inline TimestampValue TimestampValue::UtcFromUnixTimeTicks(int64_t unix_time_ticks) {
+ static const boost::gregorian::date EPOCH(1970,1,1);
+ int64_t days = SplitTime<(uint64_t)TICKS_PER_SEC*24*60*60>(&unix_time_ticks);
+
+ return TimestampValue(EPOCH + boost::gregorian::date_duration(days),
+ boost::posix_time::nanoseconds(unix_time_ticks*(NANOS_PER_SEC/TICKS_PER_SEC)));
+}
+
inline TimestampValue TimestampValue::UtcFromUnixTimeMicros(int64_t unix_time_micros) {
- int64_t ts_seconds = unix_time_micros / MICROS_PER_SEC;
- int64_t micros_part = unix_time_micros - (ts_seconds * MICROS_PER_SEC);
- boost::posix_time::ptime temp = UnixTimeToUtcPtime(ts_seconds);
- temp += boost::posix_time::microseconds(micros_part);
- return TimestampValue(temp);
+ return UtcFromUnixTimeTicks<MICROS_PER_SEC>(unix_time_micros);
}
inline TimestampValue TimestampValue::FromUnixTimeMicros(int64_t unix_time_micros,
const Timezone& local_tz) {
- int64_t ts_seconds = unix_time_micros / MICROS_PER_SEC;
- int64_t micros_part = unix_time_micros - (ts_seconds * MICROS_PER_SEC);
- boost::posix_time::ptime temp = UnixTimeToLocalPtime(ts_seconds, local_tz);
- temp += boost::posix_time::microseconds(micros_part);
- return TimestampValue(temp);
+ int64_t ts_seconds = SplitTime<MICROS_PER_SEC>(&unix_time_micros);
+ TimestampValue result = FromUnixTime(ts_seconds, local_tz);
+ if (result.HasDate()) result.time_ += boost::posix_time::microseconds(unix_time_micros);
+ return result;
}
+inline TimestampValue TimestampValue::UtcFromUnixTimeMillis(int64_t unix_time_millis) {
+ return UtcFromUnixTimeTicks<MILLIS_PER_SEC>(unix_time_millis);
+}
+
+inline TimestampValue TimestampValue::FromSubsecondUnixTime(
+ double unix_time, const Timezone& local_tz) {
+ int64_t unix_time_whole = unix_time;
+ int64_t nanos = (unix_time - unix_time_whole) / ONE_BILLIONTH;
+ return FromUnixTimeNanos(unix_time_whole, nanos, local_tz);
+}
+
+inline TimestampValue TimestampValue::FromUnixTimeNanos(time_t unix_time, int64_t nanos,
+ const Timezone& local_tz) {
+ unix_time += SplitTime<NANOS_PER_SEC>(&nanos);
+ TimestampValue result = FromUnixTime(unix_time, local_tz);
+ // 'nanos' is guaranteed to be between [0,NANOS_PER_SEC) at this point, so the
+ // next addition cannot change the day or step to a different timezone.
+ if (result.HasDate()) {
+ DCHECK_GE(nanos, 0);
+ DCHECK_LT(nanos, NANOS_PER_SEC);
+ result.time_ += boost::posix_time::nanoseconds(nanos);
+ }
+ return result;
+}
+
+
/// Interpret 'this' as a timestamp in UTC and convert to unix time.
/// Returns false if the conversion failed ('unix_time' will be undefined), otherwise
/// true.
[2/4] impala git commit: IMPALA-7527: add fetch-from-catalogd cache
info to profile
Posted by ta...@apache.org.
IMPALA-7527: add fetch-from-catalogd cache info to profile
Reapplies reverted IMPALA-7527. Adding a top-level entry to the profile
broke downstream consumers. The change here is to add the additional stats
to the summary profile.
This patch adds a Java wrapper for a RuntimeProfile object. The wrapper
supports some basic operations like non-hierarchical counters and
informational strings.
During planning, a profile is created, and passed back to the backend as
part of the ExecRequest. The backend then updates the query profile
based on the info emitted from the frontend.
This patch also adds the first use case for this profile information:
the CatalogdMetaProvider emits counters for cache hits, misses, and
fetch times, broken down by metadata category.
The emitted profile is a bit of a superset of the existing 'timeline'
functionality. However, it seems that some tools may parse the timeline
in its current location in the profile, so moving it might be
incompatible. I elected to leave that alone for now and just emit
counters in the new profile.
Change-Id: I419be157168cddb7521ea61e8f86733306b9315e
Reviewed-on: http://gerrit.cloudera.org:8080/11569
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Project: http://git-wip-us.apache.org/repos/asf/impala/repo
Commit: http://git-wip-us.apache.org/repos/asf/impala/commit/e6bbe4ea
Tree: http://git-wip-us.apache.org/repos/asf/impala/tree/e6bbe4ea
Diff: http://git-wip-us.apache.org/repos/asf/impala/diff/e6bbe4ea
Branch: refs/heads/master
Commit: e6bbe4eaf5ba606ea3f4f1ed3360ecf9172a9ec3
Parents: ccec241
Author: Todd Lipcon <to...@apache.org>
Authored: Tue Sep 4 19:07:12 2018 -0700
Committer: Impala Public Jenkins <im...@cloudera.com>
Committed: Thu Oct 4 01:37:45 2018 +0000
----------------------------------------------------------------------
be/src/service/client-request-state.cc | 10 ++
be/src/service/client-request-state.h | 9 +
be/src/service/impala-server.cc | 1 +
be/src/util/runtime-profile.h | 2 +-
common/thrift/Frontend.thrift | 4 +
.../catalog/local/CatalogdMetaProvider.java | 92 ++++++++++-
.../org/apache/impala/service/Frontend.java | 13 +-
.../apache/impala/service/FrontendProfile.java | 163 +++++++++++++++++++
.../catalog/local/CatalogdMetaProviderTest.java | 20 +++
tests/custom_cluster/test_local_catalog.py | 5 +-
10 files changed, 301 insertions(+), 18 deletions(-)
----------------------------------------------------------------------
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/be/src/service/client-request-state.cc
----------------------------------------------------------------------
diff --git a/be/src/service/client-request-state.cc b/be/src/service/client-request-state.cc
index ca871ab..c19c974 100644
--- a/be/src/service/client-request-state.cc
+++ b/be/src/service/client-request-state.cc
@@ -77,6 +77,7 @@ ClientRequestState::ClientRequestState(
coord_exec_called_(false),
// Profile is assigned name w/ id after planning
profile_(RuntimeProfile::Create(&profile_pool_, "Query")),
+ frontend_profile_(RuntimeProfile::Create(&profile_pool_, "Frontend")),
server_profile_(RuntimeProfile::Create(&profile_pool_, "ImpalaServer")),
summary_profile_(RuntimeProfile::Create(&profile_pool_, "Summary")),
frontend_(frontend),
@@ -118,6 +119,8 @@ ClientRequestState::ClientRequestState(
"Sql Statement", query_ctx_.client_request.stmt);
summary_profile_->AddInfoString("Coordinator",
TNetworkAddressToString(exec_env->GetThriftBackendAddress()));
+
+ summary_profile_->AddChild(frontend_profile_);
}
ClientRequestState::~ClientRequestState() {
@@ -138,6 +141,13 @@ Status ClientRequestState::SetResultCache(QueryResultSet* cache,
return Status::OK();
}
+void ClientRequestState::SetFrontendProfile(TRuntimeProfileNode profile) {
+ // Should we defer creating and adding the child until here? probably.
+ TRuntimeProfileTree prof_tree;
+ prof_tree.nodes.emplace_back(std::move(profile));
+ frontend_profile_->Update(prof_tree);
+}
+
Status ClientRequestState::Exec(TExecRequest* exec_request) {
MarkActive();
exec_request_ = *exec_request;
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/be/src/service/client-request-state.h
----------------------------------------------------------------------
diff --git a/be/src/service/client-request-state.h b/be/src/service/client-request-state.h
index 9442a0d..7ff5285 100644
--- a/be/src/service/client-request-state.h
+++ b/be/src/service/client-request-state.h
@@ -66,6 +66,11 @@ class ClientRequestState {
~ClientRequestState();
+ /// Sets the profile that is produced by the frontend. The frontend creates the
+ /// profile during planning and returns it to the backend via TExecRequest,
+ /// which then sets the frontend profile.
+ void SetFrontendProfile(TRuntimeProfileNode profile);
+
/// Based on query type, this either initiates execution of a exec_request or submits
/// the query to the Admission controller for asynchronous admission control. When this
/// returns the operation state is either RUNNING_STATE or PENDING_STATE.
@@ -352,6 +357,9 @@ class ClientRequestState {
/// The ClientRequestState builds three separate profiles.
/// * profile_ is the top-level profile which houses the other
/// profiles, plus the query timeline
+ /// * frontend_profile_ is the profile emitted by the frontend
+ /// during planning. Added to summary_profile_ so as to avoid
+ /// breaking other tools that depend on the profile_ layout.
/// * summary_profile_ contains mostly static information about the
/// query, including the query statement, the plan and the user who submitted it.
/// * server_profile_ tracks time spent inside the ImpalaServer,
@@ -370,6 +378,7 @@ class ClientRequestState {
/// - Query Status
/// - Error logs
RuntimeProfile* const profile_;
+ RuntimeProfile* const frontend_profile_;
RuntimeProfile* const server_profile_;
RuntimeProfile* const summary_profile_;
RuntimeProfile::Counter* row_materialization_timer_;
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/be/src/service/impala-server.cc
----------------------------------------------------------------------
diff --git a/be/src/service/impala-server.cc b/be/src/service/impala-server.cc
index 649f1fc..af9414c 100644
--- a/be/src/service/impala-server.cc
+++ b/be/src/service/impala-server.cc
@@ -965,6 +965,7 @@ Status ImpalaServer::ExecuteInternal(
(*request_state)->set_user_profile_access(result.user_has_profile_access);
(*request_state)->summary_profile()->AddEventSequence(
result.timeline.name, result.timeline);
+ (*request_state)->SetFrontendProfile(result.profile);
if (result.__isset.result_set_metadata) {
(*request_state)->set_result_metadata(result.result_set_metadata);
}
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/be/src/util/runtime-profile.h
----------------------------------------------------------------------
diff --git a/be/src/util/runtime-profile.h b/be/src/util/runtime-profile.h
index cae2462..a6b06ba 100644
--- a/be/src/util/runtime-profile.h
+++ b/be/src/util/runtime-profile.h
@@ -263,7 +263,7 @@ class RuntimeProfile { // NOLINT: This struct is not packed, but there are not s
Counter* inactive_timer() { return counter_map_[INACTIVE_TIME_COUNTER_NAME]; }
int64_t local_time() { return local_time_ns_; }
- /// Prints the counters in a name: value format.
+ /// Prints the contents of the profile in a name: value format.
/// Does not hold locks when it makes any function calls.
void PrettyPrint(std::ostream* s, const std::string& prefix="") const;
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/common/thrift/Frontend.thrift
----------------------------------------------------------------------
diff --git a/common/thrift/Frontend.thrift b/common/thrift/Frontend.thrift
index f8cd472..ffbfc07 100644
--- a/common/thrift/Frontend.thrift
+++ b/common/thrift/Frontend.thrift
@@ -634,6 +634,7 @@ struct TExecRequest {
10: optional TSetQueryOptionRequest set_query_option_request
// Timeline of planner's operation, for profiling
+ // TODO(todd): should integrate this with the 'profile' member instead.
11: optional RuntimeProfile.TEventSequence timeline
// If false, the user that runs this statement doesn't have access to the runtime
@@ -643,6 +644,9 @@ struct TExecRequest {
// Set iff stmt_type is ADMIN_FN.
13: optional TAdminRequest admin_request
+
+ // Profile information from the planning process.
+ 14: optional RuntimeProfile.TRuntimeProfileNode profile
}
// Parameters to FeSupport.cacheJar().
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java b/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
index 2684c33..62f1d3e 100644
--- a/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
+++ b/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
@@ -48,6 +48,7 @@ import org.apache.impala.common.InternalException;
import org.apache.impala.common.Pair;
import org.apache.impala.common.Reference;
import org.apache.impala.service.FeSupport;
+import org.apache.impala.service.FrontendProfile;
import org.apache.impala.thrift.CatalogLookupStatus;
import org.apache.impala.thrift.TBackendGflags;
import org.apache.impala.thrift.TCatalogInfoSelector;
@@ -66,6 +67,7 @@ import org.apache.impala.thrift.TPartialPartitionInfo;
import org.apache.impala.thrift.TTable;
import org.apache.impala.thrift.TTableInfoSelector;
import org.apache.impala.thrift.TUniqueId;
+import org.apache.impala.thrift.TUnit;
import org.apache.impala.thrift.TUpdateCatalogCacheRequest;
import org.apache.impala.thrift.TUpdateCatalogCacheResponse;
import org.apache.impala.util.ListMap;
@@ -81,6 +83,7 @@ import org.slf4j.LoggerFactory;
import com.google.common.annotations.VisibleForTesting;
import com.google.common.base.Objects;
import com.google.common.base.Preconditions;
+import com.google.common.base.Stopwatch;
import com.google.common.base.Throwables;
import com.google.common.cache.Cache;
import com.google.common.cache.CacheBuilder;
@@ -185,6 +188,25 @@ public class CatalogdMetaProvider implements MetaProvider {
*/
private static final Object DB_LIST_CACHE_KEY = new Object();
+ private static final String CATALOG_FETCH_PREFIX = "CatalogFetch";
+ private static final String DB_LIST_STATS_CATEGORY = "DatabaseList";
+ private static final String DB_METADATA_STATS_CATEGORY = "Databases";
+ private static final String TABLE_NAMES_STATS_CATEGORY = "TableNames";
+ private static final String TABLE_METADATA_CACHE_CATEGORY = "Tables";
+ private static final String PARTITION_LIST_STATS_CATEGORY = "PartitionLists";
+ private static final String PARTITIONS_STATS_CATEGORY = "Partitions";
+ private static final String COLUMN_STATS_STATS_CATEGORY = "ColumnStats";
+ private static final String GLOBAL_CONFIGURATION_STATS_CATEGORY = "Config";
+ private static final String FUNCTION_LIST_STATS_CATEGORY = "FunctionLists";
+ private static final String FUNCTIONS_STATS_CATEGORY = "Functions";
+ private static final String RPC_STATS_CATEGORY = "RPCs";
+ private static final String RPC_REQUESTS =
+ CATALOG_FETCH_PREFIX + "." + RPC_STATS_CATEGORY + ".Requests";
+ private static final String RPC_BYTES =
+ CATALOG_FETCH_PREFIX + "." + RPC_STATS_CATEGORY + ".Bytes";
+ private static final String RPC_TIME =
+ CATALOG_FETCH_PREFIX + "." + RPC_STATS_CATEGORY + ".Time";
+
/**
* File descriptors store replicas using a compressed format that references hosts
* by index in a "host index" list rather than by their full addresses. Since we cache
@@ -290,11 +312,20 @@ public class CatalogdMetaProvider implements MetaProvider {
TGetPartialCatalogObjectRequest req)
throws TException {
TGetPartialCatalogObjectResponse resp;
- byte[] ret;
+ byte[] ret = null;
+ Stopwatch sw = new Stopwatch().start();
try {
ret = FeSupport.GetPartialCatalogObject(new TSerializer().serialize(req));
} catch (InternalException e) {
throw new TException(e);
+ } finally {
+ sw.stop();
+ FrontendProfile profile = FrontendProfile.getCurrentOrNull();
+ if (profile != null) {
+ profile.addToCounter(RPC_REQUESTS, TUnit.NONE, 1);
+ profile.addToCounter(RPC_BYTES, TUnit.BYTES, ret == null ? 0 : ret.length);
+ profile.addToCounter(RPC_TIME, TUnit.TIME_MS, sw.elapsed(TimeUnit.MILLISECONDS));
+ }
}
resp = new TGetPartialCatalogObjectResponse();
new TDeserializer().deserialize(resp, ret);
@@ -343,13 +374,15 @@ public class CatalogdMetaProvider implements MetaProvider {
@SuppressWarnings("unchecked")
private <CacheKeyType, ValueType> ValueType loadWithCaching(String itemString,
- CacheKeyType key, final Callable<ValueType> loadCallable) throws TException {
+ String statsCategory, CacheKeyType key,
+ final Callable<ValueType> loadCallable) throws TException {
// TODO(todd): there a race here if an invalidation comes in while we are
// fetching here. Perhaps we need some locking, or need to remember the
// version numbers of the invalidation messages and ensure that we don't
// 'put' an element with a too-old version? See:
// https://softwaremill.com/race-condition-cache-guava-caffeine/
final Reference<Boolean> hit = new Reference<>(true);
+ Stopwatch sw = new Stopwatch().start();
try {
return (ValueType)cache_.get(key, new Callable<ValueType>() {
@Override
@@ -362,13 +395,39 @@ public class CatalogdMetaProvider implements MetaProvider {
Throwables.propagateIfPossible(e.getCause(), TException.class);
throw new RuntimeException(e);
} finally {
+ sw.stop();
+ addStatsToProfile(statsCategory, /*numHits=*/hit.getRef() ? 1 : 0,
+ /*numMisses=*/hit.getRef() ? 0 : 1, sw);
LOG.trace("Request for {}: {}", itemString, hit.getRef() ? "hit" : "miss");
}
}
+ /**
+ * Adds basic statistics to the query's profile when accessing cache entries.
+ * For each cache request, the number of hits, misses, and elapsed time is aggregated.
+ * Cache requests for different types of cache entries, such as function names vs.
+ * table names, are differentiated by a 'statsCategory'.
+ */
+ private void addStatsToProfile(String statsCategory, int numHits, int numMisses,
+ Stopwatch stopwatch) {
+ FrontendProfile profile = FrontendProfile.getCurrentOrNull();
+ if (profile == null) return;
+ final String prefix = CATALOG_FETCH_PREFIX + "." +
+ Preconditions.checkNotNull(statsCategory) + ".";
+ profile.addToCounter(prefix + "Requests", TUnit.NONE, numHits + numMisses);;
+ profile.addToCounter(prefix + "Time", TUnit.TIME_MS,
+ stopwatch.elapsed(TimeUnit.MILLISECONDS));
+ if (numHits > 0) {
+ profile.addToCounter(prefix + "Hits", TUnit.NONE, numHits);
+ }
+ if (numMisses > 0) {
+ profile.addToCounter(prefix + "Misses", TUnit.NONE, numMisses);
+ }
+ }
+
@Override
public ImmutableList<String> loadDbList() throws TException {
- return loadWithCaching("database list", DB_LIST_CACHE_KEY,
+ return loadWithCaching("database list", DB_LIST_STATS_CATEGORY, DB_LIST_CACHE_KEY,
new Callable<ImmutableList<String>>() {
@Override
public ImmutableList<String> call() throws Exception {
@@ -415,6 +474,7 @@ public class CatalogdMetaProvider implements MetaProvider {
@Override
public Database loadDb(final String dbName) throws TException {
return loadWithCaching("database metadata for " + dbName,
+ DB_METADATA_STATS_CATEGORY,
new DbCacheKey(dbName, DbCacheKey.DbInfoType.HMS_METADATA),
new Callable<Database>() {
@Override
@@ -433,6 +493,7 @@ public class CatalogdMetaProvider implements MetaProvider {
public ImmutableList<String> loadTableNames(final String dbName)
throws MetaException, UnknownDBException, TException {
return loadWithCaching("table names for database " + dbName,
+ TABLE_NAMES_STATS_CATEGORY,
new DbCacheKey(dbName, DbCacheKey.DbInfoType.TABLE_NAMES),
new Callable<ImmutableList<String>>() {
@Override
@@ -474,6 +535,7 @@ public class CatalogdMetaProvider implements MetaProvider {
TableCacheKey cacheKey = new TableCacheKey(dbName, tableName);
TableMetaRefImpl ref = loadWithCaching(
"table metadata for " + dbName + "." + tableName,
+ TABLE_METADATA_CACHE_CATEGORY,
cacheKey,
new Callable<TableMetaRefImpl>() {
@Override
@@ -493,6 +555,7 @@ public class CatalogdMetaProvider implements MetaProvider {
@Override
public List<ColumnStatisticsObj> loadTableColumnStatistics(final TableMetaRef table,
List<String> colNames) throws TException {
+ Stopwatch sw = new Stopwatch().start();
List<ColumnStatisticsObj> ret = Lists.newArrayListWithCapacity(colNames.size());
// Look up in cache first, keeping track of which ones are missing.
// We can't use 'loadWithCaching' since we need to fetch several entries batched
@@ -534,6 +597,9 @@ public class CatalogdMetaProvider implements MetaProvider {
NEGATIVE_COLUMN_STATS_SENTINEL);
}
}
+ sw.stop();
+ addStatsToProfile(COLUMN_STATS_STATS_CATEGORY,
+ hitCount + negativeHitCount, missingCols.size(), sw);
LOG.trace("Request for column stats of {}: hit {}/ neg hit {} / miss {}",
table, hitCount, negativeHitCount, missingCols.size());
return ret;
@@ -544,8 +610,8 @@ public class CatalogdMetaProvider implements MetaProvider {
public List<PartitionRef> loadPartitionList(final TableMetaRef table)
throws TException {
PartitionListCacheKey key = new PartitionListCacheKey((TableMetaRefImpl) table);
- return (List<PartitionRef>) loadWithCaching(
- "partition list for " + table, key, new Callable<List<PartitionRef>>() {
+ return (List<PartitionRef>) loadWithCaching("partition list for " + table,
+ PARTITION_LIST_STATS_CATEGORY, key, new Callable<List<PartitionRef>>() {
/** Called to load cache for cache misses */
@Override
public List<PartitionRef> call() throws Exception {
@@ -574,13 +640,13 @@ public class CatalogdMetaProvider implements MetaProvider {
throws MetaException, TException {
Preconditions.checkArgument(table instanceof TableMetaRefImpl);
TableMetaRefImpl refImpl = (TableMetaRefImpl)table;
-
+ Stopwatch sw = new Stopwatch().start();
// Load what we can from the cache.
Map<PartitionRef, PartitionMetadata> refToMeta = loadPartitionsFromCache(refImpl,
hostIndex, partitionRefs);
- LOG.trace("Request for partitions of {}: hit {}/{}", table, refToMeta.size(),
- partitionRefs.size());
+ final int numHits = refToMeta.size();
+ final int numMisses = partitionRefs.size() - numHits;
// Load the remainder from the catalogd.
List<PartitionRef> missingRefs = Lists.newArrayList();
@@ -594,6 +660,10 @@ public class CatalogdMetaProvider implements MetaProvider {
// Write back to the cache.
storePartitionsInCache(refImpl, hostIndex, fromCatalogd);
}
+ sw.stop();
+ addStatsToProfile(PARTITIONS_STATS_CATEGORY, refToMeta.size(), numMisses, sw);
+ LOG.trace("Request for partitions of {}: hit {}/{}", table, refToMeta.size(),
+ partitionRefs.size());
// Convert the returned map to be by-name instead of by-ref.
Map<String, PartitionMetadata> nameToMeta = Maps.newHashMapWithExpectedSize(
@@ -724,7 +794,9 @@ public class CatalogdMetaProvider implements MetaProvider {
@Override
public String loadNullPartitionKeyValue() throws MetaException, TException {
return (String) loadWithCaching("null partition key value",
- NULL_PARTITION_KEY_VALUE_CACHE_KEY, new Callable<String>() {
+ GLOBAL_CONFIGURATION_STATS_CATEGORY,
+ NULL_PARTITION_KEY_VALUE_CACHE_KEY,
+ new Callable<String>() {
/** Called to load cache for cache misses */
@Override
public String call() throws Exception {
@@ -736,6 +808,7 @@ public class CatalogdMetaProvider implements MetaProvider {
@Override
public List<String> loadFunctionNames(final String dbName) throws TException {
return loadWithCaching("function names for database " + dbName,
+ FUNCTION_LIST_STATS_CATEGORY,
new DbCacheKey(dbName, DbCacheKey.DbInfoType.FUNCTION_NAMES),
new Callable<ImmutableList<String>>() {
@Override
@@ -755,6 +828,7 @@ public class CatalogdMetaProvider implements MetaProvider {
final String functionName) throws TException {
ImmutableList<TFunction> thriftFuncs = loadWithCaching(
"function " + dbName + "." + functionName,
+ FUNCTIONS_STATS_CATEGORY,
new FunctionsCacheKey(dbName, functionName),
new Callable<ImmutableList<TFunction>>() {
@Override
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/fe/src/main/java/org/apache/impala/service/Frontend.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/service/Frontend.java b/fe/src/main/java/org/apache/impala/service/Frontend.java
index 39983ea..d31b573 100644
--- a/fe/src/main/java/org/apache/impala/service/Frontend.java
+++ b/fe/src/main/java/org/apache/impala/service/Frontend.java
@@ -1061,11 +1061,14 @@ public class Frontend {
throws ImpalaException {
// Timeline of important events in the planning process, used for debugging
// and profiling.
- EventSequence timeline = new EventSequence("Query Compilation");
- TExecRequest result = getTExecRequest(queryCtx, timeline, explainString);
- timeline.markEvent("Planning finished");
- result.setTimeline(timeline.toThrift());
- return result;
+ try (FrontendProfile.Scope scope = FrontendProfile.createNewWithScope()) {
+ EventSequence timeline = new EventSequence("Query Compilation");
+ TExecRequest result = getTExecRequest(queryCtx, timeline, explainString);
+ timeline.markEvent("Planning finished");
+ result.setTimeline(timeline.toThrift());
+ result.setProfile(FrontendProfile.getCurrent().emitAsThrift());
+ return result;
+ }
}
/**
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/fe/src/main/java/org/apache/impala/service/FrontendProfile.java
----------------------------------------------------------------------
diff --git a/fe/src/main/java/org/apache/impala/service/FrontendProfile.java b/fe/src/main/java/org/apache/impala/service/FrontendProfile.java
new file mode 100644
index 0000000..3344bf6
--- /dev/null
+++ b/fe/src/main/java/org/apache/impala/service/FrontendProfile.java
@@ -0,0 +1,163 @@
+// Licensed to the Apache Software Foundation (ASF) under one
+// or more contributor license agreements. See the NOTICE file
+// distributed with this work for additional information
+// regarding copyright ownership. The ASF licenses this file
+// to you under the Apache License, Version 2.0 (the
+// "License"); you may not use this file except in compliance
+// with the License. You may obtain a copy of the License at
+//
+// http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing,
+// software distributed under the License is distributed on an
+// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+// KIND, either express or implied. See the License for the
+// specific language governing permissions and limitations
+// under the License.
+package org.apache.impala.service;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+
+import javax.annotation.Nonnull;
+import javax.annotation.Nullable;
+import javax.annotation.concurrent.ThreadSafe;
+
+import org.apache.impala.thrift.TCounter;
+import org.apache.impala.thrift.TRuntimeProfileNode;
+import org.apache.impala.thrift.TUnit;
+
+import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableMap;
+import com.google.errorprone.annotations.concurrent.GuardedBy;
+
+/**
+ * Wrapper class for creating a runtime profile within the frontend.
+ *
+ * In order to avoid plumbing an object through all code that might want to emit counters
+ * into the profile, this class provides some support for storing a current profile in
+ * a thread-local variable.
+ *
+ * This class is thread-safe.
+ */
+@ThreadSafe
+public class FrontendProfile {
+ private static final String ROOT_COUNTER_NAME = "";
+
+ private static ThreadLocal<FrontendProfile> THREAD_LOCAL =
+ new ThreadLocal<>();
+
+ @GuardedBy("this")
+ private TRuntimeProfileNode profile_;
+
+ /**
+ * Name-based access to the counters in the profile_.counters List<TCounter>.
+ */
+ @GuardedBy("this")
+ private final Map<String, TCounter> countersByName_ = new HashMap<>();
+
+ FrontendProfile() {
+ profile_ = new TRuntimeProfileNode("Frontend",
+ /*num_children=*/ 0,
+ /*counters=*/new ArrayList<>(),
+ /*metadata=*/-1L, // TODO(todd) what is this used for? why is it required?
+ /*indent=*/false,
+ /*info_strings=*/new HashMap<>(),
+ /*info_strings_display_order*/new ArrayList<>(),
+ /*child_counters_map=*/ImmutableMap.of(ROOT_COUNTER_NAME, new HashSet<>()));
+ }
+
+ /**
+ * Create a new profile, setting it as the current thread-local profile for the
+ * length of the current scope. This is meant to be used in a try-with-resources
+ * statement. Supports at most one scope per thread. No nested scopes are currently
+ * allowed.
+ */
+ public static Scope createNewWithScope() {
+ return new Scope(new FrontendProfile());
+ }
+
+ /**
+ * Get the profile attached to the current thread, throw IllegalStateException if there
+ * is none.
+ */
+ @Nonnull
+ public static FrontendProfile getCurrent() {
+ FrontendProfile prof = THREAD_LOCAL.get();
+ Preconditions.checkState(prof != null, "no profile in scope");
+ return prof;
+ }
+
+ /**
+ * Get the profile attached to the current thread, or null if there is no current
+ * profile.
+ */
+ @Nullable
+ public static FrontendProfile getCurrentOrNull() {
+ return THREAD_LOCAL.get();
+ }
+
+ /**
+ * Return the profile in Thrift format. This may be called only once, and after it is
+ * called, no further methods may be used on this PlannerProfile object. Any attempts
+ * to do so will result in IllegalStateExceptions.
+ */
+ public synchronized TRuntimeProfileNode emitAsThrift() {
+ Preconditions.checkState(profile_ != null, "already emitted profile");
+ TRuntimeProfileNode ret = profile_;
+ profile_ = null;
+ return ret;
+ }
+
+ /**
+ * Add an informational key/value string pair to the profile. These are written out
+ * as is to the user. Subsequent calls with the same key will overwrite previous ones.
+ */
+ public synchronized void addInfoString(String key, String val) {
+ Preconditions.checkState(profile_ != null, "already emitted profile");
+ Preconditions.checkNotNull(key);
+ Preconditions.checkNotNull(val);
+ if (profile_.getInfo_strings().put(key, val) == null) {
+ // If it's a new info string instead of replacing an existing one,
+ // we need to also include it in the 'ordering' list.
+ profile_.getInfo_strings_display_order().add(key);
+ }
+ }
+
+ /**
+ * Add 'delta' to the counter with the given name and unit. Counters are created
+ * on-demand.
+ */
+ public synchronized void addToCounter(String name, TUnit unit, long delta) {
+ Preconditions.checkState(profile_ != null, "already emitted profile");
+ TCounter counter = countersByName_.get(Preconditions.checkNotNull(name));
+ if (counter == null) {
+ // Need to create the counter.
+ counter = new TCounter(name, unit, 0);
+ countersByName_.put(name, counter);
+ profile_.counters.add(counter);
+ // Currently we don't support hierarchical counters in the frontend.
+ profile_.child_counters_map.get(ROOT_COUNTER_NAME).add(name);
+ }
+ counter.value += delta;
+ }
+
+
+ public static class Scope implements AutoCloseable {
+ private final FrontendProfile oldThreadLocalValue_;
+
+ private Scope(FrontendProfile profile) {
+ oldThreadLocalValue_ = THREAD_LOCAL.get();
+ // TODO: remove when allowing nested scopes.
+ Preconditions.checkState(oldThreadLocalValue_ == null);
+ THREAD_LOCAL.set(profile);
+ }
+
+ @Override
+ public void close() {
+ THREAD_LOCAL.set(oldThreadLocalValue_);
+ }
+ }
+}
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
----------------------------------------------------------------------
diff --git a/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java b/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
index 74264c6..de6dd07 100644
--- a/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
+++ b/fe/src/test/java/org/apache/impala/catalog/local/CatalogdMetaProviderTest.java
@@ -19,6 +19,7 @@ package org.apache.impala.catalog.local;
import static org.junit.Assert.*;
+import java.util.Collections;
import java.util.List;
import java.util.Map;
@@ -31,11 +32,13 @@ import org.apache.impala.catalog.local.MetaProvider.PartitionRef;
import org.apache.impala.catalog.local.MetaProvider.TableMetaRef;
import org.apache.impala.common.Pair;
import org.apache.impala.service.FeSupport;
+import org.apache.impala.service.FrontendProfile;
import org.apache.impala.thrift.TBackendGflags;
import org.apache.impala.thrift.TCatalogObject;
import org.apache.impala.thrift.TCatalogObjectType;
import org.apache.impala.thrift.TDatabase;
import org.apache.impala.thrift.TNetworkAddress;
+import org.apache.impala.thrift.TRuntimeProfileNode;
import org.apache.impala.thrift.TTable;
import org.apache.impala.util.ListMap;
import org.junit.Test;
@@ -221,4 +224,21 @@ public class CatalogdMetaProviderTest {
assertEquals(0, stats.hitCount());
assertEquals(1, stats.missCount());
}
+
+ @Test
+ public void testProfile() throws Exception {
+ FrontendProfile profile;
+ try (FrontendProfile.Scope scope = FrontendProfile.createNewWithScope()) {
+ provider_.loadTable("functional", "alltypes");
+ profile = FrontendProfile.getCurrent();
+ }
+ TRuntimeProfileNode prof = profile.emitAsThrift();
+ assertEquals(3, prof.counters.size());
+ Collections.sort(prof.counters);
+ assertEquals("TCounter(name:CatalogFetch.Tables.Hits, unit:NONE, value:1)",
+ prof.counters.get(0).toString());
+ assertEquals("TCounter(name:CatalogFetch.Tables.Requests, unit:NONE, value:1)",
+ prof.counters.get(1).toString());
+ assertEquals("CatalogFetch.Tables.Time", prof.counters.get(2).name);
+ }
}
\ No newline at end of file
http://git-wip-us.apache.org/repos/asf/impala/blob/e6bbe4ea/tests/custom_cluster/test_local_catalog.py
----------------------------------------------------------------------
diff --git a/tests/custom_cluster/test_local_catalog.py b/tests/custom_cluster/test_local_catalog.py
index b78ef88..14a9a54 100644
--- a/tests/custom_cluster/test_local_catalog.py
+++ b/tests/custom_cluster/test_local_catalog.py
@@ -306,9 +306,8 @@ class TestCompactCatalogUpdates(CustomClusterTestSuite):
for _ in xrange(0, 10):
for query in queries_to_test:
ret = self.execute_query_expect_success(client, query)
- # TODO: re-enable checks when counters are put back into profile
- # assert ret.runtime_profile.count("Frontend:") == 1
- # assert ret.runtime_profile.count("CatalogFetch") > 1
+ assert ret.runtime_profile.count("Frontend:") == 1
+ assert ret.runtime_profile.count("CatalogFetch") > 1
cache_metrics = self.get_catalog_cache_metrics(impalad)
cache_hit_rate = cache_metrics[cache_hit_rate_metric_key]
cache_miss_rate = cache_metrics[cache_miss_rate_metric_key]