[Impala-ASF-CR] Revert "IMPALA-5589: change "set" in impala-shell to show empty string for unset query options"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: Revert "IMPALA-5589: change "set" in impala-shell to show empty 
string for unset query options"
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] Revert "IMPALA-5589: change "set" in impala-shell to show empty string for unset query options"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: Revert "IMPALA-5589: change "set" in impala-shell to show empty 
string for unset query options"
..


Revert "IMPALA-5589: change "set" in impala-shell to show empty string for 
unset query options"

Due to re-use of connections in the test infrastructure, this commit
is causing ASAN failures in the builds. That is being worked out
as part of IMPALA-5908, but, in the meanwhile, it's prudent
to revert.

This reverts commit 387bde0639ffd8ef580ccbf727152954e62bacbe.

Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
Reviewed-on: http://gerrit.cloudera.org:8080/8087
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
M tests/shell/test_shell_commandline.py
7 files changed, 60 insertions(+), 106 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/exec/filter-context.cc
File be/src/exec/filter-context.cc:

Line 217: Status FilterContext::CodegenInsert(
> It would be nice not to add the IRBuilder code but we don't have a good pat
I'm still learning here. I don't see a loop here. What makes this ineligible 
for cross-compilation?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 6: Code-Review-1

Dan pointed out an important point that unsetting might mean that we should 
*not* set the mask. I'm working out what that means in terms of implementation 
and adding a test. In the meanwhile, https://gerrit.cloudera.org/#/c/8087/ is 
the revert of the original change.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5199: prevent hang on empty row batch exchange

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5199: prevent hang on empty row batch exchange
..


IMPALA-5199: prevent hang on empty row batch exchange

The error path where delivery of "eos" fails now behaves
the same as if delivery of a row batch fails.

Testing:
Added a timeout test where the leaf fragments return 0 rows. Before
the change this reproduced the hang.

Change-Id: Ib370ebe44e3bb34d3f0fb9f05aa6386eb91c8645
Reviewed-on: http://gerrit.cloudera.org:8080/8005
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/data-stream-mgr.cc
M testdata/workloads/functional-query/queries/QueryTest/exchange-delays.test
2 files changed, 23 insertions(+), 4 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Ib370ebe44e3bb34d3f0fb9f05aa6386eb91c8645
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5199: prevent hang on empty row batch exchange

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5199: prevent hang on empty row batch exchange
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib370ebe44e3bb34d3f0fb9f05aa6386eb91c8645
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3877: support unpatched LLVM

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-3877: support unpatched LLVM
..


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dfbe44ed8a1464b9b0991fd54e72b194ad6155d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3877: support unpatched LLVM

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-3877: support unpatched LLVM
..


IMPALA-3877: support unpatched LLVM

The p1 patch we use for LLVM avoided merging of structurally
identical Struct types in unpredictable ways when linking in
IR UDF modules. This avoided hitting type assertions when
generating calls to IR UDfs.

This implements an alternative solution, which is to bitcast
the arguments when calling IR UDFs. This means we do not need to carry
the patch when we upgrade LLVM.

Testing:
Ran core tests with unpatched LLVM 3.8, including the IR UDF test that
originally required the patch to pass.

Change-Id: I3dfbe44ed8a1464b9b0991fd54e72b194ad6155d
Reviewed-on: http://gerrit.cloudera.org:8080/7973
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/codegen/CMakeLists.txt
M be/src/codegen/codegen-anyval.cc
A be/src/codegen/codegen-util.cc
A be/src/codegen/codegen-util.h
M be/src/codegen/llvm-codegen.h
M be/src/exprs/expr-codegen-test.cc
6 files changed, 158 insertions(+), 5 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I3dfbe44ed8a1464b9b0991fd54e72b194ad6155d
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] Revert "IMPALA-5589: change "set" in impala-shell to show empty string for unset query options"

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: Revert "IMPALA-5589: change "set" in impala-shell to show empty 
string for unset query options"
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-HasComments: No

[Impala-ASF-CR] Revert "IMPALA-5589: change "set" in impala-shell to show empty string for unset query options"

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: Revert "IMPALA-5589: change "set" in impala-shell to show empty 
string for unset query options"
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1229/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] Revert "IMPALA-5589: change "set" in impala-shell to show empty string for unset query options"

[Philip Zeyliger (Code Review)]

Philip Zeyliger has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8087

Change subject: Revert "IMPALA-5589: change "set" in impala-shell to show empty 
string for unset query options"
..

Revert "IMPALA-5589: change "set" in impala-shell to show empty string for 
unset query options"

Due to re-use of connections in the test infrastructure, this commit
is causing ASAN failures in the builds. That is being worked out
as part of IMPALA-5908, but, in the meanwhile, it's prudent
to revert.

This reverts commit 387bde0639ffd8ef580ccbf727152954e62bacbe.

Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/hs2/hs2_test_suite.py
M tests/hs2/test_hs2.py
M tests/shell/test_shell_commandline.py
7 files changed, 60 insertions(+), 106 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/87/8087/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8087
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: I41bc8ab0f1df45bbd311030981a7c18989c2edc8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8070

