You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Daniel Becker (Code Review)" <ge...@cloudera.org> on 2022/12/06 10:12:12 UTC

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Daniel Becker has uploaded this change for review. ( http://gerrit.cloudera.org:8080/19309


Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections when
using the HS2 protocol.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
M be/src/service/hs2-util.cc
1 file changed, 115 insertions(+), 20 deletions(-)



  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/2
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Gergely Fürnstáhl (Code Review)" <ge...@cloudera.org>.
Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/19309/2/be/src/service/hs2-util.cc
File be/src/service/hs2-util.cc:

http://gerrit.cloudera.org:8080/#/c/19309/2/be/src/service/hs2-util.cc@409
PS2, Line 409:   if (is_map) {
4 if(is_map) does not feel right here. At the very least, is_map should be const, but I think the best would be to separate the array/map writing logic into different functions.


http://gerrit.cloudera.org:8080/#/c/19309/2/be/src/service/hs2-util.cc@467
PS2, Line 467: element_type.type == TYPE_MAP
No need to convert this to bool, using the enum is better for the reader imo, but the best would be still to separate the array and the map logic.



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 13:46:50 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections for
both the HS2 and the Beeswax protocol.

The old code handling the printing of collections in raw-value.{h,cc} is
removed.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Reviewed-on: http://gerrit.cloudera.org:8080/19309
Tested-by: Impala Public Jenkins <im...@cloudera.com>
Reviewed-by: Daniel Becker <da...@cloudera.com>
---
A be/src/runtime/complex-value-writer.h
A be/src/runtime/complex-value-writer.inline.h
M be/src/runtime/raw-value.cc
M be/src/runtime/raw-value.h
M be/src/service/hs2-util.cc
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
12 files changed, 435 insertions(+), 143 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Daniel Becker: Looks good to me, approved

-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 14
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#6). ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections for
both the HS2 and the Beeswax protocol.

The old code handling the printing of collections in raw-value.{h,cc} is
removed.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/complex-value-writer.cc
A be/src/runtime/complex-value-writer.h
M be/src/runtime/raw-value.cc
M be/src/runtime/raw-value.h
M be/src/service/hs2-util.cc
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
13 files changed, 444 insertions(+), 142 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/6
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 6
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 10: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8905/


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 10
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 22:39:36 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 11:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8913/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 11
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 11:50:12 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#5). ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections for
both the HS2 and the Beeswax protocol.

The old code handling the printing of collections in raw-value.{h,cc} is
removed.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/complex-value-writer.cc
A be/src/runtime/complex-value-writer.h
M be/src/runtime/raw-value.cc
M be/src/runtime/raw-value.h
M be/src/service/hs2-util.cc
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
13 files changed, 437 insertions(+), 140 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/5
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/19309/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19309/2//COMMIT_MSG@13
PS2, Line 13: HS2
As we have discussed offline, it would be nicer to have the same logic in HS2 and Beeswax and delete the old json writer code for collections. A new class could be created with responsibility of writing complex types to JSON with rapidjson and this could be called both from HS2 and Beeswax. This would also allow returning structs from beeswax.


http://gerrit.cloudera.org:8080/#/c/19309/2/be/src/service/hs2-util.cc
File be/src/service/hs2-util.cc:

http://gerrit.cloudera.org:8080/#/c/19309/2/be/src/service/hs2-util.cc@535
PS2, Line 535: 
             :       rapidjson::StringBuffer buffer;
optimization idea: reusing the buffer in the FOREACH_ROW_LIMIT loop could reduce the number of allocations

rapidjson::StringBuffer has a Clear() function that resets the buffer without actually deleting the allocated memory, so it could be called after buffer.GetString()



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 15:55:11 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 11: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 11
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 11:50:11 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 13: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 13
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Dec 2022 15:04:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 13:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12035/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 13
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Dec 2022 10:12:23 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 9:

The failing test is flaky.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 9
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 17:29:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 12:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8917/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 12
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 17:06:44 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 10: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 10
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 17:29:40 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lipenglin@sensorsdata.cn has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 2:

Not a problem, I am just wondering. 
Is there any test in hs2-util-test.cc for this? Or maybe we don't need to test it.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 15:01:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 12: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8917/


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 12
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 22:13:00 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 13:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8921/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 13
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Dec 2022 09:52:48 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 13: Verified+1


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 13
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 15 Dec 2022 15:01:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8905/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 10
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 17:29:41 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 11:

The failure of metadata.test_load.TestLoadData.test_load may be related to 
IMPALA-11794 (https://gerrit.cloudera.org/#/c/19350/)


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 11
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 11:53:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 8: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 8
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 11:36:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 12: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 12
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 17:06:43 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 7:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12020/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 10:14:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12013/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 5
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Fri, 09 Dec 2022 17:10:27 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#13). ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections for
both the HS2 and the Beeswax protocol.

The old code handling the printing of collections in raw-value.{h,cc} is
removed.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
A be/src/runtime/complex-value-writer.h
A be/src/runtime/complex-value-writer.inline.h
M be/src/runtime/raw-value.cc
M be/src/runtime/raw-value.h
M be/src/service/hs2-util.cc
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
12 files changed, 435 insertions(+), 143 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/13
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 13
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/runtime/complex-value-writer.cc
File be/src/runtime/complex-value-writer.cc:

http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/runtime/complex-value-writer.cc@49
PS6, Line 49: tring tmp;
            :   RawValue::PrintValue(value, type, -1, &tmp);
> This branch could return before calling PrintValue.
Done


http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/service/query-result-set.cc
File be/src/service/query-result-set.cc:

http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/service/query-result-set.cc@238
PS6, Line 238: 
> I don't think that we really need to pass scale - this probably comes from 
Removed the 'scale' parameter from the functions.



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 09:54:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.h
File be/src/runtime/complex-value-writer.h:

http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.h@35
PS7, Line 35: template <class JsonStream>
            : class ComplexValueWriter {
> nit: putting the implementation into a *.inline.h would prevent the need of
Done


http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.cc
File be/src/runtime/complex-value-writer.cc:

http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.cc@50
PS7, Line 50: 
> Please add this as a const class member and give it some comments why we ar
I made it a constexpr variable here because this is the only place we use it in this class so I didn't want to pollute the class definition.



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 8
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 10:25:15 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Gergely Fürnstáhl (Code Review)" <ge...@cloudera.org>.
Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 8: Code-Review+1

LGTM


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 8
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 11:22:51 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#3). ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections when
using the HS2 protocol.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
M be/src/service/hs2-util.cc
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
6 files changed, 271 insertions(+), 28 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/3
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/19309/2//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/19309/2//COMMIT_MSG@13
PS2, Line 13: HS2
> Created a new class with the json writing logic. I'm not sure if I'll add B
I think that this should depend on the complexity of the change.
- it would be much better to avoid having a different logic for collections in HS2 and Beeswax
- if already doing it for collections in Beeswax, then adding struct support also shouldn't be too hard, but I have no problem with moving this to another patch



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 4
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Dec 2022 16:34:48 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#4). ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections when
using the HS2 protocol.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/complex-value-writer.cc
A be/src/runtime/complex-value-writer.h
M be/src/service/hs2-util.cc
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
9 files changed, 366 insertions(+), 46 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/4
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 4
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/11971/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 10:32:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Peter Rozsa (Code Review)" <ge...@cloudera.org>.
Peter Rozsa has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/19309/1/be/src/service/hs2-util.cc
File be/src/service/hs2-util.cc:

http://gerrit.cloudera.org:8080/#/c/19309/1/be/src/service/hs2-util.cc@397
PS1, Line 397:   const int item_byte_size = item_tuple_desc->byte_size();
This section (dchecks, starts, stops) could be separated as MapToJSON and ArrayToJson. It would require less conditionals and the key/value conversion would be more visible (L433 is a bit cryptic in this manner)


http://gerrit.cloudera.org:8080/#/c/19309/1/be/src/service/hs2-util.cc@528
PS1, Line 528:     } else {
We could use coll_val as well



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 2
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 06 Dec 2022 14:43:54 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Csaba Ringhofer (Code Review)" <ge...@cloudera.org>.
Csaba Ringhofer has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/runtime/complex-value-writer.cc
File be/src/runtime/complex-value-writer.cc:

http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/runtime/complex-value-writer.cc@49
PS6, Line 49:  else if (type.IsBooleanType() && !map_key) {
            :     writer->Bool( *(reinterpret_cast<bool*>(value)) );
This branch could return before calling PrintValue.


http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/service/query-result-set.cc
File be/src/service/query-result-set.cc:

http://gerrit.cloudera.org:8080/#/c/19309/6/be/src/service/query-result-set.cc@238
PS6, Line 238: scale
I don't think that we really need to pass scale - this probably comes from a misunderstanding during array implementation by me / Attila Jeges.

According to https://github.com/apache/impala/blob/master/be/src/exprs/scalar-expr-evaluator.h#L201 output scale is only set to non -1 in RoundUpTo(), which makes it irrelevant for complex types. So my guess is that always passing -1 or removing it wouldn't break any tests.

I think that we should have the same handling in HS2 and Beeswax, so either:
a. always use default scale in both protocols
b. add a way to configure this

I would vote for a. and adding a config only if someone requests it.



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 6
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Dec 2022 22:11:42 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#7). ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections for
both the HS2 and the Beeswax protocol.

The old code handling the printing of collections in raw-value.{h,cc} is
removed.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
M be/src/runtime/CMakeLists.txt
A be/src/runtime/complex-value-writer.cc
A be/src/runtime/complex-value-writer.h
M be/src/runtime/raw-value.cc
M be/src/runtime/raw-value.h
M be/src/service/hs2-util.cc
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
13 files changed, 442 insertions(+), 143 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/7
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12018/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 6
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Mon, 12 Dec 2022 15:04:08 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 9: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8901/


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 9
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 17:12:00 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 9: Code-Review+2


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 9
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 11:46:19 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 11: Verified-1

Build failed: https://jenkins.impala.io/job/gerrit-verify-dryrun/8913/


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 11
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 16:58:09 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Anonymous Coward (Code Review)" <ge...@cloudera.org>.
lipenglin@sensorsdata.cn has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 3:

Got it! Thanks for your explanation.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Dec 2022 10:26:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/11998/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 3
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 07 Dec 2022 17:16:14 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Gergely Fürnstáhl (Code Review)" <ge...@cloudera.org>.
Gergely Fürnstáhl has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 7: Code-Review+1

(2 comments)

Added a few nitpicks, but generally LGTM!

http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.h
File be/src/runtime/complex-value-writer.h:

http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.h@35
PS7, Line 35: // rapidjson::BasicOStreamWrapper<std::stringstream>, the class template is explicitly
            : // instantiated for these types.
nit: putting the implementation into a *.inline.h would prevent the need of explicit instantiation and the user could just include the implementation if needed. As I understand this is our preferred way based on the style guide, but it has to be noted it is a pivot from the Google C++ Style Guide.


http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.cc
File be/src/runtime/complex-value-writer.cc:

http://gerrit.cloudera.org:8080/#/c/19309/7/be/src/runtime/complex-value-writer.cc@50
PS7, Line 50: -1
Please add this as a const class member and give it some comments why we are passing -1



-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 7
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 09:57:09 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 9:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/8901/ DRY_RUN=false


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 9
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 11:46:20 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 8:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12021/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 8
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Tue, 13 Dec 2022 10:45:03 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 10:

Now a different test failed: metadata.test_load.TestLoadData.test_load.
Doesn't seem to be related and passes locally. Trying one more time.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 10
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Wed, 14 Dec 2022 11:49:50 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Daniel Becker (Code Review)" <ge...@cloudera.org>.
Daniel Becker has uploaded a new patch set (#8). ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................

IMPALA-11717: Use rapidjson for printing collections

We have been using rapidjson to print structs but didn't use it to print
collections (arrays and maps).

This change introduces the usage of rapidjson to print collections for
both the HS2 and the Beeswax protocol.

The old code handling the printing of collections in raw-value.{h,cc} is
removed.

Testing:
 - Ran existing EE tests
 - Added EE tests with non-string and NULL map keys in
   nested-map-in-select-list.test and map_null_keys.test.

Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
---
A be/src/runtime/complex-value-writer.h
A be/src/runtime/complex-value-writer.inline.h
M be/src/runtime/raw-value.cc
M be/src/runtime/raw-value.h
M be/src/service/hs2-util.cc
M be/src/service/query-result-set.cc
M be/src/service/query-result-set.h
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
A testdata/workloads/functional-query/queries/QueryTest/map_null_keys.test
M testdata/workloads/functional-query/queries/QueryTest/nested-map-in-select-list.test
M tests/query_test/test_nested_types.py
12 files changed, 435 insertions(+), 143 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/09/19309/8
-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 8
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>

[Impala-ASF-CR] IMPALA-11717: Use rapidjson for printing collections

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has posted comments on this change. ( http://gerrit.cloudera.org:8080/19309 )

Change subject: IMPALA-11717: Use rapidjson for printing collections
......................................................................


Patch Set 4:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/12007/ : Initial code review checks passed. Use gerrit-verify-dryrun-external or gerrit-verify-dryrun to run full precommit tests.


-- 
To view, visit http://gerrit.cloudera.org:8080/19309
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I08a2d596a498fbbaf1419b18284846b992f49165
Gerrit-Change-Number: 19309
Gerrit-PatchSet: 4
Gerrit-Owner: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Anonymous Coward <li...@sensorsdata.cn>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Daniel Becker <da...@cloudera.com>
Gerrit-Reviewer: Gergely Fürnstáhl <gf...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Peter Rozsa <pr...@cloudera.com>
Gerrit-Comment-Date: Thu, 08 Dec 2022 16:37:21 +0000
Gerrit-HasComments: No