[impala] 02/04: IMPALA-8252: Only write node_metadata if it's set

[bo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

boroknagyz pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/impala.git

commit 8b143f53036657fc74180def4fb8276d57ca179a
Author: Lars Volker <lv...@cloudera.com>
AuthorDate: Tue Feb 26 16:24:06 2019 -0800

    IMPALA-8252: Only write node_metadata if it's set
    
    Since IMPALA-1048 we write TRuntimeProfileNode.node_metadata
    unconditionally, even when both its fields are unset. This trips up the
    Thrift library Java reader code, which expects to find exactly one type
    of a union to be set.
    
    This change fixes the issue by checking whether at least one of the
    fields is set before marking node_metadata as set.
    
    This change also adds a test to runtime-profile-test.cc to make sure
    that the __isset flag is set correctly. Additionally I wrote a profile
    and then manually read it with a Java client.
    
    Change-Id: Ice1efcf75daba5c023566eb63d4e921c228c26fd
    Reviewed-on: http://gerrit.cloudera.org:8080/12616
    Reviewed-by: Lars Volker <lv...@cloudera.com>
    Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
 be/src/util/CMakeLists.txt          |  2 +-
 be/src/util/runtime-profile-test.cc | 17 +++++++++++++++++
 be/src/util/runtime-profile.cc      |  6 +++++-
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/be/src/util/CMakeLists.txt b/be/src/util/CMakeLists.txt
index 0a26372..8acb5ef 100644
--- a/be/src/util/CMakeLists.txt
+++ b/be/src/util/CMakeLists.txt
@@ -187,7 +187,7 @@ ADD_UNIFIED_BE_LSAN_TEST(redactor-config-parser-test ParserTest.*)
 ADD_UNIFIED_BE_LSAN_TEST(redactor-test "RedactorTest.*")
 ADD_UNIFIED_BE_LSAN_TEST(redactor-unconfigured-test "RedactorUnconfigTest.*")
 ADD_UNIFIED_BE_LSAN_TEST(rle-test "BitArray.*:RleTest.*")
-ADD_UNIFIED_BE_LSAN_TEST(runtime-profile-test "CountersTest.*:TimerCounterTest.*:TimeSeriesCounterTest.*:VariousNumbers/TimeSeriesCounterResampleTest.*")
+ADD_UNIFIED_BE_LSAN_TEST(runtime-profile-test "CountersTest.*:TimerCounterTest.*:TimeSeriesCounterTest.*:VariousNumbers/TimeSeriesCounterResampleTest.*:ToThrift.*")
 ADD_UNIFIED_BE_LSAN_TEST(string-parser-test "StringToInt.*:StringToIntWithBase.*:StringToFloat.*:StringToBool.*")
 ADD_UNIFIED_BE_LSAN_TEST(string-util-test "TruncateDownTest.*:TruncateUpTest.*:CommaSeparatedContainsTest.*")
 ADD_UNIFIED_BE_LSAN_TEST(symbols-util-test "SymbolsUtil.*")
diff --git a/be/src/util/runtime-profile-test.cc b/be/src/util/runtime-profile-test.cc
index e07994e..795ef29 100644
--- a/be/src/util/runtime-profile-test.cc
+++ b/be/src/util/runtime-profile-test.cc
@@ -1141,5 +1141,22 @@ INSTANTIATE_TEST_CASE_P(VariousNumbers, TimeSeriesCounterResampleTest,
     "120, 123, 126 (Showing 43 of 129 values from Thrift Profile)"})
     ));
 
+// Tests that the __isset field for TRuntimeProfileNode.node_metadata is set correctly
+// (IMPALA-8252).
+TEST(ToThrift, NodeMetadataIsSetCorrectly) {
+  ObjectPool pool;
+  RuntimeProfile* profile = RuntimeProfile::Create(&pool, "Profile");
+  TRuntimeProfileTree thrift_profile;
+  profile->ToThrift(&thrift_profile);
+  // Profile is empty, expect 0 nodes
+  EXPECT_EQ(thrift_profile.nodes.size(), 1);
+  EXPECT_FALSE(thrift_profile.nodes[0].__isset.node_metadata);
+
+  // Set the plan node ID and make sure that the field is marked correctly
+  profile->SetPlanNodeId(1);
+  profile->ToThrift(&thrift_profile);
+  EXPECT_TRUE(thrift_profile.nodes[0].__isset.node_metadata);
+}
+
 } // namespace impala
 
diff --git a/be/src/util/runtime-profile.cc b/be/src/util/runtime-profile.cc
index ec7417b..8f91b81 100644
--- a/be/src/util/runtime-profile.cc
+++ b/be/src/util/runtime-profile.cc
@@ -937,7 +937,11 @@ void RuntimeProfile::ToThrift(vector<TRuntimeProfileNode>* nodes) const {
   // Set the required metadata field to the plan node ID for compatibility with any tools
   // that rely on the plan node id being set there.
   node.metadata = metadata_.__isset.plan_node_id ? metadata_.plan_node_id : -1;
-  node.__set_node_metadata(metadata_);
+  // Thrift requires exactly one field of a union to be set so we only mark node_metadata
+  // as set if that is the case.
+  if (metadata_.__isset.plan_node_id || metadata_.__isset.data_sink_id) {
+    node.__set_node_metadata(metadata_);
+  }
   node.indent = true;
 
   CounterMap counter_map;