to look at the new patch set (#6).

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET =""' will now unset option, reverting it to
its default state.

This change became necessary with "SET" started returning an
empty string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it
thinks is its defaults, and, when this broke, some ASAN builds
started to fail, presumably due to a timing issue with how
we re-use connections between tests.

Testing:
* Added a simple test that triggered this side-effect without this code.
  Specifically, "impala-python infra/python/env/bin/py.test 
tests/metadata/test_set.py -s"
  with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
  defaulted and non-defaulted values.
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
6 files changed, 67 insertions(+), 8 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Hello Impala Public Jenkins, Bharath Vissapragada, Dan Hecht, Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8070

to look at the new patch set (#5).

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..

IMPALA-5908: Allow SET to unset modified query options.

The query 'SET =""' will now unset option, reverting it to
its default state.

This change became necessary with "SET" started returning an
empty string for unset options which don't have a default. The test
infrastructure (impala_test_suite.py) resets options to what it
thinks is its defaults, and, when this broke, some ASAN builds
started to fail, presumably due to a timing issue with how
we re-use connections between tests.

Testing:
* Added a simple test that triggered this side-effect without this code.
  Specifically, "impala-python infra/python/env/bin/py.test 
tests/metadata/test_set.py -s"
  with the modified set.test triggers.
* Added cases to query-options-test to check behavior for both
  defaulted and non-defaulted values.
* Ran an ASAN build where this was triggering previously.

Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M be/src/service/query-options.h
M bin/bootstrap_development.sh
M common/thrift/ImpalaInternalService.thrift
M testdata/workloads/functional-query/queries/QueryTest/set.test
M tests/common/impala_test_suite.py
7 files changed, 214 insertions(+), 130 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 5:

How embarrassing! New patch uploaded. The relevant test passes now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5538: Use explicit catalog versions for deleted objects

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5538: Use explicit catalog versions for deleted objects
..


Patch Set 5:

(16 comments)

http://gerrit.cloudera.org:8080/#/c/7731/5/be/src/catalog/catalog-server.cc
File be/src/catalog/catalog-server.cc:

PS5, Line 231:   // If this is not a delta update, request an update from 
version 0 from the local
 :   // catalog. There is an optimization that checks if 
pending_topic_updates_ was just
 :   // reloaded from version 0. If so, then skip this step and use 
that data.
I found it impossible to understand this comment without reading through most 
of the code in this file, which kinda defeats the point of having a comment :) 
i think it would be clearer if it said something like:

If not generating a delta update and 'pending_topic_updates_' doesn't already 
contain the full catalog (beginning with version 0), then force 
GatherCatalogUpdatesThread() to reload the full catalog.


PS5, Line 234: delta.from_version == 0 && delta.to_version == 0
from the comment above, presumably this condition means "this is not a delta 
update"?  Why is that the condition and not 'delta.is_delta'?


PS5, Line 310: if (catalog_object.catalog_version <= 
last_sent_catalog_version_) continue;
given that last_sent_catalog_version_ was passed to GetAllCatalogObjects(), why 
is this needed?


http://gerrit.cloudera.org:8080/#/c/7731/5/be/src/service/impala-server.cc
File be/src/service/impala-server.cc:

PS5, Line 1392: 
can we delete this function now?


http://gerrit.cloudera.org:8080/#/c/7731/5/be/src/statestore/statestore.cc
File be/src/statestore/statestore.cc:

PS5, Line 177: entry_it->second.SetDeleted(true);
 : entry_it->second.SetVersion(last_version_);
now that some subscribes require deleted entries to have a value, how does this 
all work? are we implicitly requiring that only non-transient subscribers 
depend on values for deletions?  If that's the case, that seems fragile (and at 
the least should be documented).


PS5, Line 490:  
would be easier to read if you line break it there to keep the last arg all on 
one line.


PS5, Line 538: if (topic_entry.value() != Statestore::TopicEntry::NULL_VALUE)
why do we need that guard? why can't we set topic_item.value even in that case?
assuming we don't need it, it seems like we can remove the NULL_VALUE constant 
altogether.


PS5, Line 546: int64_t topic_size = topic.total_key_size_bytes() - 
deleted_key_size_bytes
 : + topic.total_value_size_bytes();
that math doesn't look correct anymore (deleted entries can have non-zero value 
size).  it might be more straight forward to just add up the topic size as we 
go rather than try to subtract out the deleted portion.


http://gerrit.cloudera.org:8080/#/c/7731/5/be/src/statestore/statestore.h
File be/src/statestore/statestore.h:

PS5, Line 180: may
may or will?


http://gerrit.cloudera.org:8080/#/c/7731/5/common/thrift/CatalogInternalService.thrift
File common/thrift/CatalogInternalService.thrift:

PS5, Line 25: GetAllCatalogObjects
see below. I think we should rename this.


PS5, Line 25: Contains all known Catalog objects
: // (databases, tables/views, and functions) from the 
CatalogService's cache.
I think that should be updated to explain that this is relative to a particular 
catalog version and that it now expresses it as an update and delete.


PS5, Line 38: empty list if no objects detected in the Catalog
> Yes, there is some context that is missing here, hence it's not easy to und
Thanks. As we talked about in person, I think we should rename 
GetAllCatalogObjects to GetCatalogDelta() or something similar to make it clear 
that this gets the delta. There's no hint at that until you read the header 
file comment.


http://gerrit.cloudera.org:8080/#/c/7731/5/common/thrift/StatestoreService.thrift
File common/thrift/StatestoreService.thrift:

