You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@impala.apache.org by "Fredy Wijaya (Code Review)" <ge...@cloudera.org> on 2019/03/06 19:53:30 UTC

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options 

Fredy Wijaya has uploaded this change for review. ( http://gerrit.cloudera.org:8080/12682


Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................

IMPALA-8271: Refactor the use of Thrift enums for query options

This patch refactors the use of Thrift enums with GetThriftEnum helper
function that can automatically validate and convert the query option
value into the corresponding Thrift enum value. The validation error
message has also been improved to list all possible valid query option
values.

Testing:
- Added missing test cases in both BE and E2E
- Ran query-options-test.cc
- Ran metadata/test_set.py
- Ran query_test/test_nested_types.py
- Ran query_test/test_scanners.py

Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/parquet-ambiguous-list-modern.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-resolution-by-name.test
M testdata/workloads/functional-query/queries/QueryTest/set.test
5 files changed, 85 insertions(+), 109 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@341
PS2, Line 341: iequals(value, "true") || iequals(value, "1")
optional: if you are already cleaning things up, these could be replaced with a IsTrue(value) after creating an IsTrue() function.


http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@343
PS2, Line 343: TImpalaQueryOptions::REPLICA_PREFERENCE:
Was this query option left out intentionally?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 17:38:08 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 6:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2397/ : 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/12682
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 19:34:29 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 7:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 18:36:53 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 5: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/12682/5/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/12682/5/be/src/service/query-options.cc@504
PS5, Line 504:             &enum_type));
nit: could fit in one less lines



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 18:54:12 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 6:

Thanks for cleaning this up btw, I appreciate it when people take the time to make parts of the code more pleasant to work with.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 18:31:16 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 6: Code-Review+1

(1 comment)

Carry Csaba's +1. Tim, can you take a look at it for the +2?

http://gerrit.cloudera.org:8080/#/c/12682/5/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/12682/5/be/src/service/query-options.cc@504
PS5, Line 504:         query_options->__
> nit: could fit in one less lines
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 19:02:47 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 2:

I don't have time to fully review this but a quick skim looks good - thanks for making this part of the code more sane!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 21:02:06 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 7: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 18:36:52 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 5:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@343
PS2, Line 343:  {0, "CACHE_LOCAL"},
> I see. There are some other query options where not all values are actually
Done. We can actually just pass our own map in GetThriftEnum for the list of valid enum values.


http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@365
PS2, Line 365: 
> Specifying the template does not seem necessary here, as it can be deduced 
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 18:10:19 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 6: Code-Review+2

I'll promote to a +2 based on Csaba's +1 and a quick once-over.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 18:24:05 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 7: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 7
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 14 Mar 2019 22:43:49 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 5:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2395/ : 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/12682
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 18:32:46 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................

IMPALA-8271: Refactor the use of Thrift enums for query options

This patch refactors the use of Thrift enums with GetThriftEnum helper
function that can automatically validate and convert the query option
value into the corresponding Thrift enum value. The validation error
message has also been improved to list all possible valid query option
values.

Testing:
- Added missing test cases in both BE and E2E
- Ran query-options-test.cc
- Ran metadata/test_set.py
- Ran query_test/test_nested_types.py
- Ran query_test/test_scanners.py

Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/parquet-ambiguous-list-modern.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-resolution-by-name.test
M testdata/workloads/functional-query/queries/QueryTest/set.test
5 files changed, 110 insertions(+), 147 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................

IMPALA-8271: Refactor the use of Thrift enums for query options

This patch refactors the use of Thrift enums with GetThriftEnum helper
function that can automatically validate and convert the query option
value into the corresponding Thrift enum value. The validation error
message has also been improved to list all possible valid query option
values.

Testing:
- Added missing test cases in both BE and E2E
- Ran query-options-test.cc
- Ran metadata/test_set.py
- Ran query_test/test_nested_types.py
- Ran query_test/test_scanners.py

Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/parquet-ambiguous-list-modern.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-resolution-by-name.test
M testdata/workloads/functional-query/queries/QueryTest/set.test
5 files changed, 126 insertions(+), 160 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 2:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2377/ : 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/12682
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 2
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Wed, 06 Mar 2019 20:37:22 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

