[Impala-ASF-CR] IMPALA-4000: Restricted Sentry authorization for Kudu Tables

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables
..


Patch Set 7: Code-Review+2

Fixed the small issue with how we check if security is enabled. Carrying the +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4000: Restricted Sentry authorization for Kudu Tables

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#7).

Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables
..

IMPALA-4000: Restricted Sentry authorization for Kudu Tables

At this time, there is no comprehensive way of enforcing a Sentry
authorization policy against tables stored in Kudu. The following
behavior was implemented in this patch:
- Only the ALL privilege level can be granted to Kudu tables.
  Finer-grained levels such as only SELECT or only INSERT are not
  supported.
- Column level permissions on Kudu tables are not supported.
- Only users with ALL privileges on SERVER may create external Kudu
  tables.

Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
6 files changed, 58 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4000: Restricted Sentry authorization for Kudu Tables

[Taras Bobrovytsky (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables
..

IMPALA-4000: Restricted Sentry authorization for Kudu Tables

At this time, there is no comprehensive way of enforcing a Sentry
authorization policy against tables stored in Kudu. The following
behavior was implemented in this patch:
- Only the ALL privilege level can be granted to Kudu tables.
  Finer-grained levels such as only SELECT or only INSERT are not
  supported.
- Column level permissions on Kudu tables are not supported.
- Only users with ALL privileges on SERVER may create external Kudu
  tables.

Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
6 files changed, 58 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4000: Restricted Sentry authorization for Kudu Tables

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables
..


Patch Set 7: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..


Patch Set 4:

(6 comments)

Responding to comments. I'll wait for some of the larger changes before doing 
another round.

http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

Line 351:   Map> newFileDescMap = 
Maps.newHashMap();
> I did that initially but the problem happens when one of the files is delet
Before, we used to want to reuse as much of the existing block metadata as 
possible since that was an expensive operation. However, with this new design, 
we are already loading all the metadata from HDFS, so there is no need to 
optimize this case by not reloading the metadata (we are loading it anyway). So 
I think this can be simplified to just discarding the entries from the global 
map and updating the global map in place. We also don't need any of the "should 
we reload the metadata" logic.


Line 386: String[] blockHostPorts = loc.getNames();
> We are using them in preconditions check in L396 and loop condition in L403
The code and comments around these is very sparse on content, so I think we 
should just condense it. This method looks really long but really it is just 
very "fluffy", so I was hoping to make it more content dense.


Line 431:   numHdfsFiles_ += fileDescs.size();
> I think its due to the current design. The reload is implemented as dropPar
Got it, makes sense.


Line 483: for (HdfsPartition partition: partitions) {
> Although it looks like O(N^2), it isn't. 'partitions' correspond to each pa
Ahh, of course. You are right. I missed the local partitions var.


Line 934: boolean isMarkedCached = isMarkedCached_;
> Yep, wanted to discuss about this. I'll ping you offline.
Looks like we used to force reloading the block locations of cached partitions. 
As discussed in a comment above, the optimization of only reloading the 
metadata for some files doesn't seem to make sense anymore, so forcing the 
reload based on caching doesn't make anymore, either.


http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
File fe/src/main/java/org/apache/impala/common/FileSystemUtil.java:

Line 423: if (p.isRoot()) return false;
> That returns true for isChildPath(root, root)?
Good point. You are right.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#7).

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..

IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.

Without this patch:
--
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.

With this patch:
---
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.

- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.

- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.

Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.

Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
4 files changed, 393 insertions(+), 260 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Mostafa Mokhtar 

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change.

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..


Patch Set 6:

(29 comments)

http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
File fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java:

Line 37: 
> why not:
Done


Line 50: // the storage ID of a particular disk is unique across all the 
nodes in the cluster.
> make members and methods non-static since we are using a singleton
Done


Line 63:  * the initial metadata load. Figure out ways to fix it using 
finer locking scheme.
> storageUuid
Done


Line 72:   // amount of data loading is done and storageIdtoInt is 
populated with storage IDs
> I did some more reading and I'm afraid this "double checked locking" scheme
Done


http://gerrit.cloudera.org:8080/#/c/5148/4/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

Line 120: import java.io.IOException;
> I think these might need cleanup.
My editor is messing up these imports. Let me try fixing them by hand.


Line 351:   Map> newFileDescMap = 
Maps.newHashMap();
> What's the benefit of populating this temporary map and then merging it int
I did that initially but the problem happens when one of the files is deleted. 
For example, consider the following sequence of actions.

- create partition p1 with file f1
- perPartitionFileDescMap_ contains f1
- insert overwrite p1 select * from random_table, say this overwrites f1 and 
creates f2.
- In such a case, we loop p1, and add f2 to perPartitionFileDescMap_ but we 
still have f1 at the end of it.
- So we need to scan all entries in perPartitionFileDescMap_ and figure out the 
difference with listStatus() and remove them. This is a costly operation.
- Easier way is to build a newFdMap with the current snapshot of listStatus() 
and replace it in perPartitionFileDescMap_

Also this way I felt the code is more readable. This is arguable, but I felt so.


Line 382:   // Loop over all blocks in the file.
> comment doesn't add anything, remove
Done


Line 384: Preconditions.checkNotNull(loc);
> Not your change, but this looks weird, why would it be null? Remove?
Done


Line 386: String[] blockHostPorts = loc.getNames();
> Let's just use loc.getNames() and loc.getHosts() inside the loop in L403. I
We are using them in preconditions check in L396 and loop condition in L403 and 
L405,L408 for index based access. Let me know if you still think we should 
remove it.


Line 395: 
Sets.newHashSet(Arrays.asList(loc.getCachedHosts()));
> I think we can get rid of the asList() here
nice catch. done


Line 399: // to hostMap_/hostList_. The host ID (index in to the 
hostList_) for each
> update comment, "hostMap_" doesn't seem to exist
Remnants of old clean-up. This logic is now merged into hostIndex_. Updated 
comment accordingly.


Line 413:   // Remember the THdfsFileBlocks and corresponding 
BlockLocations.  Once all the
> is this comment still accurate?
Yes, we basically add all these blocks to perFsFileBlocks and run getDiskId() 
on it in bulk.


Line 431:   numHdfsFiles_ += fileDescs.size();
> Why do the number of files and bytes always keep increasing?
I think its due to the current design. The reload is implemented as 
dropPartition() + creatPartition(). Also we resetPartitions() at few places to 
make it 0. Basically we never delete entries from it, we just overwrite them 
with new values.


Line 458: fileStatus.getModificationTime());
> indent 4 (and same issue elsewhere)
Fixed my editor settings now. Done.


Line 468: // All the partitions corresponding to the same partition 
path, have the same
> I don't think this is necessarily true, but you can state that we assume it
Ok, modified the comment.


Line 483: for (HdfsPartition partition: partitions) {
> This function looks much more expensive than before. We loop over all parti
Although it looks like O(N^2), it isn't. 'partitions' correspond to each 
partition that maps to a single location. So if we flatten 
List its actually all the partitions in the table. Basically 
we don't loop like a conventional nested loop "for each partition, for each 
fileStatus". We loop through the pruned partition list i.e, partitions 
corresponding to that path.


Line 780: Map blocksToLoad = Maps.newHashMap();
> unused?
Done


Line 784: HashMap filteredParts = 
Maps.newHashMap();
> The var name filteredParts is a little confusing to me. How about partsByPa
Done


Line 830: if (filteredParts.containsKey(partDir)) 
filteredParts.get(partDir).add(partition);
> use {}
Done


Line 833: // Check if the partition 

[Impala-ASF-CR] IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4512: Add a script that builds Impala on stock Ubuntu 
14.04.
..


Patch Set 4: Verified-1

Build failed: http://35.164.73.121:8080/job/gerrit-verify-dryrun/42/

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

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

[Impala-ASF-CR] IMPALA-4000: Restricted Sentry authorization for Kudu Tables

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables
..


Patch Set 7:

(1 comment)

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

Line 2485: 
> Done
Actually it turns out that don't need this method any more.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4000: Restricted Sentry authorization for Kudu Tables

[Taras Bobrovytsky (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables
..

IMPALA-4000: Restricted Sentry authorization for Kudu Tables

At this time, there is no comprehensive way of enforcing a Sentry
authorization policy against tables stored in Kudu. The following
behavior was implemented in this patch:
- Only the ALL privilege level can be granted to Kudu tables.
  Finer-grained levels such as only SELECT or only INSERT are not
  supported.
- Column level permissions on Kudu tables are not supported.
- Only users with ALL privileges on SERVER may create external Kudu
  tables.

Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
6 files changed, 59 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4000: Restricted Sentry authorization for Kudu Tables

[Taras Bobrovytsky (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-4000: Restricted Sentry authorization for Kudu Tables
..

IMPALA-4000: Restricted Sentry authorization for Kudu Tables

At this time, there is no comprehensive way of enforcing a Sentry
authorization policy against tables stored in Kudu. The following
behavior was implemented in this patch:
- Only the ALL privilege level can be granted to Kudu tables.
  Finer-grained levels such as only SELECT or only INSERT are not
  supported.
- Column level permissions on Kudu tables are not supported.
- Only users with ALL privileges on SERVER may create external Kudu
  tables.

Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/authorization/PrivilegeRequestBuilder.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeAuthStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
6 files changed, 59 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I183f08ad8ce80deee011a6b90ad67b9cefc0452c
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] Add a build flag for the undefined behavior sanitizer, aka "ubsan".

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: Add a build flag for the undefined behavior sanitizer, aka 
"ubsan".
..


Patch Set 2:

Are any of these overflows legitimate use of signed overflows?  Assuming not, 
shouldn't we just fix the code that can lead to signed overflows? Otherwise, 
while the behavior would become defined, it's still not correct.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88c7234bd7c5eb7404490a0913d90470c10835e7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

Once the Promise (of Status) is set in ProcessBuildInputAsync(),
the main thread in ProcessBuildInputAndOpenProbe() may proceed
to finish up the query and free RuntimeState. So, it's unsafe
to access the RuntimeState afterwards. Commit bb1c633 broke that
assumption (which is arguably fragile). This change fixes the
problem by adding a scope for the that counter to avoid the
use-after-free problem.

Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Reviewed-on: http://gerrit.cloudera.org:8080/5246
Reviewed-by: Dan Hecht 
Reviewed-by: Tim Armstrong 
Tested-by: Internal Jenkins
---
M be/src/exec/blocking-join-node.cc
1 file changed, 15 insertions(+), 11 deletions(-)

Approvals:
  Internal Jenkins: Verified
  Tim Armstrong: Looks good to me, but someone else must approve
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


Patch Set 2: Code-Review+2

Carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
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: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has submitted this change and it was merged.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


IMPALA-4519: increase timeout in TestFragmentLifecycle

Increase the timeout to over 120s to match datastream_sender_timeout_ms.
This should avoid spurious test failures if we are unlucky and a sender
gets stuck waiting for a receiver fragment that will never start.

Testing:
Ran the test in a loop for a while to flush out any flakiness.

Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
Reviewed-on: http://gerrit.cloudera.org:8080/5244
Tested-by: Impala Public Jenkins
Reviewed-by: Tim Armstrong 
---
M tests/query_test/test_lifecycle.py
1 file changed, 4 insertions(+), 3 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
Gerrit-PatchSet: 3
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: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4397,IMPALA-3259: reduce codegen time and memory

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4397,IMPALA-3259: reduce codegen time and memory
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4956/14/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

PS14, Line 475: avoid inlining avoid inlining for the exprs exceeding this 
threshold.
> Just noticed this change of behavior since PS7.
Right, this was done in response to one of Marcel's comments. The idea is that 
this way there is less change of a performance cliff, when crossing the 
threshold the first 100 expressions will still be inlined.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id10015b49da182cb181a653ac8464b4a18b71091
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Silvius Rus 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4397,IMPALA-3259: reduce codegen time and memory

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4397,IMPALA-3259: reduce codegen time and memory
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/4956/14/be/src/codegen/llvm-codegen.h
File be/src/codegen/llvm-codegen.h:

PS14, Line 475: avoid inlining avoid inlining for the exprs exceeding this 
threshold.
Just noticed this change of behavior since PS7.

This is not quite precise, right ? In fact, this is the maximum number of 
expression evaluation which will be inlined into a given function (e.g. 
EvalConjuncts). Any expression evaluations after the first 
CODEGEN_INLINE_EXPRS_THRESHOLD expressions will not be inlined but the first 
CODEGEN_INLINE_EXPRS_THRESHOLD may still be inlined, right ?

This behavior seems a bit odd to me. It seems like the behavior of PS7 is more 
deterministic. With this approach, it's really unclear which expressions get 
inlined.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id10015b49da182cb181a653ac8464b4a18b71091
Gerrit-PatchSet: 14
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Silvius Rus 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
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: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Add a build flag for the undefined behavior sanitizer, aka "ubsan".

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Add a build flag for the undefined behavior sanitizer, aka 
"ubsan".
..


Patch Set 2:

> (1 comment)

Yeah, it's counterintuitive.

Given that new information, if I can remove any overflows from our testsuite 
using UBSAN and show -fwrapv does not hurt our benchmarks and get codegenned 
code to use -fwrap just like non-codegenned code can with just this flag, wold 
you be OK with using -fwrapv?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88c7234bd7c5eb7404490a0913d90470c10835e7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4498: crash in to utc timestamp/from utc timestamp

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4498: crash in to_utc_timestamp/from_utc_timestamp
..

IMPALA-4498: crash in to_utc_timestamp/from_utc_timestamp

The bugs was that the functions did not check whether the conversion
pushed the value out of range. The fix is to use boost's validation
immediately to check the validity of the timestamp and catch any
exceptions thrown.

It would be preferable to avoid the exceptions, but Boost does not
provide a straightforward way to disable the exceptions or extract
potentially-invalid values from a date object.

Testing:
Added expression tests that exercise out-of-range cases. Also
added additional tests to confirm that date addition and subtraction
weren't affected by similar bugs.

Change-Id: Idc427b06ac33ec874a05cb98d01c00e970d3dde6
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
M be/src/runtime/timestamp-value.h
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 139 insertions(+), 22 deletions(-)


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

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

[Impala-ASF-CR] Add a build flag for the undefined behavior sanitizer, aka "ubsan".

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Add a build flag for the undefined behavior sanitizer, aka 
"ubsan".
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5082/1/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 40: #  -fwrapv: force signed integer arithmetic operations to wrap
> Oh, I see. There's a funny thing here -fwrapv does not stop ubsan from catc
Oh I didn't realise that., well I guess that's a good thing for us.


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

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

[Impala-ASF-CR] Avoid std::function when possible.

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

> > I do not find it more readable, but I find it to be readable
 > enough; I think the readability delta is a good price to pay to
 > avoid boxing up a lambda.
 > 
 > Could you be more specific about the drawbacks? We've spoken about
 > heap allocation; personally I don't think that's an issue because
 > it's not unusual, but perhaps that's a dealbreaker for you.
 > Otherwise I know you consider it 'overkill', but I don't yet know
 > why. Perhaps there's something about this class in particular that
 > makes it a bad choice for std::function?

Heap allocation is actually an issue, because it doesn't go with how we 
(eventually) want to do memory management and reservations. I understand that 
we do this elsewhere, by virtue of using STL, but we shouldn't add to it.

Throwing in the c'tor is also not okay.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4542: Fix use-after-free in some BE tests

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4542: Fix use-after-free in some BE tests
..


Patch Set 1: Verified-1

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/562/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5302d665756183289edf227588da1c094e197584
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Avoid std::function when possible.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

> > I do not find it more readable, but I find it to be readable
 > enough; I think the readability delta is a good price to pay to
 > avoid boxing up a lambda.
 > 
 > Could you be more specific about the drawbacks? We've spoken about
 > heap allocation; personally I don't think that's an issue because
 > it's not unusual, but perhaps that's a dealbreaker for you.
 > Otherwise I know you consider it 'overkill', but I don't yet know
 > why. Perhaps there's something about this class in particular that
 > makes it a bad choice for std::function?

It is another abstraction layer between the caller and the function they passed 
in. With the code as it is now, they may have to understand the subtleties of 
no only anonymous functions but also std::function, a task that, as the slides 
and talk demonstrate, was too difficult in the corner cases for even the 
standards committee and will have to get fixed up in C++17.

I don't know of a corner case yet that is going to bite us, but I'd rather not 
find one the hard way.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

[Mostafa Mokhtar (Code Review)]

Mostafa Mokhtar has posted comments on this change.

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5148/6/fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
File fe/src/main/java/org/apache/impala/catalog/HdfsTable.java:

Line 347:   LOG.debug("Loading block md for " + name_ + " directory " + 
dirPath.toString());
Please add If(LOG.isDebugEnabled()) to avoid calling the dirPath.toString()  as 
this causes lots of unnecessary allocations which causes GC pauses .


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Avoid std::function when possible.

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

> I do not find it more readable, but I find it to be readable enough; I think 
> the readability delta is a good price to pay to avoid boxing up a lambda.

Could you be more specific about the drawbacks? We've spoken about heap 
allocation; personally I don't think that's an issue because it's not unusual, 
but perhaps that's a dealbreaker for you. Otherwise I know you consider it 
'overkill', but I don't yet know why. Perhaps there's something about this 
class in particular that makes it a bad choice for std::function?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] Avoid std::function when possible.

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

> I don't think we should never use it, but I think it is overkill for this use 
> case

The alternative implementation is more complex, in terms of lines of code and 
also wrt the need for a specialised construction pattern. 

Here, std::function serves the role of simplifying the implementation and the 
interface (what do I pass in? Oh, it's a function). Do you find the templated 
version more readable?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] Avoid std::function when possible.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

> > It's the added hidden run-time complexity generally
 > 
 > Can you be more specific? Abstractions naturally hide complexity;
 > what are the gotchas that you think mean we shouldn't use
 > std::function?

I don't think we should never use it, but I think it is overkill for this use 
case, just like std::map is sometimes overkill for char[].

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has uploaded a new patch set (#5).

Change subject: IMPALA-4172/IMPALA-3653: Improvements to block metadata loading
..

IMPALA-4172/IMPALA-3653: Improvements to block metadata loading

This patch improves the block metadata loading (locations and disk
storage IDs) for partitioned and un-partitioned tables in the Catalog
server.

Without this patch:
--
We loop throuh each and every file in the table/partition directories
and call getFileBlockLocations() on it to obtain the block metadata.
This results in large no. of RPC calls to namenode, especially with
tables with large no. of files/partitions.

With this patch:
---
We move the block metadata querying to use listStatus() call which
accepts a directory as input and fetches the 'BlockLocation' objects
for every file recursively in that directory. This improves the
metadata loading in the following ways.

- For non-partitioned tables, we query all the BlockLocations in a
single RPC call in the base table directory and load the corresponding
disk IDs.

- For partitioned tables, we query the BlockLocations for all the
partitions residing under the base table directories in a single RPC
and then load every partition with non-default partition directory
separately.

Also, this patch does away with VolumeIds returned by the HDFS NN
and uses the new StorageIDs returned by the BlockLocation class.
These StorageIDs are UUID strings and hence are mapped to a
per-node 0-based index as expected by the backend. In the upcoming
versions of Hadoop APIs, getFileBlockStorageLocations() is deprecated
and instead the listStatus() returns BlockLocations with storage IDs
embedded. This patch makes use of this improvement to reduce an
additional RPC to the NN to fetch the storage locations.

Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
---
A fe/src/main/java/org/apache/impala/catalog/DiskIdMapper.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/common/FileSystemUtil.java
3 files changed, 387 insertions(+), 250 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie127658172e6e70dae441374530674a4ac9d5d26
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Mostafa Mokhtar 

[Impala-ASF-CR] Avoid std::function when possible.

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

> It's the added hidden run-time complexity generally

Can you be more specific? Abstractions naturally hide complexity; what are the 
gotchas that you think mean we shouldn't use std::function?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] Avoid std::function when possible.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

> Am I right to summarise that your main objection is that
 > std::function() heap allocates?

It's the added hidden run-time complexity generally, which is evidenced by, but 
not isolated to, the heap allocation under the hood.

 > I see that there are other reasons to prefer auto in certain
 > situations (e.g. performance), but those mostly seemed like
 > case-specific concerns rather than something we should make
 > general.

I agree.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] Add a build flag for the undefined behavior sanitizer, aka "ubsan".

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Add a build flag for the undefined behavior sanitizer, aka 
"ubsan".
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5082/1/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 40: #  -fwrapv: force signed integer arithmetic operations to wrap
> My main remaining concern would be that over/underflows won't be caught by 
Oh, I see. There's a funny thing here -fwrapv does not stop ubsan from catching 
overflows. It stops the compiler from treating overflows like UB, though.

ubsan has another flag to enable/disable overflow checking, 
-fno-sanitize=signed-integer-overflow


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

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

[Impala-ASF-CR] Add a build flag for the undefined behavior sanitizer, aka "ubsan".

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: Add a build flag for the undefined behavior sanitizer, aka 
"ubsan".
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5082/1/be/CMakeLists.txt
File be/CMakeLists.txt:

Line 40: #  -fwrapv: force signed integer arithmetic operations to wrap
> Tim, if I understand you correctly, your POV is that:
My main remaining concern would be that over/underflows won't be caught by 
UBSAN. I think mostly an uncaught overflow is just as bad even if it's 
well-defined behaviour, since it will break most calculations anyway.

It seems like regardless we're going to want to fix the overflows we found and 
then use UBSAN with -fwrapv disabled to detect regressions, and if we do that 
I'm not sure that -fwrapv will be particularly important.


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

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

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5246/1/be/src/exec/blocking-join-node.cc
File be/src/exec/blocking-join-node.cc:

PS1, Line 167: // Please keep this as the last line in this function to avoid 
use-after-free problem.
 :   // Once 'status' is set, ProcessBuildInputAndProbe() will 
start running and 'states'
 :   // may have been freed after this line once the query 
completes. IMPALA-4532.
 :   // TODO: Make this less fragile.
 :   status->Set(s);
> Thanks Henry. Would you mind if I address this in a follow-up patch ? This 
Fine by me.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change.

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..


Patch Set 1:

A better fix could be to make the main thread call Join() on the build thread.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 
Gerrit-Reviewer: Michael Ho 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


Patch Set 2:

> > Build started: http://35.164.73.121:8080/job/gerrit-verify-dryrun/43/
 > 
 > SOmething went haywire with Jenkins; I'm working on it.

Running now at http://35.164.73.121:8080/job/gerrit-verify-dryrun/44/ , but in 
dryrun mode. Still trying to figure out what's up with Jenkins

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
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: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

[Michael Ho (Code Review)]

Michael Ho has uploaded a new change for review.

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

Change subject: IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()
..

IMPALA-4532: Fix use-after-free in ProcessBuildInputAsync()

Once the Promise (of Status) is set in ProcessBuildInputAsync(),
the main thread in ProcessBuildInputAndOpenProbe() may proceed
to finish up the query and free RuntimeState. So, it's unsafe
to access the RuntimeState afterwards. Commit bb1c633 broke that
assumption (which is arguably fragile). This change fixes the
problem by adding a scope for the that counter to avoid the
use-after-free problem.

Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
---
M be/src/exec/blocking-join-node.cc
1 file changed, 15 insertions(+), 11 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I6bfd094e2e9500f1b7843486f3f745cb921764d4
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Ho 

[Impala-ASF-CR] Avoid std::function when possible.

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Avoid std::function when possible.
..


Patch Set 1:

I checked EMC++ and the ppt you linked to.

Am I right to summarise that your main objection is that std::function() heap 
allocates? Is that a different issue than classes that use STL containers also 
might heap allocate on construction?

I see that there are other reasons to prefer auto in certain situations (e.g. 
performance), but those mostly seemed like case-specific concerns rather than 
something we should make general.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: If9dc034c2e094ea7f87f78d8d9101a71d8d2e295
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Jim Apple 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2890: Support ALTER TABLE statements for Kudu tables

[Dimitris Tsirogiannis (Code Review)]

Dimitris Tsirogiannis has posted comments on this change.

Change subject: IMPALA-2890: Support ALTER TABLE statements for Kudu tables
..


Patch Set 9: Code-Review+2

Rebase. Carry +2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I04bc87e04e05da5cc03edec79d13cedfd2012896
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Internal Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


Patch Set 2:

> Build started: http://35.164.73.121:8080/job/gerrit-verify-dryrun/43/

SOmething went haywire with Jenkins; I'm working on it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
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: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


Patch Set 2:

Build started: http://35.164.73.121:8080/job/gerrit-verify-dryrun/43/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
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: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4512: Add a script that builds Impala on stock Ubuntu 
14.04.
..


Patch Set 4:

Build started: http://35.164.73.121:8080/job/gerrit-verify-dryrun/42/

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

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

[Impala-ASF-CR] IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4512: Add a script that builds Impala on stock Ubuntu 
14.04.
..


Patch Set 4: Code-Review+2

(1 comment)

Carry Tim's +2

http://gerrit.cloudera.org:8080/#/c/5154/3/bin/bootstrap_build.sh
File bin/bootstrap_build.sh:

Line 34: sudo apt-get --yes install g++ gcc git libsasl2-dev libssl-dev make 
maven openjdk-7-jdk \
> Maybe alphabetise the list, although I don't feel strongly.
Good idea; done


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

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

[Impala-ASF-CR] IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

[Jim Apple (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-4512: Add a script that builds Impala on stock Ubuntu 
14.04.
..

IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

This is a simpler alternative to bootstrap_development.sh - it
acquires enough dependencies to build, but does not attempt to load
the test data or even build the tests. This is sometimes a lightweight
testing method used by Apache PPMC members who are voting on a release
of an incubating project.

Change-Id: If34e398052a61dfda9825b1cf3a918eb61736048
---
M README.md
A bin/bootstrap_build.sh
2 files changed, 39 insertions(+), 1 deletion(-)


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

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

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5244/1/tests/query_test/test_lifecycle.py
File tests/query_test/test_lifecycle.py:

Line 65:   # timeout is 120s before they wake up and cancel themselves.
> Refer to --datastream_sender_timeout_ms (so that if we remove that flag we'
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..


Patch Set 1: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5244/1/tests/query_test/test_lifecycle.py
File tests/query_test/test_lifecycle.py:

Line 65:   # timeout is 120s before they wake up and cancel themselves.
Refer to --datastream_sender_timeout_ms (so that if we remove that flag we'll 
catch this as a place to update).


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4542: Fix use-after-free in some BE tests

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4542: Fix use-after-free in some BE tests
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I5302d665756183289edf227588da1c094e197584
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4519: increase timeout in TestFragmentLifecycle

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

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

Change subject: IMPALA-4519: increase timeout in TestFragmentLifecycle
..

IMPALA-4519: increase timeout in TestFragmentLifecycle

Increase the timeout to over 120s to match datastream_sender_timeout_ms.
This should avoid spurious test failures if we are unlucky and a sender
gets stuck waiting for a receiver fragment that will never start.

Testing:
Ran the test in a loop for a while to flush out any flakiness.

Change-Id: I9fe6e6c74538d0747e3eeb578cf0518494ff10c8
---
M tests/query_test/test_lifecycle.py
1 file changed, 2 insertions(+), 2 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-4542: Fix use-after-free in some BE tests

[Henry Robinson (Code Review)]

Henry Robinson has uploaded a new change for review.

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

Change subject: IMPALA-4542: Fix use-after-free in some BE tests
..

IMPALA-4542: Fix use-after-free in some BE tests

Change RuntimeState::ReleaseResources() to use cached ExecEnv pointer,
rather than singleton.

Change-Id: I5302d665756183289edf227588da1c094e197584
Testing: Ran affected BE tests under ASAN.
---
M be/src/runtime/runtime-state.cc
1 file changed, 1 insertion(+), 1 deletion(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I5302d665756183289edf227588da1c094e197584
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 

[Impala-ASF-CR] IMPALA-4363: Add Parquet timestamp validation

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4363: Add Parquet timestamp validation
..


Patch Set 8:

(4 comments)

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

Line 473: ErrorMsg* msg = NULL;
see comment below. this pointer is never freed.  what's the objection to 
putting the logic inside ValidateSlot() so that the memory allocation can stay 
automatic?


Line 484:   // This point is reached if slot validation succeeds, but slot 
conversion fails.
but don't we need to validate the converted value? i.e. the timestamp might be 
within bounds before conversion, but not after. do we handle that?


Line 603: *msg = new ErrorMsg(TErrorCode::PARQUET_TIMESTAMP_OUT_OF_RANGE,
this memory is leaked.


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

PS7, Line 160: g buffer. The size of the buffer should be a
> The result of a select statement would look like, for example:
What I'm asking is: don't some days legitimately have more than 
NUMBER_OF_NANOSECNODS_IN_24H when leap seconds are in play?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9988449aa0dc0f39fabb91ce6cce0dd8a06e8bcf
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

[Jim Apple (Code Review)]

Jim Apple has uploaded a new patch set (#3).

Change subject: IMPALA-4512: Add a script that builds Impala on stock Ubuntu 
14.04.
..

IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

This is a simpler alternative to bootstrap_development.sh - it
acquires enough dependencies to build, but does not attempt to load
the test data or even build the tests. This is sometimes a lightweight
testing method used by Apache PPMC members who are voting on a release
of an incubating project.

Change-Id: If34e398052a61dfda9825b1cf3a918eb61736048
---
M README.md
A bin/bootstrap_build.sh
2 files changed, 39 insertions(+), 1 deletion(-)


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

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

[Impala-ASF-CR] IMPALA-4450: qgen: use string concatenation operator for postgres queries

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4450: qgen: use string concatenation operator for 
postgres queries
..


Patch Set 4: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I149b695889addfd7df4ca5f40dc991456da51687
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4512: Add a script that builds Impala on stock Ubuntu 14.04.

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4512: Add a script that builds Impala on stock Ubuntu 
14.04.
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5154/2/bin/bootstrap_build.sh
File bin/bootstrap_build.sh:

Line 39: # when done building:
> Yeah, I agree. I was thinking of taking out almost everything except the ap
That makes sense to me


PS2, Line 44: gcc
> I thought this way it would be easier to port to other distributions, or ma
Ah, makes sense. Sounds good to me.


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

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

[Impala-ASF-CR] IMPALA-4450: qgen: use string concatenation operator for postgres queries

[Michael Brown (Code Review)]

Michael Brown has posted comments on this change.

Change subject: IMPALA-4450: qgen: use string concatenation operator for 
postgres queries
..


Patch Set 4: Code-Review+1

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I149b695889addfd7df4ca5f40dc991456da51687
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: No