Line 84:   // is not included in the topic key (e.g. catalog version of deleted 
catalog objects).
> Let me try to explain. Statestore only uses that flag to avoid sending dele
Please see my comments elsewhere in this file for suggestions on clarifying the 
interface.

It still seems confusing and weird that 'value' can matter for delete when 
delete, but I guess the comments I'm suggested elsewhere help clarify at least 
what 'delete' actually means.


PS5, Line 87: If true, this item was deleted
How about:

If true, this item was deleted.  When false, this TTopicItem need not be 
included in non-delta TTopicDelta's (since the complete set of TTopicItems will 
be included in that case).

or something like that.


PS5, Line 97: List of changes to topic entries, including deletions.
That's only true when is_delta==true.  So, how about we say:

When is_delta=true, a list of changes to topic entries, including deletions, 
within [from_version, to_version].
When 

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 4:

That last error looks like an infra issue but it does looks like there was a 
test failure earlier (I ctrl-fed for "failed"):

  20:43:48 ] [ RUN  ] QueryOptions.ResetToDefaultViaEmptyString
20:43:48 ] /home/ubuntu/Impala/be/src/service/query-options-test.cc:170: Failure
20:43:48 ] Value of: expectedMask
20:43:48 ]   Actual: 
1000
20:43:48 ] Expected: mask
20:43:48 ] Which is: 

20:43:48 ] [  FAILED  ] QueryOptions.ResetToDefaultViaEmptyString (0 ms)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 2: Code-Review+1

(4 comments)

Looks good to me minus a couple of things. Not sure if Dan wants to take a look

http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/exec/filter-context.cc
File be/src/exec/filter-context.cc:

Line 216: 
Forgot in the previous pass: can you include an example of the IR it generates?