Posted by "Impala Public Jenkins (Code Review)" <ge...@cloudera.org>.
Impala Public Jenkins has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/12682 )

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................

IMPALA-8271: Refactor the use of Thrift enums for query options

This patch refactors the use of Thrift enums with GetThriftEnum helper
function that can automatically validate and convert the query option
value into the corresponding Thrift enum value. The validation error
message has also been improved to list all possible valid query option
values.

Testing:
- Added missing test cases in both BE and E2E
- Ran query-options-test.cc
- Ran metadata/test_set.py
- Ran query_test/test_nested_types.py
- Ran query_test/test_scanners.py

Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Reviewed-on: http://gerrit.cloudera.org:8080/12682
Reviewed-by: Impala Public Jenkins <im...@cloudera.com>
Tested-by: Impala Public Jenkins <im...@cloudera.com>
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/parquet-ambiguous-list-modern.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-resolution-by-name.test
M testdata/workloads/functional-query/queries/QueryTest/set.test
5 files changed, 125 insertions(+), 160 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 3:

Build Successful 

https://jenkins.impala.io/job/gerrit-code-review-checks/2386/ : 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/12682
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 19:27:57 +0000
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................

IMPALA-8271: Refactor the use of Thrift enums for query options

This patch refactors the use of Thrift enums with GetThriftEnum helper
function that can automatically validate and convert the query option
value into the corresponding Thrift enum value. The validation error
message has also been improved to list all possible valid query option
values.

Testing:
- Added missing test cases in both BE and E2E
- Ran query-options-test.cc
- Ran metadata/test_set.py
- Ran query_test/test_nested_types.py
- Ran query_test/test_scanners.py

Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/parquet-ambiguous-list-modern.test
M testdata/workloads/functional-query/queries/QueryTest/parquet-resolution-by-name.test
M testdata/workloads/functional-query/queries/QueryTest/set.test
5 files changed, 125 insertions(+), 160 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@343
PS2, Line 343: uery_options->__set_replica_preference(T
> Yeah this is an odd one because we don't remove the unused options, .i.e. C
I see. There are some other query options where not all values are actually supported, e.g. I am sure that parquet_compression_codec will only work with a subset of codecs. default_file_format is also questionable, as Impala can create all kinds of tables, but cannot insert into some of them, so I am not sure if it makes sense to allow those as defaults. I am ok with the current state of the patch, but it may make sense to filter unsupported enums, e.g. by adding an optional argument to GetThriftEnum with the list of valid values.


http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@365
PS2, Line 365: 
Specifying the template does not seem necessary here, as it can be deduced from argument 'enum_type'.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Fri, 08 Mar 2019 17:17:02 +0000
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-8271: Refactor the use of Thrift enums for query options

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

Change subject: IMPALA-8271: Refactor the use of Thrift enums for query options
......................................................................


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc
File be/src/service/query-options.cc:

http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@341
PS2, Line 341: ImpalaQueryOptions::REPLICA_PREFERENCE:
> optional: if you are already cleaning things up, these could be replaced wi
Done


http://gerrit.cloudera.org:8080/#/c/12682/2/be/src/service/query-options.cc@343
PS2, Line 343: uery_options->__set_replica_preference(T
> Was this query option left out intentionally?
Yeah this is an odd one because we don't remove the unused options, .i.e. CACHE_RACK(1) and DISK_RACK(3) but using those options will throw an error. To keep the existing behavior, I leave this option out.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I6d747aae2c689765be72e117ce030ce4e3ce4641
Gerrit-Change-Number: 12682
Gerrit-PatchSet: 3
Gerrit-Owner: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Csaba Ringhofer <cs...@cloudera.com>
Gerrit-Reviewer: Fredy Wijaya <fw...@cloudera.com>
Gerrit-Reviewer: Impala Public Jenkins <im...@cloudera.com>
Gerrit-Reviewer: Tim Armstrong <ta...@cloudera.com>
Gerrit-Comment-Date: Thu, 07 Mar 2019 18:48:03 +0000
Gerrit-HasComments: Yes