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 <l...@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 <l...@cloudera.com> Tested-by: Impala Public Jenkins <impala-public-jenk...@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;