[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore in name

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4757: addendum: avoid double underscore in name
..


IMPALA-4757: addendum: avoid double underscore in name

Names containing double underscore are implementation-reserved.
E.g. a global macro called __status could in principle be defined
in a C++ system header.

Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Reviewed-on: http://gerrit.cloudera.org:8080/5702
Reviewed-by: Jim Apple 
Tested-by: Impala Public Jenkins
---
M be/src/testutil/gtest-util.h
1 file changed, 12 insertions(+), 12 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore in name

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4757: addendum: avoid double underscore in name
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..


Patch Set 5: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4752: make ObjectPool more efficient
..


Patch Set 4: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
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: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4752: make ObjectPool more efficient
..


IMPALA-4752: make ObjectPool more efficient

Previously it was implemented as a vector of pointers to
dynamically allocated wrapper objects, where each wrapper object
stored a pointer to the object and a pointer to a vtable, which
had a pointer to a destructor, which then calls the actual
object's destructor.

Instead of doing this, this patch changes ObjectPool to stores
the object pointer and function pointer inline in the vector.
This avoids an unnecessary malloc() and free() pair per object.

Testing:
Ran core tests, which should exercise this heavily during query
execution.

Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Reviewed-on: http://gerrit.cloudera.org:8080/5666
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/common/object-pool.h
1 file changed, 15 insertions(+), 25 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
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: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 7: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-2615: warn if Status is ignored
..


IMPALA-2615: warn if Status is ignored

This introduces a WARN_UNUSED_RESULT macro. It can be used at the end
of function declarations to issue warnings if the return value of the
function is implicitly ignored by the caller. E.g. if a Status or
bool return value indicates the success/failure of a method.

It can also be prepended to function definitions (gcc doesn't allow
it to be put before the { in function definitions).

This can help find bugs. E.g. I found IMPALA-4391 by applying this
to some other places in HdfsScanner.

This commit uses the macro in a few key APIs. We can use it in more
places if we agree on the pattern.

Note:
Gcc has a bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38172
that prevents this from being effective for many functions that
return C++ classes or structs. Clang does not have this issue.

Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Reviewed-on: http://gerrit.cloudera.org:8080/4878
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M be/src/common/status.h
M be/src/exec/exec-node.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/disk-io-mgr-stress.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/test-env.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/openssl-util.h
15 files changed, 112 insertions(+), 96 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3202: implement spill-to-disk in new buffer pool

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool
..

IMPALA-3202: implement spill-to-disk in new buffer pool

See https://goo.gl/0zuy97 for a high-level summary of the design.

Unpinned pages can now be written to disk to free up memory. After
unpinning, pages enter a "dirty" state. Each client initiates
asynchronous writes for dirty page to free up memory to allocate
more buffers. After the write completes, pages are "clean" and can
be evicted from memory by any client that needs the buffer. This
is implemented by moving pages between lists in ClientImpl
(a new internal class that stores the client's state) and BufferPool.

I/O:

The mechanics of I/O to scratch files is handled by the TmpFileMgr
mechanisms introduced in earlier IMPALA-3202 patches.

The decision to start a write I/O is based on two factors. First,
each client maintains the invariant that bytes of the dirty
pages should not exceed the unused reservation. This is to ensure
that any client's reservation can always be fulfilled by evicting
a clean page. Second, additional writes are initiated to proactively
write data to disk, so that clean pages are available when needed.
The buffer pool tries to keep enough writes in flight to keep all
disks busy.

The buffer pool avoids read I/O whenever possible: if an unpinned
page is pinned again and its buffer is still in memory, no I/O
is required to read back the data.

Locking:

Concurrency is managed using client, page, and clean page list locks.
The design uses intrusive doubly-linked lists to track pages. This
patch adds a LockType parameter to InternalQueue and a FakeLock type
to support a non-concurrent InternalList type that is more convenient
for the buffer pool's locking scheme.

Testing:

Added some basic unit tests to BufferPoolTest that exercise the new
code paths. More test coverage will be added later with system tests
and porting of tests from BufferedBlockMgrTest - see the draft
test plan for IMPALA-3200.

Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01
---
M be/src/common/compiler-util.h
A be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/aligned-new.h
A be/src/util/fake-lock.h
M be/src/util/internal-queue.h
10 files changed, 1,021 insertions(+), 299 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT statements

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT 
statements
..


IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT statements

- Generate INSERT statements that are either INSERT ... VALUES or INSERT
  ... SELECT

- On both types of INSERTs, we either insert into all columns, or into
  some column list. If the column list exists, all primary keys will be
  present, and 0 or more additional columns will also be in the list.
  The ordering of the column list is random.

- For INSERT ... SELECT, occasionally generate a WITH clause

- For INSERT ... VALUES, generate non-null constants for the primary
  keys, but for the non-primary keys, randomly generate a value
  expression.

The type system in the random statement/query generator isn't
sophisticated enough to the implicit type of a SELECT item or a value
expression. It knows it will be some INT-based type, but not if it's
going to be a SMALLINT or a BIGINT. To get around this, the easiest
thing seems to be to explicitly cast the SELECT items or value
expressions to the columns' so-called exact_type attribute.

Much of the testing here involved running discrepancy_searcher.py
--explain-only on both tpch_kudu and a random HDFS table, using both the
default profile and DML-only profile. This was done to quickly find bugs
in the statement generation, as they tend to bubble up as analysis
errors. I expect to make other changes as follow on patches and more
random statements find small test issues.

For actual use against Kudu data, you need to migrate data from Kudu
into PostgreSQL 5 (instructions tests/comparison/POSTGRES.txt) and run
something like:

tests/comparison/discrepancy_searcher.py \
  --use-postgresql \
  --postgresql-port 5433 \
  --profile dmlonly \
  --timeout 300 \
  --db-name tpch_kudu \
  --query-count 10

Change-Id: I842b41f0eed07ab30ec76d8fc3cdd5affb525af6
Reviewed-on: http://gerrit.cloudera.org:8080/5486
Reviewed-by: Jim Apple 
Tested-by: Impala Public Jenkins
---
M tests/comparison/common.py
M tests/comparison/db_connection.py
M tests/comparison/funcs.py
M tests/comparison/model_translator.py
M tests/comparison/query_generator.py
M tests/comparison/query_profile.py
M tests/comparison/statement_generator.py
M tests/comparison/tests/query_object_testdata.py
8 files changed, 263 insertions(+), 88 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I842b41f0eed07ab30ec76d8fc3cdd5affb525af6
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT statements

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT 
statements
..


Patch Set 7: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I842b41f0eed07ab30ec76d8fc3cdd5affb525af6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] Clean up tags for 2 query options

[Laurel Hale (Code Review)]

Laurel Hale has posted comments on this change.

Change subject: Clean up  tags for 2 query options
..


Patch Set 1: Code-Review+1

pulled and built the changes in a new branch--no merge conflicts. HTML nav 
titles look good.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2350baffeb216655380d4055bff8bdc09457ec3f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3202: implement spill-to-disk in new buffer pool

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool
..


Patch Set 7:

(15 comments)

http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/runtime/bufferpool/buffer-pool-internal.h
File be/src/runtime/bufferpool/buffer-pool-internal.h:

PS7, Line 38: its state
> I find this terminology a little confusing given that we also talk about a 
Yes they are substates of Unpinned. I reorganised to make it clearer that they 
are substates.


Line 79: boost::lock_guard lock(lock_);
> DCHECK_GT(page->pin_count, 0)?
Done. Moved to buffer-pool.cc since it now needs the non-forward-declaration of 
Page.


PS7, Line 94: UnpinPage
> This name (and "RepinPage") is a bit confusing because it doesn't actually 
The new names are clearer, thanks.


Line 104:   Status EvictBeforeAllocation(ReservationTracker* reservation, 
int64_t allocation_len);
I realised that Evict is a misnomer here. It's really ensuring that enough 
pages get moved to the clean state.


http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/runtime/bufferpool/buffer-pool.cc
File be/src/runtime/bufferpool/buffer-pool.cc:

Line 420:   DCHECK(pinned_pages_.Contains(page));
> DCHECK_EQ(page->pin_count, 0)?
Done


Line 457: // Let WriteCompleteCallback() move it atomically to 
'pinned_pages_'.
> why would the callback move it to pinned list rather than clean list? oh, m
Yeah I didn't love the flag solution, but doing it this way avoid duplicating 
some of the logic in WriteCompleteCallback().


Line 474: // the earlier IsEvicted() check.
> why do we do the earlier check then? to avoid the pool lock?
Exactly. I added a comment to the first check to explain that it's an 
optimisation.


PS7, Line 485: client->impl_->
> isn't that 'this'?
Yes, I missed fixing this while moving code from one class to another.


http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/runtime/bufferpool/buffer-pool.h
File be/src/runtime/bufferpool/buffer-pool.h:

Line 169:   TmpFileMgr::FileGroup* file_group, RuntimeProfile* profile, 
Client* client);
> missing blank line
Done


PS7, Line 170: All pages and buffers for the client
 :   /// must be destroyed
> All pages must be destroyed and buffers must be freed?
Done


Line 242:   /// no locks lower in the lock acquisition order should be held by 
the caller.
> where is the lock ordering documented?
Yeah I thought moving it there would better separate the external/internal 
interfaces, even it's difficult to totally separate them in C++.


PS7, Line 255: between lists is atomic
> how is that, given that the clean page list is global (not client specific)
I meant that there's no way another thread can see the intermediate state where 
the page was removed from a list in the client but not added to 
'clean_pages_list_'. I'm not sure if there's a better way to express this.

If the client lock is released before acquiring clean_pages_lock_ it makes some 
additional races possible (e.g. the cascaded if statements checking which list 
the page is in get more complicated).


Line 400: 
> comment, including the fact that it's only valid to call when pinned.  do w
Added the comment. It seems useful as a convenience function to simplify client 
code (I can see cases where either mem_range() or data()/len() would lead to 
the cleanest client code).


http://gerrit.cloudera.org:8080/#/c/5584/7/be/src/util/internal-queue.h
File be/src/util/internal-queue.h:

PS7, Line 39: InternalQueue
> InternalQueueBase?
I doubt it makes sense to do that. Removed this sentence.


PS7, Line 213: intended to
 :   /// be used for debugging.
> this comment seems outdated now that you're using it for non-debugging.
It's been used for non-debugging for a while I think :).

Fixed it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3202: implement spill-to-disk in new buffer pool

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-3202: implement spill-to-disk in new buffer pool
..

IMPALA-3202: implement spill-to-disk in new buffer pool

See https://goo.gl/0zuy97 for a high-level summary of the design.

Unpinned pages can now be written to disk to free up memory. After
unpinning, pages enter a "dirty" state. Each client initiates
asynchronous writes for dirty page to free up memory to allocate
more buffers. After the write completes, pages are "clean" and can
be evicted from memory by any client that needs the buffer. This
is implemented by moving pages between lists in ClientImpl
(a new internal class that stores the client's state) and BufferPool.

I/O:

The mechanics of I/O to scratch files is handled by the TmpFileMgr
mechanisms introduced in earlier IMPALA-3202 patches.

The decision to start a write I/O is based on two factors. First,
each client maintains the invariant that bytes of the dirty
pages should not exceed the unused reservation. This is to ensure
that any client's reservation can always be fulfilled by evicting
a clean page. Second, additional writes are initiated to proactively
write data to disk, so that clean pages are available when needed.
The buffer pool tries to keep enough writes in flight to keep all
disks busy.

The buffer pool avoids read I/O whenever possible: if an unpinned
page is pinned again and its buffer is still in memory, no I/O
is required to read back the data.

Locking:

Concurrency is managed using client, page, and clean page list locks.
The design uses intrusive doubly-linked lists to track pages. This
patch adds a LockType parameter to InternalQueue and a FakeLock type
to support a non-concurrent InternalList type that is more convenient
for the buffer pool's locking scheme.

Testing:

Added some basic unit tests to BufferPoolTest that exercise the new
code paths. More test coverage will be added later with system tests
and porting of tests from BufferedBlockMgrTest - see the draft
test plan for IMPALA-3200.

Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01
---
M be/src/common/compiler-util.h
A be/src/runtime/bufferpool/buffer-pool-internal.h
M be/src/runtime/bufferpool/buffer-pool-test.cc
M be/src/runtime/bufferpool/buffer-pool.cc
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/bufferpool/suballocator-test.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/aligned-new.h
A be/src/util/fake-lock.h
M be/src/util/internal-queue.h
10 files changed, 1,014 insertions(+), 299 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8c6119a4557da853fefa02e17c49d8be0e9fbe01
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore in name

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4757: addendum: avoid double underscore in name
..


Patch Set 3:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/188/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore in name

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4757: addendum: avoid double underscore in name
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4748: crash in TmpFileMgr when hitting process mem limit

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4748: crash in TmpFileMgr when hitting process mem limit
..


Patch Set 2:

(1 comment)

Hopefully that is a little clearer.

http://gerrit.cloudera.org:8080/#/c/5683/2/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:

Line 282: Status Write(DiskIoMgr* io_mgr, DiskIoRequestContext* io_ctx, 
File* file,
> Maybe briefly note pre/post conditions on write_in_flight_ here and for Ret
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib0a624212bc17f7824e6d14ad143c0d5894206f8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4748: crash in TmpFileMgr when hitting process mem limit

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4748: crash in TmpFileMgr when hitting process mem limit
..

IMPALA-4748: crash in TmpFileMgr when hitting process mem limit

The bug is that FileGroup didn't correctly handle its 'io_ctx_'
being asynchronously cancelled: it left the WriteHandle in an
invalid state. This could happen when the process memory limit
was exceeded.

I fixed this in two ways (either of which would be sufficient
to avoid this exact crash):
* Fix the error handling in TmpFileMgr so that things are left
  in a valid state on the error path.
* Stop DiskIoMgr from asynchronously cancelling I/O contexts
  with no associated MemTracker. The mem_limit check and error
  propagation is necessary when DiskIoMgr will allocate memory
  on behalf of the client, but is not necessary when it is not
  allocating memory for the client - it just added a redundant
  error propagation mechanism.

Testing:
This scenario should no longer be possible for BufferedBlockMgr
since DiskIoMgr won't cancel its I/O context, since it has no
associated MemTracker. However, to test that errors on this path
are correctly handled, I added a simple unit test to TmpFileMgr
that forces cancellation of the I/O context.

Change-Id: Ib0a624212bc17f7824e6d14ad143c0d5894206f8
---
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M be/src/runtime/tmp-file-mgr.h
4 files changed, 57 insertions(+), 15 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib0a624212bc17f7824e6d14ad143c0d5894206f8
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore in name

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4757: addendum: avoid double underscore in name
..

IMPALA-4757: addendum: avoid double underscore in name

Names containing double underscore are implementation-reserved.
E.g. a global macro called __status could in principle be defined
in a C++ system header.

Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
---
M be/src/testutil/gtest-util.h
1 file changed, 12 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore in name

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4757: addendum: avoid double underscore in name
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5702/1//COMMIT_MSG
Commit Message:

PS1, Line 9: starting
> containing:
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore prefix

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4757: addendum: avoid double underscore prefix
..

IMPALA-4757: addendum: avoid double underscore prefix

Names starting with double underscore are implementation-reserved.
E.g. a global macro called __status could in principle be defined
in a C++ system header.

Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
---
M be/src/testutil/gtest-util.h
1 file changed, 12 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-4549: consistently treat 9999 as upper bound for timestamp year

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4549: consistently treat  as upper bound for 
timestamp year
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5665/3/be/src/exprs/expr-test.cc
File be/src/exprs/expr-test.cc:

Line 1483: 
> It looks like this is a duplicate of line 1478. Also, can you put these in 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4549: consistently treat 9999 as upper bound for timestamp year

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4549: consistently treat  as upper bound for 
timestamp year
..

IMPALA-4549: consistently treat  as upper bound for timestamp year

Previously Impala was inconsistent about whether the year 1 was
supported, as a result of inconsistency in boost, which reported the
maximum year as  but sometimes allowed 1. This meant that
Impala sometimes accepted the year 1 and sometimes not.

Use the patched boost version and update tests accordingly.

Testing:
Ran an exhaustive build.

Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
---
M CMakeLists.txt
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-test.cc
M bin/impala-config.sh
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M tests/comparison/discrepancy_searcher.py
6 files changed, 33 insertions(+), 27 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..


Patch Set 5:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/187/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore prefix

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4757: addendum: avoid double underscore prefix
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5702/1//COMMIT_MSG
Commit Message:

PS1, Line 9: starting
containing:

http://eel.is/c++draft/lex.name#3.1


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4643: Centralize tags in separate .ditamap

[Laurel Hale (Code Review)]

Laurel Hale has posted comments on this change.

Change subject: IMPALA-4643: Centralize  tags in separate .ditamap
..


Patch Set 2:

you do have a merge conflict that must be resolved.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d3098356e1b112ba08bfaf7386c3a1f30306223
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4652: Add crcutil to build

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4652: Add crcutil to build
..


Patch Set 3: Code-Review+2 Verified+1

Rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I095d1c6b8e9e8f40cf62c1ecfdc880e708a72c28
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4651: Add LibEv to build

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4651: Add LibEv to build
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf0646533592e6a8cd929a8cb015b83a7ea3008f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4651: Add LibEv to build

[Henry Robinson (Code Review)]

Henry Robinson has submitted this change and it was merged.

Change subject: IMPALA-4651: Add LibEv to build
..


IMPALA-4651: Add LibEv to build

Add libev 4.20 to the Impala build. This is a dependency of KRPC.

FindLibEv.cmake was taken from Apache Kudu.

Change-Id: Iaf0646533592e6a8cd929a8cb015b83a7ea3008f
Reviewed-on: http://gerrit.cloudera.org:8080/5659
Tested-by: Impala Public Jenkins
Reviewed-by: Henry Robinson 
---
M CMakeLists.txt
M bin/bootstrap_toolchain.py
M bin/impala-config.sh
A cmake_modules/FindLibEv.cmake
4 files changed, 51 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: Iaf0646533592e6a8cd929a8cb015b83a7ea3008f
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4747: macros should only evaluate their arguments once

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4747: macros should only evaluate their arguments once
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5686/2/be/src/testutil/gtest-util.h
File be/src/testutil/gtest-util.h:

Line 30: const Status& __status = (status);   \
> "Each identifier that contains a double underscore __ or begins with an und
Posted a follow-up https://gerrit.cloudera.org/#/c/5702/


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1229849150d3f747e6780cc072ba063152ade1a9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4757: addendum: avoid double underscore prefix

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

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

Change subject: IMPALA-4757: addendum: avoid double underscore prefix
..

IMPALA-4757: addendum: avoid double underscore prefix

Names starting with double underscore are implementation-reserved.
E.g. a global macro called __status could in principle be defined
in a C++ system header.

Change-Id: I9b189fe1ec1e70e19b2fd0ed8b183b7569e5fd05
---
M be/src/testutil/gtest-util.h
1 file changed, 6 insertions(+), 6 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4752: make ObjectPool more efficient
..


Patch Set 4:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/186/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
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: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Tim Armstrong (Code Review)]

Hello Lars Volker, Thomas Tauber-Marshall, Dan Hecht,

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

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

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

Change subject: IMPALA-4752: make ObjectPool more efficient
..

IMPALA-4752: make ObjectPool more efficient

Previously it was implemented as a vector of pointers to
dynamically allocated wrapper objects, where each wrapper object
stored a pointer to the object and a pointer to a vtable, which
had a pointer to a destructor, which then calls the actual
object's destructor.

Instead of doing this, this patch changes ObjectPool to stores
the object pointer and function pointer inline in the vector.
This avoids an unnecessary malloc() and free() pair per object.

Testing:
Ran core tests, which should exercise this heavily during query
execution.

Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
---
M be/src/common/object-pool.h
1 file changed, 15 insertions(+), 25 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4752: make ObjectPool more efficient
..


Patch Set 4: Code-Review+2

Carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4752: make ObjectPool more efficient
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5666/3/be/src/common/object-pool.h
File be/src/common/object-pool.h:

PS3, Line 60: //
> '///' ?
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4678: move query MemTracker into QueryState
..


Patch Set 4:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/coordinator.cc
File be/src/runtime/coordinator.cc:

Line 56: #include "runtime/fragment-instance-state.h"
> there's no requirement or expectation that these are alphabetical
clang-format did this for me. The google style guide also says to do it:

https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


Line 466:   
query_state_.Reset(ExecEnv::GetInstance()->query_exec_mgr()->GetOrCreateQueryState(
> there's not need for 'get' semantics here
That's true - from this callsite it always goes down the "Create" code path. We 
need the generality for other fragments though and it didn't seem worth having 
two separate implementations.


Line 485: if (coord_instance_ == nullptr) {
> leave comment that this is a startup failure scenario
Done - didn't mean to remove that.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/coordinator.h
File be/src/runtime/coordinator.h:

Line 43: #include "scheduling/query-schedule.h"
> did you move these?
clang-format sorts includes - I added query-state.h so this happened when I ran 
clang-format on the diff. 

This is consistent with the Google style guide: 
https://google.github.io/styleguide/cppguide.html#Names_and_Order_of_Includes


Line 295:   QueryState::ScopedRef query_state_;
> i'm not sure this is a good idea. we don't want this to be managed by d'tor
Done


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/data-stream-test.cc
File be/src/runtime/data-stream-test.cc:

Line 193:   scoped_ptr runtime_state_;
> why?
We need to call the RuntimeState() constructor after setting up the ExecEnv, 
because it hooks up the RuntimeState's MemTracker to things in the ExecEnv.

Previously this wasn't an issue because InitMemTrackers() was split out from 
the RuntimeState constructor.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/mem-tracker.h
File be/src/runtime/mem-tracker.h:

Line 420:   /// Construct a MemTracker object for query 'id'. The query limits 
are determined based
> this is not a lookup function, make it a static function in MemTracker itse
If I rewrite it as a static function, it would end up being a static function 
with MemTrackerRegistry* as its first argument (because it needs to obtain the 
pool MemTracker from there).  Seemed cleaner just to have it as a member 
function.

This also has the advantage that one class is responsible for setting up the 
top two levels of the hierarchy correctly, rather than having responsibility 
split between two classes.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-exec-mgr.h
File be/src/runtime/query-exec-mgr.h:

Line 63:   QueryState* GetOrCreateQueryState(
> are the 'get' semantics really needed?
We need it to behave in this way, since outside of the coordinator we don't 
know which fragment instance will start first. I could rename it 
CreateQueryState() - it looks like in the codebase we use both naming 
conventions for this kind of function.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.cc
File be/src/runtime/query-state.cc:

Line 69:   query_mem_tracker_->UnregisterFromParent();
> we want to get away from doing anything meaningful in the d'tors.
It's also called TearDown() in Coordinator, to add another option. I called it 
ReleaseResources() to be consistent with RuntimeState.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/query-state.h
File be/src/runtime/query-state.h:

Line 75: /// Releases the previous query state (if it was non-NULL) and 
replaces it with
> is this really needed, rather than just creating a new one?
I ended up removing this, since I removed the use case in the coordinator.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/runtime-state.h
File be/src/runtime/runtime-state.h:

PS4, Line 380: this
> remove
Done


Line 389:   void InitMemTrackers();
> move above comment
This was leftover from an earlier iteration -it's not needed.


http://gerrit.cloudera.org:8080/#/c/5630/4/be/src/runtime/test-env.h
File be/src/runtime/test-env.h:

Line 48:   const TQueryOptions* query_options, QueryState** query_state,
> superfluous; you can get the qs from the runtimestate.
Done


Line 53:   void TearDownStates();
> or simply TearDown()?
I wanted the name to reflect that it didn't tear down TestEnv - just the things 
owned by TestEnv that were created since the last TearDownStates() call.

Renamed to TearDownQueries() - that may be clearer about what it's meant to 
simulate.


Line 83:   std::vector 
fragment_instance_states_;
> these should be owned by the querystates
I changed this to just stick them in the QueryState object pool instead. We 
don't need this vector for anything.


Line 86:   

[Impala-ASF-CR] IMPALA-4678: move query MemTracker into QueryState

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4678: move query MemTracker into QueryState
..

IMPALA-4678: move query MemTracker into QueryState

The query MemTracker for query execution is now owned directly by
QueryState, which greatly simplifies the lifecycle of the MemTracker.
This required various other changes and enabled some simplifications:

* The coordinator's QueryState is constructed earlier before fragments
  are sent out, since we need a MemTracker at that point.
* The global query MemTracker map can be removed.
* The static request pool mem tracker is moved into into ExecEnv.
* Temporary query MemTrackers used to evaluate expressions during
  planning do not need to be registered globally and are owned
  directly by the RuntimeState.
* Various cleanup logic is moved around to reflect the other changes.

Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
---
M be/src/exec/hash-table-test.cc
M be/src/exprs/expr-test.cc
M be/src/runtime/buffered-block-mgr-test.cc
M be/src/runtime/buffered-tuple-stream-test.cc
M be/src/runtime/coordinator.cc
M be/src/runtime/coordinator.h
M be/src/runtime/data-stream-test.cc
M be/src/runtime/exec-env.cc
M be/src/runtime/exec-env.h
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M be/src/runtime/plan-fragment-executor.cc
M be/src/runtime/query-exec-mgr.cc
M be/src/runtime/query-exec-mgr.h
M be/src/runtime/query-state.cc
M be/src/runtime/query-state.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/test-env.cc
M be/src/runtime/test-env.h
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/scheduling/admission-controller.cc
M be/src/service/fe-support.cc
M be/src/service/impala-http-handler.cc
M be/src/service/query-exec-state.cc
25 files changed, 363 insertions(+), 351 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id6b46652932b5638993623e98d1f0d60d8380ba0
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 7: Code-Review+2

Carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 7:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/185/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4643: Centralize tags in separate .ditamap

[Laurel Hale (Code Review)]

Laurel Hale has posted comments on this change.

Change subject: IMPALA-4643: Centralize  tags in separate .ditamap
..


Patch Set 2: Code-Review+1

I pulled the files into my own branch and built them with the dita ot 2.3.3 
into a pdf and it looks good to me.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4d3098356e1b112ba08bfaf7386c3a1f30306223
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5631/4/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:

Line 133:   BinaryPredicate.Operator.EQ, encoded.clone(), 
candidate));
> another one that was missed.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Thomas Tauber-Marshall (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..

IMPALA-4716: Expr rewrite causes IllegalStateException

The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.

The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.

Testing:
- Added a regression test to exprs.test

Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
3 files changed, 35 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Anonymous Coward (Code Review)]

zams...@cloudera.com has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 6:

(2 comments)

I realize this is already merged - simply learning how to use gerrit and I 
actually did have two comments.

http://gerrit.cloudera.org:8080/#/c/5662/6/fe/src/main/java/org/apache/impala/analysis/Analyzer.java
File fe/src/main/java/org/apache/impala/analysis/Analyzer.java:

PS6, Line 1065: ;
Nit: spurious semi-colon


http://gerrit.cloudera.org:8080/#/c/5662/6/testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test:

PS6, Line 67: sum(2 + id) > 1, sum(2 + id) >= 5
Looks like further opportunities for redundant subexpression elimination 
remain, although this one is a bit more difficult.  Is there already a JIRA to 
track this?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: zams...@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT statements

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT 
statements
..


Patch Set 7:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/184/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I842b41f0eed07ab30ec76d8fc3cdd5affb525af6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4651: Add LibEv to build

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4651: Add LibEv to build
..


Patch Set 3: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf0646533592e6a8cd929a8cb015b83a7ea3008f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT statements

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT 
statements
..


Patch Set 7: Code-Review+2

carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I842b41f0eed07ab30ec76d8fc3cdd5affb525af6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT statements

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT 
statements
..


Patch Set 7:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/183/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I842b41f0eed07ab30ec76d8fc3cdd5affb525af6
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4752: make ObjectPool more efficient
..


Patch Set 3: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5631/3/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:

Line 133:   children_.add(encodedIsNull.clone());
> explain somewhere why this is needed
Done


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

Line 234
> this needs to be a test case in exprs.test (ie, a functional test), you don
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Thomas Tauber-Marshall (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..

IMPALA-4716: Expr rewrite causes IllegalStateException

The DECODE constructor in CaseExpr uses the same decodeExpr object when
building the BinaryPredicates that compare the decodeExpr to each 'when'
of the DECODE. This causes problems when different BinaryPredicates try
to cast the same decodeExpr object to different types during analysis,
in this case leading to a Precondition check failure.

The solution is to clone the decodeExpr in the DECODE constructor in
CaseExpr for each generated BinaryPredicate.

Testing:
- Added a regression test to exprs.test

Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
---
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
2 files changed, 24 insertions(+), 4 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 7:

> Turns out there's a gcc bug that prevents this from working
 > sometimes (including for DiskIoMgr::Init()). I updated the commit
 > message to include this info.
 > 
 > Shall we still go ahead even with this caveat?
 > 
 > I think this is still worthwhile since clang will detect the
 > problems/

I think we should go ahead.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4036: invalid SQL generated for partitioned table with comment

[Henry Robinson (Code Review)]

Henry Robinson has submitted this change and it was merged.

Change subject: IMPALA-4036: invalid SQL generated for partitioned table with 
comment
..


IMPALA-4036: invalid SQL generated for partitioned table with comment

For a table that has both a table comment and a partition specified,
"show create table" incorrectly outputs the comment before the partition.
This is not the correct order, and it results in an invalid SQL.

This transaction fixes the ordering (partition comes before comment) and
adds tests for this case.

Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
Reviewed-on: http://gerrit.cloudera.org:8080/5648
Tested-by: Impala Public Jenkins
Reviewed-by: Henry Robinson 
---
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/show-create-table.test
3 files changed, 8 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-4036: invalid SQL generated for partitioned table with comment

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4036: invalid SQL generated for partitioned table with 
comment
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-1861: Simplify conditionals with constant conditions

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-1861: Simplify conditionals with constant conditions
..


Patch Set 8:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5585/7/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 154: // Contains all 'when' clauses with non-constant conditions, used 
to construct the new
> construct
Done


Line 157: // Set to THEN of first constant TRUE clause, if any.
> comment: first constant true when clause
Done


Line 188: } else {
> this can be misread to apply to the following else if
Done


Line 189:   // This WHEN is always FALSE, so it can be removed.
> can whenExpr be a null literal, given l160?
You're right - I was thinking of the hasCaseExpr case where whenExpr comes from 
the BinaryPredicate we construct and constant fold above, but I don't guess 
there's any expression of the form "non-null literal = non-null literal" that 
would constant fold down to NULL.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1861: Simplify conditionals with constant conditions

[Thomas Tauber-Marshall (Code Review)]

Hello Marcel Kornacker,

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

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

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

Change subject: IMPALA-1861: Simplify conditionals with constant conditions
..

IMPALA-1861: Simplify conditionals with constant conditions

When there are conditionals with constant values of TRUE or
FALSE we can simplify them during analysis using the ExprRewriter.

This patch introduces the SimplifyConditionalsRule with covers IF,
OR, AND, CASE, and DECODE.

It also introduces NormalizeExprsRule which normalizes AND and OR
such that if either child is a BoolLiteral, then the left child is a
BoolLiteral.

Testing:
- Added unit tests to ExprRewriteRulesTest.
- Ran FE planner tests and BE expr-test.

Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CaseWhenClause.java
A fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
6 files changed, 383 insertions(+), 13 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4216: Test became flaky: TestTpchMemLimitError.test low mem limit q20

[Attila Jeges (Code Review)]

Attila Jeges has abandoned this change.

Change subject: IMPALA-4216: Test became flaky: 
TestTpchMemLimitError.test_low_mem_limit_q20
..


Abandoned

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: I471a90c8e3bdecb4fed08fbae3436c50b8618471
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Attila Jeges 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4748: crash in TmpFileMgr when hitting process mem limit

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4748: crash in TmpFileMgr when hitting process mem limit
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5683/2/be/src/runtime/tmp-file-mgr.h
File be/src/runtime/tmp-file-mgr.h:

Line 282: Status Write(DiskIoMgr* io_mgr, DiskIoRequestContext* io_ctx, 
File* file,
Maybe briefly note pre/post conditions on write_in_flight_ here and for 
RetryWrite below.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ib0a624212bc17f7824e6d14ad143c0d5894206f8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


IMPALA-4722: Disable log caching in test_scratch_disk

test_scratch_disk fails sporadically when trying to assert the presence
of log messages. This is probably caused by log caching, since after
such failures the log files do contains the lines in question.

I manually tested this by running the tests repeatedly for 2 days (10k
runs).

To make future diagnosis of similar problems easier, this change also
adds more output to assert_impalad_log_contains().

Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Reviewed-on: http://gerrit.cloudera.org:8080/5669
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M tests/common/custom_cluster_test_suite.py
M tests/custom_cluster/test_scratch_disk.py
2 files changed, 15 insertions(+), 9 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 7:

Turns out there's a gcc bug that prevents this from working sometimes 
(including for DiskIoMgr::Init()). I updated the commit message to include this 
info.

Shall we still go ahead even with this caveat?

I think this is still worthwhile since clang will detect the problems/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4651: Add LibEv to build

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4651: Add LibEv to build
..


Patch Set 3:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/182/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf0646533592e6a8cd929a8cb015b83a7ea3008f
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins, Jim Apple, Dan Hecht,

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

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

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

Change subject: IMPALA-2615: warn if Status is ignored
..

IMPALA-2615: warn if Status is ignored

This introduces a WARN_UNUSED_RESULT macro. It can be used at the end
of function declarations to issue warnings if the return value of the
function is implicitly ignored by the caller. E.g. if a Status or
bool return value indicates the success/failure of a method.

It can also be prepended to function definitions (gcc doesn't allow
it to be put before the { in function definitions).

This can help find bugs. E.g. I found IMPALA-4391 by applying this
to some other places in HdfsScanner.

This commit uses the macro in a few key APIs. We can use it in more
places if we agree on the pattern.

Note:
Gcc has a bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=38172
that prevents this from being effective for many functions that
return C++ classes or structs. Clang does not have this issue.

Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
---
M be/src/common/status.h
M be/src/exec/exec-node.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/disk-io-mgr-stress.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/test-env.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/openssl-util.h
15 files changed, 112 insertions(+), 96 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4752: make ObjectPool more efficient

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4752: make ObjectPool more efficient
..


Patch Set 3: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5666/3/be/src/common/object-pool.h
File be/src/common/object-pool.h:

PS3, Line 60: //
'///' ?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1e6a40ac798fa64da4767f530c48710cba623ea5
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4878/6/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

Line 791: ASSERT_OK(io_mgr.Init(_tracker));
> Good question. I'll have to figure that out - I won't resolve the JIRA unti
okay, thanks.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4878/6/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

Line 791: ASSERT_OK(io_mgr.Init(_tracker));
> why was this only caught by clang-tidy and not by the new warn unused resul
Good question. I'll have to figure that out - I won't resolve the JIRA until 
we've figured that out (and added the annotation in a few more places, 
probably).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4355: random query generator: modify statement execution flow to support DML

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4355: random query generator: modify statement execution 
flow to support DML
..


Patch Set 6:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/181/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4c63a2223185d0e056cc5713796772e5d1b8414
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4355: random query generator: modify statement execution flow to support DML

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4355: random query generator: modify statement execution 
flow to support DML
..


Patch Set 6: Code-Review+2

carry taras's +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ia4c63a2223185d0e056cc5713796772e5d1b8414
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4878/6/be/src/runtime/disk-io-mgr-test.cc
File be/src/runtime/disk-io-mgr-test.cc:

Line 791: ASSERT_OK(io_mgr.Init(_tracker));
why was this only caught by clang-tidy and not by the new warn unused result?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 6:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/180/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 6: Code-Review+2

Missed one clang-tidy warning. Really need to figure out how to run it locally.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins, Jim Apple, Dan Hecht,

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

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

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

Change subject: IMPALA-2615: warn if Status is ignored
..

IMPALA-2615: warn if Status is ignored

This introduces a WARN_UNUSED_RESULT macro. It can be used at the end
of function declarations to issue warnings if the return value of the
function is implicitly ignored by the caller. E.g. if a Status or
bool return value indicates the success/failure of a method.

It can also be prepended to function definitions (gcc doesn't allow
it to be put before the { in function definitions).

This can help find bugs. E.g. I found IMPALA-4391 by applying this
to some other places in HdfsScanner.

This commit uses the macro in a few key APIs. We can use it in more
places if we agree on the pattern.

Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
---
M be/src/common/status.h
M be/src/exec/exec-node.h
M be/src/exec/hdfs-scan-node-base.cc
M be/src/runtime/bufferpool/buffer-allocator.h
M be/src/runtime/bufferpool/buffer-pool.h
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/disk-io-mgr-stress.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/disk-io-mgr.h
M be/src/runtime/mem-tracker.h
M be/src/runtime/test-env.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.h
M be/src/util/openssl-util.h
15 files changed, 112 insertions(+), 96 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I8c26e7532b5f2c7fe167accc73179e8b72b192bc
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4747: macros should only evaluate their arguments once

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4747: macros should only evaluate their arguments once
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5686/2/be/src/testutil/gtest-util.h
File be/src/testutil/gtest-util.h:

Line 30: const Status& __status = (status);   \
"Each identifier that contains a double underscore __ or begins with an 
underscore followed by an uppercase letter is reserved to the implementation 
for any use."


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I1229849150d3f747e6780cc072ba063152ade1a9
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Release note updates for Impala 2.8

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Release note updates for Impala 2.8
..


Patch Set 3:

Is this a duplicate of https://gerrit.cloudera.org/#/c/5668/?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I03144b423c4d698e87dd335914a8b7c0ff030496
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4716: Expr rewrite causes IllegalStateException

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4716: Expr rewrite causes IllegalStateException
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5631/3/fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
File fe/src/main/java/org/apache/impala/analysis/CaseExpr.java:

Line 133:   BinaryPredicate.Operator.EQ, encoded.clone(), 
candidate));
explain somewhere why this is needed


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

Line 234: // correctly when FoldConstantRule is used.
this needs to be a test case in exprs.test (ie, a functional test), you don't 
want to test just for a specific rule. (what if some other rule gets added 
later that reintroduces the bug?)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I4de9ed7118c8d18ec3f02ff74c9cca211c716e51
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/179/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2522: Add doc for sortby() and clustered hints

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-2522: Add doc for sortby() and clustered hints
..


Patch Set 4:

(10 comments)

Thanks for the change. I added comments inline.

http://gerrit.cloudera.org:8080/#/c/5655/4/docs/shared/impala_common.xml
File docs/shared/impala_common.xml:

PS4, Line 2639: minimizes
Technically this is incorrect now, since CLUSTERED reduces the number of 
concurrent files even more. Maybe rephrase to "reduces".


PS4, Line 2672: perform organize
This reads as if one word was superfluous


PS4, Line 2674: risking
to reduce the risk of an...


PS4, Line 2686: large insert operations
  : that use dynamic partitioning
This surprises me. Maybe I'm missing something?


Line 3707: 
?


http://gerrit.cloudera.org:8080/#/c/5655/4/docs/topics/impala_hints.xml
File docs/topics/impala_hints.xml:

Line 87:   either the /* */ or -- 
notation.
We might want to state that the use of [] is discourage and will be deprecated 
in the next compatibility-breaking release of Impala.


PS4, Line 91: immediately before the hint name
It must only be present immediately before the hint list, i.e. the first hint 
name. For example /* +clustered,shuffle,sortby(a,b) */


Line 129: 
we should have one example with multiple hints.


http://gerrit.cloudera.org:8080/#/c/5655/4/docs/topics/impala_insert.xml
File docs/topics/impala_insert.xml:

Line 67: hint_with_dashes ::= -- +SHUFFLE | -- +NOSHUFFLE | -- +SORTBY(col, col 
...)]
Add CLUSTERED to the list?


Line 72:   (Note: with this hint format, the square brackets are part of the 
syntax.)
Maybe mention that this will deprecated in the next compatibility-breaking 
release here, too?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id3c1da9a87ace361b096fa73d8504b2f54e75bed
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4651: Add LibEv to build

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4651: Add LibEv to build
..


Patch Set 2:

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/178/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf0646533592e6a8cd929a8cb015b83a7ea3008f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4652: Add crcutil to build

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4652: Add crcutil to build
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I095d1c6b8e9e8f40cf62c1ecfdc880e708a72c28
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No