Line 217: Status FilterContext::CodegenInsert(
It would be nice not to add the IRBuilder code but we don't have a good path 
yet to automatically unroll loops in cross-compiled code. This seems pretty 
reasonable as far as IRBuilder code - I didn't see any obvious opportunities to 
simplify things.


http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

Line 935: Status PhjBuilder::CodegenInsertRuntimeFilters(
It would be good to have an example of the IR here too.


http://gerrit.cloudera.org:8080/#/c/8029/2/be/src/util/bloom-filter-ir.cc
File be/src/util/bloom-filter-ir.cc:

PS2, Line 24: IR_ALWAYS_INLINE
Is the always_inline needed here?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 4:

I'm not an expert at reading these yet, but I think it's complaining about the 
following. This doesn't smell like something my change could have caused.

Am I misreading the logs?

https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/281/console
20:43:45 [WS-CLEANUP] Deleting project workspace...Cannot delete workspace 
:remote file operation failed: /home/ubuntu at 
hudson.remoting.Channel@44f2e578:ubuntu-16.04 (i-04402f53583d90ca1): 
java.io.IOException: Unable to delete '/home/ubuntu'. Tried 3 times (of a 
maximum of 3) waiting 0.1 sec between attempts.
20:43:53 ERROR: Step ‘Delete workspace when build is done’ failed: Cannot 
delete workspace: remote file operation failed: /home/ubuntu at 
hudson.remoting.Channel@44f2e578:ubuntu-16.04 (i-04402f53583d90ca1): 
java.io.IOException: Unable to delete '/home/ubuntu'. Tried 3 times (of a 
maximum of 3) waiting 0.1 sec between attempts.
20:43:53 Finished: FAILURE

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new patch set (#2).

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..

IMPALA-3360: Codegen inserting into runtime filters

This patch codegens PhjBuilder::InsertRuntimeFilters() and
FilterContext::Insert().

This allows us to unroll the loop over all the filters in
PhjBuilder::ProcessBuildBatch(), eliminate the branch on type that
happens in RawValue::GetHashValue(), and eliminate the AVX check
that happens in BloomFilter::Insert().

Testing:
- Ran existing runtime filter tests.
- Ran perf tests locally (all avg. over three runs):
  - Four way self join on tpch_parquet.lineitem. Should be a good case
for this as there's several large hash join build sides that will
benefit from the codegen. Total query running time improved ~7%
(from 16.07s to 14.91s).
  - Single join of tpch_parquet.lineitem against a selectively
filtered tpch_parquet.lineitem. Should be a bad case for this
patch, as the build side of the join is very small. Total query
running time regressed by about ~2% (from 0.73s to 0.75s) due to
an increase in codegen time (from 295ms to 309ms for the fragment
containing the hash join).

Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
---
M be/src/codegen/gen_ir_descriptions.py
M be/src/codegen/impala-ir.cc
M be/src/exec/filter-context.cc
M be/src/exec/filter-context.h
M be/src/exec/partitioned-hash-join-builder-ir.cc
M be/src/exec/partitioned-hash-join-builder.cc
M be/src/exec/partitioned-hash-join-builder.h
M be/src/util/CMakeLists.txt
A be/src/util/bloom-filter-ir.cc
M be/src/util/bloom-filter.h
10 files changed, 246 insertions(+), 9 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3360: Codegen inserting into runtime filters

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3360: Codegen inserting into runtime filters
..


Patch Set 2:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/8029/1/be/src/exec/filter-context.h
File be/src/exec/filter-context.h:

Line 124:   static Status CodegenInsert(LlvmCodeGen* codegen, ScalarExpr* 
filter_expr,
> This is a bit tricky because it doesn't actually do exactly the same thing 
Done


http://gerrit.cloudera.org:8080/#/c/8029/1/be/src/exec/partitioned-hash-join-builder.cc
File be/src/exec/partitioned-hash-join-builder.cc:

Line 953:   for (int i = 0; i < num_filters; ++i) {
> Nit: it looks like the two branches are identical except for adding the NoI
Done


Line 958: Value* filter_context_ptr =
> Going forward, we want to avoid have this dependence between the codegen co
Done


http://gerrit.cloudera.org:8080/#/c/8029/1/be/src/util/bloom-filter-ir.cc
File be/src/util/bloom-filter-ir.cc:

Line 32: }
> It would be nice to keep this in header file to avoid regressing the non-co
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I79cf23ad92dadaab996a50a2ca07ef9ebe8639bb
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 10: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


IMPALA-5211: Simplifying nullif conditional.

This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.

As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.

It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)

Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
  distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.

Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Reviewed-on: http://gerrit.cloudera.org:8080/7829
Reviewed-by: Alex Behm 
Tested-by: Impala Public Jenkins
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
19 files changed, 205 insertions(+), 122 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

[Pranay Singh (Code Review)]

Pranay Singh has uploaded a new patch set (#3).

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..

IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

test_scanners_fuzz.py currently tests compressed parquet but
does not test uncompressed parquet. This fix adds a new test
case for uncompressed parquet.

Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
---
M tests/query_test/test_scanners_fuzz.py
1 file changed, 24 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new patch set (#5).

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..

IMPALA-5895: clean up runtime profile lifecycle

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 437 insertions(+), 410 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5895: clean up runtime profile lifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8069

Change subject: IMPALA-5895: clean up runtime profile lifecycle
..

IMPALA-5895: clean up runtime profile lifecycle

TODO: cleanup bucketing counters?

Require callers to explicitly stop counter updating instead of doing it
in the destructor. This replaces ad-hoc logic to stop individual
counters.

Track which counters need to be stopped in separate lists instead of
stopping everything.

Force all RuntimeProfiles to use ObjectPools in a uniform way - the
profile, its counters and its children all are allocated from the
same pool. This is done via a new Create() method.

Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
---
M be/src/benchmarks/hash-benchmark.cc
M be/src/codegen/llvm-codegen.cc
M be/src/codegen/llvm-codegen.h
M be/src/exec/data-sink.cc
M be/src/exec/data-source-scan-node.cc
M be/src/exec/exec-node.cc
M be/src/exec/exec-node.h
M be/src/exec/hash-table-test.cc
M be/src/exec/hbase-scan-node.cc
M be/src/exec/hdfs-scan-node-base.cc
M be/src/exec/hdfs-scan-node-base.h
M be/src/exec/kudu-scan-node-base.cc
M be/src/exec/kudu-scan-node-base.h
M be/src/exec/kudu-scan-node-mt.cc
M be/src/exec/scan-node.cc
M be/src/exec/scan-node.h
M be/src/experiments/data-provider-test.cc
M be/src/experiments/tuple-splitter-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/bufferpool/buffer-allocator-test.cc
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/coordinator-backend-state.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-recvr.cc
M be/src/runtime/data-stream-recvr.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/service/client-request-state.cc
M be/src/service/client-request-state.h
M be/src/service/impala-server.cc
M be/src/util/dummy-runtime-profile.h
M be/src/util/periodic-counter-updater.cc
M be/src/util/periodic-counter-updater.h
M be/src/util/runtime-profile-test.cc
M be/src/util/runtime-profile.cc
M be/src/util/runtime-profile.h
43 files changed, 437 insertions(+), 410 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I45c39ac36c8e3c277213d32f5ae5f14be6b7f0df
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8056/2/tests/query_test/test_scanners_fuzz.py
File tests/query_test/test_scanners_fuzz.py:

PS2, Line 97: """
> I'll use the functional-parquet to create the cloned table which will be pa
One thing that I failed to mention clearly enough is that the uncompressed 
parquet table should not be created in the default schemas. Instead, it should 
be created in unique_database. This is why the function signature of 
run_fuzz_test will change. The normal tests will pass in table_format and a 
table from the default schema. Your new test will pass in unique_database and 
the table you created.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5416: Fix an impala-shell command recursion bug

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-5416: Fix an impala-shell command recursion bug
..


Patch Set 4: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I453af2d4694d47e184031cb07ecd2af259ba20f3
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

[Pranay Singh (Code Review)]

Pranay Singh has uploaded a new patch set (#3).

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..

IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

test_scanners_fuzz.py currently tests compressed parquet but
does not test uncompressed parquet. This fix adds a new test
case for uncompressed parquet.

Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
---
M tests/query_test/test_scanners_fuzz.py
1 file changed, 24 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

[Pranay Singh (Code Review)]

Pranay Singh has uploaded a new patch set (#3).

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..

IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

test_scanners_fuzz.py currently tests compressed parquet but
does not test uncompressed parquet. This fix adds a new test
case for uncompressed parquet.

Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
---
M tests/query_test/test_scanners_fuzz.py
1 file changed, 23 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 6:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 20: #include 
> We are generally trying to reduce our dependencies on Boost libraries. Can 
See comment on line 171.


Line 39: constexpr auto i32_max = numeric_limits::max();
> We currently use auto only for iterators and const& in loops.
Removed some autos.


Line 73: TEST(QueryOptions, SetByteOptions) {
> Can you add a comment explaining what the test does overall? Same for the o
Done


Line 96:   auto process_case_set = [&](auto& case_set) {
> I find this part hard to follow and its use of lambdas seems to deviate fro
Flattened them into template functions.


PS6, Line 97: kase
> Can you think of a better name?
Done


PS6, Line 99: minus_1_to_0
> Why is this needed? Is the added level of indirection worth it?
Removed.


PS6, Line 137:   // Expands to {"entry", prefix::entry},
 : #define ENTRY(_, prefix, entry) {BOOST_PP_STRINGIZE(entry), 
prefix::entry},
 :   // ENTRIES(prefix, (a)(b)) expands to {"a", prefix::a}, {"b", 
prefix::b},
 : #define ENTRIES(prefix, name) BOOST_PP_SEQ_FOR_EACH(ENTRY, 
prefix, name)
 :   // A case is a pair: (keydef, [(enum_string, enum_value)])
 : #define CASE(key, enumtype, enums) make_pair(key, \
 :   vector\
 :   {ENTRIES(enumtype, BOOST_PP_TUPLE_TO_SEQ(enums))})
> I find this very hard to understand. Can you have a look at the Google Test
I'm not sure which part I should look at. The complication here is that I need 
a test case to include:
1. the string name of that option "prefetch_mode"
2. A reference to the member in TQueryOptions: _mode
3. A list of enum value, and their string representation: 
   vector<...>{make_pair("NONE", TPrefetchMode::NONE), make_pair("HT_BUCKET", 
TPrefetchMode::HT_BUCKET)}

I added an example to show what expanded code looks like. If no macros are used 
here, this part of code would be really long and repetitive. I think though the 
macros are complicated, the test cases are clear and easy to modify. By looking 
at "CASE(KEY(prefetch_mode), TPrefetchMode, (NONE, HT_BUCKET))" it's easy to 
know how to add/modify a test case.


Line 171: fusion::for_each(case_set, [, accept_integer](auto& kase) 
{
> Can you use C++11 for_each instead?
Not that easy. Let me explain:
The complication here is that, each enum query option is of different type. For 
example, the type of options.prefetch_mode is TPrefetchMode::type. So the 
enum_and_int is actually a tuple of different types, and that's why I used 
make_tuple instead of a vector. What we need here is to iterate through the 
tuple, and that is not what std::for_each can do.


Line 263: for (auto& key_def : {key_def_min, key_def_default}) {
> const auto&?
Done.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

[Pranay Singh (Code Review)]

Pranay Singh has posted comments on this change.

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8056/2/tests/query_test/test_scanners_fuzz.py
File tests/query_test/test_scanners_fuzz.py:

PS2, Line 97: fq_tbl_name = "functional_parquet" + "." + tbl_name
> Do not create a table in the default schemas. Instead, with the changes I d
I'll use the functional-parquet to create the cloned table which will be passed 
to fuzz_test


PS2, Line 129: self.execute_query("create table %s.%s like %s" % 
(unique_database, table, table))
 : fuzz_table_location = 
get_fs_path("/test-warehouse/{0}.db/{1}".format(
 : unique_database, table))
> Pull this logic out into its own function e.g. clone_table. run_fuzz_test s
I'll retain the old functionality as per your latest comments


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded a new patch set (#7).

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..

IMPALA-5425: Add test for validating input when setting query options

This patch adds multiple query option validation testcases to
be/src/service/query-options-test.cc
The test cases include parsing edge cases, bondary values, special
cases for some options and some testcases moved from
testdata/workloads/functional-query/queries/QueryTest/set.test
This patch also fixes a bug generating wrong error message for
query option RUNTIME_FILTER_WAIT_TIME_MS.

Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
---
M be/src/service/query-options-test.cc
M be/src/service/query-options.cc
M testdata/workloads/functional-query/queries/QueryTest/set.test
3 files changed, 267 insertions(+), 153 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 

[Impala-ASF-CR] Increment version to 2.11.0-SNAPSHOT

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: Increment version to 2.11.0-SNAPSHOT
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2a60fbc5f2c1a9ba9697e6f1114bdf18997aa92c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] Increment version to 2.11.0-SNAPSHOT

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: Increment version to 2.11.0-SNAPSHOT
..


Increment version to 2.11.0-SNAPSHOT

Change-Id: I2a60fbc5f2c1a9ba9697e6f1114bdf18997aa92c
Reviewed-on: http://gerrit.cloudera.org:8080/8080
Reviewed-by: Lars Volker 
Tested-by: Impala Public Jenkins
---
M bin/save-version.sh
1 file changed, 1 insertion(+), 1 deletion(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I2a60fbc5f2c1a9ba9697e6f1114bdf18997aa92c
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 4: Verified-1

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

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3877: support unpatched LLVM

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3877: support unpatched LLVM
..


Patch Set 4: Code-Review+2

carry

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dfbe44ed8a1464b9b0991fd54e72b194ad6155d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5199: prevent hang on empty row batch exchange

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5199: prevent hang on empty row batch exchange
..


Patch Set 3:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1228/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib370ebe44e3bb34d3f0fb9f05aa6386eb91c8645
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5199: prevent hang on empty row batch exchange

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5199: prevent hang on empty row batch exchange
..


Patch Set 3: Code-Review+2

carry

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib370ebe44e3bb34d3f0fb9f05aa6386eb91c8645
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3877: support unpatched LLVM

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-3877: support unpatched LLVM
..


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1227/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dfbe44ed8a1464b9b0991fd54e72b194ad6155d
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8049/1/bin/distcc/distcc_env.sh
File bin/distcc/distcc_env.sh:

Line 119:   if type emulate >/dev/null 2>/dev/null; then emulate -L sh; fi
This logic is also duplicated in clean.sh. How about factoring this function 
out into a bash script and calling it from both places?

It would be nice not to accumulate this cruft.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8056/2/tests/query_test/test_scanners_fuzz.py
File tests/query_test/test_scanners_fuzz.py:

PS2, Line 97: fq_tbl_name = "functional_parquet" + "." + tbl_name
> Do not create a table in the default schemas. Instead, with the changes I d
Another option is to keep the clone code in run_fuzz_test, but change the 
arguments so that it specifies a source database, source table, destination 
database, and destination table. The existing code would simply pass in the 
appropriate existing table. The uncompressed parquet code would create an 
uncompressed parquet table and pass that in.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 6:

(9 comments)

Thank you for working on this. I left some comments inline. Overall I feel that 
we're still not completely decided on which new features of C++11 and new 
paradigms they allow we want to use. I'd be happy to follow up on this on dev@.

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 20: #include 
We are generally trying to reduce our dependencies on Boost libraries. Can you 
replace some of these with C++11 equivalents (e.g. for_each))?


Line 39: constexpr auto i32_max = numeric_limits::max();
We currently use auto only for iterators and const& in loops.


Line 73: TEST(QueryOptions, SetByteOptions) {
Can you add a comment explaining what the test does overall? Same for the other 
tests.


Line 96:   auto process_case_set = [&](auto& case_set) {
I find this part hard to follow and its use of lambdas seems to deviate from 
our current best practices somewhat. Can you try to refactor some parts using 
plain functions to generate context objects instead?


PS6, Line 97: kase
Can you think of a better name?


PS6, Line 99: minus_1_to_0
Why is this needed? Is the added level of indirection worth it?


PS6, Line 137:   // Expands to {"entry", prefix::entry},
 : #define ENTRY(_, prefix, entry) {BOOST_PP_STRINGIZE(entry), 
prefix::entry},
 :   // ENTRIES(prefix, (a)(b)) expands to {"a", prefix::a}, {"b", 
prefix::b},
 : #define ENTRIES(prefix, name) BOOST_PP_SEQ_FOR_EACH(ENTRY, 
prefix, name)
 :   // A case is a pair: (keydef, [(enum_string, enum_value)])
 : #define CASE(key, enumtype, enums) make_pair(key, \
 :   vector\
 :   {ENTRIES(enumtype, BOOST_PP_TUPLE_TO_SEQ(enums))})
I find this very hard to understand. Can you have a look at the Google Test 
documentation on how to define test cases?


Line 171: fusion::for_each(case_set, [, accept_integer](auto& kase) 
{
Can you use C++11 for_each instead?


Line 263: for (auto& key_def : {key_def_min, key_def_default}) {
const auto&?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 10:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1226/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 10: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..


Patch Set 2: Code-Review+2

Sounds like you're doing some additional testing so let me know when you thing 
it's ready to merge.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..


Patch Set 1:

(1 comment)

We have had it this way for a very long time, so I'm kicking off some Jenkins 
jobs to test data load.

http://gerrit.cloudera.org:8080/#/c/8081/1/bin/create-test-configuration.sh
File bin/create-test-configuration.sh:

Line 99:-f 
${HIVE_HOME}/scripts/metastore/upgrade/postgres/hive-schema-1.1.0.postgres.sql
> Very minor comment: it might be simpler to use a relative path here and avo
Fixed, that is definitely cleaner. I tested it on my box and it seems to work.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[Joe McDonnell (Code Review)]

Hello Tim Armstrong,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/8081

to look at the new patch set (#2).

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..

IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

The Hive Metastore schema script includes other SQL
scripts using \i, which expects absolute paths. Since
we currently invoke it from outside the schema script
directory, it is unable to find those included scripts.

The fix is to switch to the Hive Metastore script
directory when invoking the schema script.

Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
---
M bin/create-test-configuration.sh
1 file changed, 5 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3877: support unpatched LLVM

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-3877: support unpatched LLVM
..


Patch Set 3: Code-Review+2

Okay to merge, but please have Michael take a look when he's back.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I3dfbe44ed8a1464b9b0991fd54e72b194ad6155d
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Tianyi Wang (Code Review)]

Tianyi Wang has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 6:

> Is this ready for review?

Yes.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: No

[native-toolchain-CR] Bump LLVM to 3.9.1 and CMake to 3.8.2

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: Bump LLVM to 3.9.1 and CMake to 3.8.2
..


Patch Set 2: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I67e4e3b4e36d7e654adaa9323597902f056a5291
Gerrit-PatchSet: 2
Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..


Patch Set 1: Code-Review+2

(1 comment)

Looks good. Will wait to see if you agree with my suggestion before merging.

http://gerrit.cloudera.org:8080/#/c/8081/1/bin/create-test-configuration.sh
File bin/create-test-configuration.sh:

Line 99:-f 
${HIVE_HOME}/scripts/metastore/upgrade/postgres/hive-schema-1.1.0.postgres.sql
Very minor comment: it might be simpler to use a relative path here and avoid 
repeating the directory name.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

PS2, Line 80: !date_.is_special()
> I looked at ScalarColumnReader::ValidateSlot(), and n
My comment was just that HasDate() == !date_.is_special(), and using HasDate() 
would make it easier to grep all the places we check for this condition.

I'm not sure I follow your comment about public functions.  Since the scanner 
does a cast, it explicitly checks IsValidDate(). It is kinda gross, though.


PS2, Line 80: !date_.is_special()
> I think that ScalarColumnReader::ValidateSlot() is ac
Thanks for finding that. Let's discuss on the jira.


Line 86: }
> About giving a warning to the user: is it ok to log in low level classes li
One purpose of FunctionContext is to be the interface back into impala's 
runtime for UDFs (and expressions in general). It'd be best to leak it outside 
of exprs, udf, and codegen.

So, I think it'd make sense to have a static wrapper around the constructor 
where we do this validation that lives in the exprs code, and that can have 
access to FunctionContext to do the logging, and construct the TimestampValue 
only for well formed values.  (Then this constructor could do a dcheck for 
that).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4850 [DOCS] Create table "comment comes after "partioned by"

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4850 [DOCS] Create table "comment comes after "partioned 
by"
..


Patch Set 2:

> Lars, did you clarify this with Laurel? What to do with this
 > change?

I think I missed this comment as I wasn't added as a reviewer on this change. 
Apologies for the delay.

SORT BY (a) requires it's ordering columns to be in parentheses, just like 
PARTITIONED BY(a).

Laurel, how can I help? Do you want to discuss the change in person?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I543ff1dbfe1ab8a7e0a0a668130ab060e3af0a5f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5525 Extend TestScannersFuzzing to test uncompressed parquet

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5525 Extend TestScannersFuzzing to test uncompressed 
parquet
..


Patch Set 2:

(2 comments)

Take a look at these suggestions and let me know if they make sense.

http://gerrit.cloudera.org:8080/#/c/8056/2/tests/query_test/test_scanners_fuzz.py
File tests/query_test/test_scanners_fuzz.py:

PS2, Line 97: fq_tbl_name = "functional_parquet" + "." + tbl_name
Do not create a table in the default schemas. Instead, with the changes I 
described elsewhere, create a separate clone function that will create an 
uncompressed parquet table directly in unique_database and then pass that into 
run_fuzz_test.

The existing tests will run a simple table clone function that is equivalent to 
the current code.


PS2, Line 129: self.execute_query("create table %s.%s like %s" % 
(unique_database, table, table))
 : fuzz_table_location = 
get_fs_path("/test-warehouse/{0}.db/{1}".format(
 : unique_database, table))
Pull this logic out into its own function e.g. clone_table. run_fuzz_test 
should take in a table that has already been created in unique_database. It 
should not do the clone itself. This allows you to use a different clone 
function to create a parquet table without compression.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I760de7203a51cf82b16016fa8043cadc7c8325bc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change.

Change subject: IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8034/2/be/src/exec/parquet-column-readers.cc
File be/src/exec/parquet-column-readers.cc:

PS2, Line 213: dict_decoder_(parent_->dictionary_pool_.get()),
We should think about having a separate MemPool for parts of the dictionary 
that will not be referenced by the RowBatch and can be freed at the end of the 
row group.

Since some datatypes are copied by value rather than pointing into the 
dictionary, this might be used for those as well.


http://gerrit.cloudera.org:8080/#/c/8034/2/be/src/util/dict-encoding.h
File be/src/util/dict-encoding.h:

PS2, Line 229: memcpy(val_ptr, dict_[index], sizeof(T));
The memcpy is doing a dereference here and is not different from *val_ptr = 
*dict_[index];


PS2, Line 238:   std::vector dict_;
Using a T* means that GetValue() must do a dereference to obtain the value. 
This is a very performance sensitive codepath, so we need to avoid this extra 
dereference. This should maintain the value directly in the vector.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-HasComments: Yes

[Impala-ASF-CR] Increment version to 2.11.0-SNAPSHOT

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: Increment version to 2.11.0-SNAPSHOT
..


Patch Set 1:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1225/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2a60fbc5f2c1a9ba9697e6f1114bdf18997aa92c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 6:

Is this ready for review?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 4:

Build started: https://jenkins.impala.io/job/gerrit-verify-dryrun/1224/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala

[Csaba Ringhofer (Code Review)]

Csaba Ringhofer has posted comments on this change.

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

PS2, Line 80: !date_.is_special()
> I looked at ScalarColumnReader::ValidateSlot(), and n
I think that ScalarColumnReader::ValidateSlot() is 
actually a bit too strict, which lead to inconsistency between text and parquet 
files. I have created a ticket for this:
https://issues.apache.org/jira/browse/IMPALA-5942

We should decide the correct behavior in these cases, before continuing with 
this ticket.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5908: Allow SET to unset modified query options.

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-5908: Allow SET to unset modified query options.
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia8c383e68064f839cb5000118901dff77b4e5cb9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4513: Promote integer types for ABS()

[Zach Amsden (Code Review)]

Zach Amsden has posted comments on this change.

Change subject: IMPALA-4513: Promote integer types for ABS()
..


Patch Set 5:

Sorry, been under C6 crunch.  Will get back to this today.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I86cc880e78258d5f90471bd8af4caeb4305eed77
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Zach Amsden 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

[Joe McDonnell (Code Review)]

Joe McDonnell has uploaded a new change for review.

  http://gerrit.cloudera.org:8080/8081

Change subject: IMPALA-5941: Fix Metastore schema creation in 
create-test-configuration.sh
..

IMPALA-5941: Fix Metastore schema creation in create-test-configuration.sh

The Hive Metastore schema script includes other SQL
scripts using \i, which expects absolute paths. Since
we currently invoke it from outside the schema script
directory, it is unable to find those included scripts.

The fix is to switch to the Hive Metastore script
directory when invoking the schema script.

Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
---
M bin/create-test-configuration.sh
1 file changed, 4 insertions(+), 0 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/81/8081/1
-- 
To view, visit http://gerrit.cloudera.org:8080/8081
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ic312df4597c7d211d4ecd551d572f751aea0cd24
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 

[Impala-ASF-CR] Increment version to 2.11.0-SNAPSHOT

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: Increment version to 2.11.0-SNAPSHOT
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2a60fbc5f2c1a9ba9697e6f1114bdf18997aa92c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 9: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Philip Zeyliger (Code Review)]

Hello Matthew Jacobs,

I'd like you to reexamine a change.  Please visit

http://gerrit.cloudera.org:8080/7829

to look at the new patch set (#9).

Change subject: IMPALA-5211: Simplifying nullif conditional.
..

IMPALA-5211: Simplifying nullif conditional.

This commit:
* Converts nullif(x, y) into if(x IS DISTINCT FROM y, x, NULL).
* Re-writes x IS DINSTINCT FROM y -> FALSE if x.equals(y).
* Removes backend implementation of nullif.

As is the case with all conversions, the original nullif(...) is
replaced with if(...) in error messages, explain plans, and so on.

It's important and subtle that the conversion uses "x IS DISTINCT FROM y"
rather than "x != y" so that the simplification can be made while
handling null values correctly. ("x != x" may be either false or null,
but x is distinct from x is always false.)

Testing:
* Added new tests to ExprRewriteRulesTests for nullif and the if(x
  distinct from y, ...) simplification.
* New test for the rewrite in ParserTest.
* Adds an nvl2() test, incidentally.
* Confirmed (using EclEmma, which uses jococo engine) that coverage is good.
* Ran the tests.

Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
19 files changed, 205 insertions(+), 122 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/29/7829/9
-- 
To view, visit http://gerrit.cloudera.org:8080/7829
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 48:   class CountingRewriteRuleWrapper implements ExprRewriteRule {
> I was thinking along the lines that nobody outside of ExprRewriteRulesTest 
Done


http://gerrit.cloudera.org:8080/#/c/7829/8/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 606: RewritesOk("if(true, nullif(tinyint_col, int_col), null)",
> I don't understand what coverage this adds. The ParserTest already checks t
Ok, removed.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5211: Simplifying nullif conditional.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-5211: Simplifying nullif conditional.
..


Patch Set 8:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7829/7/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 48:   class CountingRewriteRuleWrapper implements ExprRewriteRule {
> I took it out.
I was thinking along the lines that nobody outside of ExprRewriteRulesTest 
needs to create an instance of this, but your argument for static makes sense, 
too. private static then?


http://gerrit.cloudera.org:8080/#/c/7829/8/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

Line 606: RewritesOk("if(true, nullif(tinyint_col, int_col), null)",
I don't understand what coverage this adds. The ParserTest already checks the 
nullif() to if() conversion. It's confusing to me to test conversion in 
ExprRewriteRulesTest because the conversion does not happen through a rewrite 
rule. Also you need to wrap the nullif() in this test with an if() to 
workaround the new counting check you added... Quite hard to comprehend what's 
going on here. Please remove unless you insist this adds coverage.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id91ca968a0c0be44e1ec54ad8602f91a5cb2e0e5
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala

[Csaba Ringhofer (Code Review)]

Csaba Ringhofer has posted comments on this change.

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7954/2/be/src/runtime/timestamp-value.h
File be/src/runtime/timestamp-value.h:

PS2, Line 80: !date_.is_special()
> how about HasDate()?  But also, should IsValidDate() be checking HasDate()?
I looked at ScalarColumnReader::ValidateSlot(), and now I 
see that TimestampValue is created via reinterpret_cast on buffer pointers - 
this means that its constructor is skipped, so it is not possible to force 
validity in the public functions.

>That code looks bogus if date_.is_special(), 
Special values lie outside the valid time range, so IsValidDate can only return 
true if is_special() is false.

TimestampValue can have not_a_date_time in date_ or time_, it is only invalid, 
if both are not_a_date_time. I am not sure, what will/should happen in these 
cases.


Line 86: }
> why is that that the year() calls in timestamp-functions-ir.cc aren't suffi
About giving a warning to the user: is it ok to log in low level classes like 
TimestampValue? AddSub calls FunctionContext::AddWarning, while 
TimestampValue`s functions do not receive FunctionContext arguments.

If TimestampValue functions cannot log, then I will have to look for their 
callers and add checks + warnings there.

Note that I am not familiar with Impala`s logging system yet.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes