[Impala-ASF-CR] IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails

[Michael Ho (Code Review)]

Michael Ho has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8572


Change subject: IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails
..

IMPALA-6184: Clean up aftr ScalarExprEvaluator::Clone() fails

When ScalarExprEvaluator::Clone() fails, the newly created evaluator was
not added to the output vector. This makes it impossible for callers to
close and clean up the evaluators afterwards. This change fixes this by
always adding the newly created evaluator to the output vector before
checking for the error status.

This path is only exercised in the scanner code. Two new tests are added
to exercise the failure paths.

Testing done: newly added tests in udf-errors.test

Change-Id: I45ffd722d0a69ad05ae3c748cf504c7f1a959a1d
---
M be/src/exprs/scalar-expr-evaluator.cc
M be/src/testutil/test-udfs.cc
M testdata/workloads/functional-query/queries/QueryTest/udf-errors.test
3 files changed, 88 insertions(+), 2 deletions(-)



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

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

[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations

[Bharath Vissapragada (Code Review)]

Bharath Vissapragada has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8545 )

Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@596
PS1, Line 596:   addTableToCatalogDeltaHelper(tbl, fromVersion, toVersion, 
resp);
> I think your assessment is correct.
I see, thanks for the explanation. I thought we are trying to make 
getCatalogDelta() non-conflicting too, since this contention can delay other 
Catalog updates like table loads etc. I understand the scope of this change 
better now, I have other comments that I'll flush out separately.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Comment-Date: Thu, 16 Nov 2017 07:00:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8545 )

Change subject: [PREVIEW] IMPALA-5058: Improve concurrency of DDL/DML operations
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java
File fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java:

http://gerrit.cloudera.org:8080/#/c/8545/1/fe/src/main/java/org/apache/impala/catalog/CatalogServiceCatalog.java@596
PS1, Line 596:   addTableToCatalogDeltaHelper(tbl, fromVersion, toVersion, 
resp);
> Maybe I'm misunderstanding something but I still see the lock contention on
I think your assessment is correct.

We are not trying to make getCatalogDelta() completely non-blocking. As you've 
correctly pointed out it can still block on a table lock.

The problem we are trying to solve is that non-conflicting operations should 
not conflict. Before this change the global lock was held for the entire 
duration of getCatalogDelta(). If a long-running operation is running against 
tbl A, and the thread executing getCatalogDelta() needs to wait to get the 
table lock on A, then no concurrent DDL whatsoever is possible due to the 
global lock. That's the main problem: a long running operation on A can prevent 
concurrent DDL against anything else.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3032437f83d39bcc8cff14d897c7c106a4ab62d3
Gerrit-Change-Number: 8545
Gerrit-PatchSet: 1
Gerrit-Owner: Dimitris Tsirogiannis 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Comment-Date: Thu, 16 Nov 2017 06:42:10 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4848: Add WIDTH BUCKET() function

[anujphadke (Code Review)]

anujphadke has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/6023 )

Change subject: IMPALA-4848: Add WIDTH_BUCKET() function
..


Patch Set 7:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc
File be/src/exprs/math-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc@429
PS7, Line 429: bucket_width
> Done
Please ignore my last comment. Accidentally got posted when replying to Dan's 
comment. I still havn't uploaded my latest patch.


http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc@431
PS7, Line 431: width_size
> Done
Please ignore my last comment. Accidentally got posted when replying to Dan's 
comment. I still havn't uploaded my latest patch.


http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc@479
PS7, Line 479: result.val = num_buckets.val;
> Done
Please ignore my last comment. Accidentally got posted when replying to Dan's 
comment. I still haven't uploaded my latest patch.


http://gerrit.cloudera.org:8080/#/c/6023/7/be/src/exprs/math-functions-ir.cc@516
PS7, Line 516:   int256_t x = ConvertToInt256(buckets.value()) * 
ConvertToInt256(width_size.value());
> This patch stores intermediate results in int256_t only when needed. Uses i
Please ignore my last comment. Accidentally got posted when replying to Dan's 
comment. I still havn't uploaded my latest patch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I081bc916b1bef7b929ca161a9aade3b54c6b858f
Gerrit-Change-Number: 6023
Gerrit-PatchSet: 7
Gerrit-Owner: anujphadke 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 16 Nov 2017 06:20:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8398 )

Change subject: IMPALA-3436: Return a decimal when rounding a double
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285:   [['round_v1','dround_v1'],
> I agree could be legit uses cases for non-const precisions.
Just to clarify. I think what Alex is saying is that the proper fix is so that 
the round function looks like this: round(double, int) -> decimal AND the 
second argument can be non-const. The limitation in this patch is that the 
second arg must be const. Alex is also saying that there should not exist a 
round() function that returns a double in decimal v2 (and I agree with this).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 16 Nov 2017 06:15:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5341: Avoid unintended filter out in fe test

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8543 )

Change subject: IMPALA-5341: Avoid unintended filter out in fe test
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie79f644a37b0ffab7b0d8e94e77650d56423697a
Gerrit-Change-Number: 8543
Gerrit-PatchSet: 2
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Comment-Date: Thu, 16 Nov 2017 05:38:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6188: make test top n reclaim less flaky

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8562 )

Change subject: IMPALA-6188: make test_top_n_reclaim less flaky
..

IMPALA-6188: make test_top_n_reclaim less flaky

Testing:
Previously I needed ~20 iterations to get the test to fail on my local
machine. After these changes I haven't been able to reproduce the
failure

Change-Id: I2bea7b0f770dec362a6df075da4e340402bd1d5d
Reviewed-on: http://gerrit.cloudera.org:8080/8562
Reviewed-by: Jim Apple 
Tested-by: Impala Public Jenkins
---
M tests/query_test/test_queries.py
1 file changed, 2 insertions(+), 1 deletion(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I2bea7b0f770dec362a6df075da4e340402bd1d5d
Gerrit-Change-Number: 8562
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-6188: make test top n reclaim less flaky

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8562 )

Change subject: IMPALA-6188: make test_top_n_reclaim less flaky
..


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2bea7b0f770dec362a6df075da4e340402bd1d5d
Gerrit-Change-Number: 8562
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Thu, 16 Nov 2017 04:57:38 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo

[Csaba Ringhofer (Code Review)]

Csaba Ringhofer has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8546 )

Change subject: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
..


Patch Set 3:

(3 comments)

> Based on my reading of "man readdir", it's not threadsafe. I think
 > the only usage here is if a user hits "http://impalad:.../; to see
 > the web UI. It's possible that there's a lock somewhere preventing
 > concurrent use, but given that this is already reasonably
 > expensive, I'd recommend using the reentrant readdir_r instead.
 >
 > I looked around, and it looks like C++17 and boost have filesystem
 > abstractions, but I think readdir() is simple enough.

I have replaced readdir() with readdir_r(), but the solution is only ok for 
Linux, and may cause problems on other Unix systems, because the size of 
dirent.d_name is not always fix.

The general solution in linux.die.net/man/3/readdir_r can be actually 
problematic, see womble.decadent.org.uk/readdir_r-advisory.html The scenario is 
not.

It is easy to allocate a buffer whose size will be surely enough in 
/proc/self/fd, but I am unsure about the ideal solution.
- Should I care about portability to other Posix variants?
- Is there a way to express "compilation error if other Posix variant than 
Linux"?
- Is there a maximum size for filenames? Most examples use 255 (+1 for \0), but 
I do not know if it is an official value or not.
- Is there a guideline for the maximum size to allocate on stack? Is ~255 ok?

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

http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-5624: Replace "ls -l" with opendir() in ProcessStateInfo
> Please describe in this line what the change does, not what it should do, i
Done


http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@11
PS2, Line 11: Posix
> nit: Posix is a name and should be capitalized.
Done


http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@18
PS2, Line 18: way to know the "expected value" in advance, and the number of 
file desciptors can
> Couldn't we test that a reasonable minimum number of file descriptors is re
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 16 Nov 2017 01:46:42 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1474: Add a metric for running queries

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/7228 )

Change subject: IMPALA-1474: Add a metric for running queries
..


Patch Set 1:

Tim, does your recent change to add registered queries metric make this change 
obsolete?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I41bc88d0e91427ca2fd70136dc6e06fa7a2663da
Gerrit-Change-Number: 7228
Gerrit-PatchSet: 1
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 16 Nov 2017 01:19:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5624: ProcessStateInfo::ReadProcFileDescriptorInfo() should not fork a process

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8546 )

Change subject: IMPALA-5624: ProcessStateInfo::ReadProcFileDescriptorInfo() 
should not fork a process
..


Patch Set 2:

> I looked around, and it looks like C++17 and boost have filesystem
 > abstractions, but I think readdir() is simple enough.

The boost abstractions have been problematic for us in the past (e.g. bugs that 
lead to exceptions thrown even when using error propagating interface), so 
agree using readdir_r is good here.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 16 Nov 2017 01:17:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6080: clean up table descriptor handling

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8330 )

Change subject: IMPALA-6080: clean up table descriptor handling
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id427dab0c196b556bd8b2d64ec618403d5cbd4d6
Gerrit-Change-Number: 8330
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 16 Nov 2017 01:07:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3436: Return a decimal when rounding a double

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8398 )

Change subject: IMPALA-3436: Return a decimal when rounding a double
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/8398/3/common/function-registry/impala_functions.py@285
PS3, Line 285:   [['round_v1','dround_v1'],
> * Yes, we want to remove decimal_v1.
I agree with Alex. I don't think it makes much sense to have a round() function 
that returns a double.

We can clearly document that round(double, int) requires a constant second 
argument in decimal_v2. If it break someone, they were probably doing something 
weird in the first place.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I240ec70da7996bbfefcf6438eba4dff8d33d7bd6
Gerrit-Change-Number: 8398
Gerrit-PatchSet: 3
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 16 Nov 2017 00:26:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6084: Avoid using of global namespace for llvm

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8489 )

Change subject: IMPALA-6084: Avoid using of global namespace for llvm
..


Patch Set 5:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8489/5/be/src/exprs/scalar-expr.cc
File be/src/exprs/scalar-expr.cc:

http://gerrit.cloudera.org:8080/#/c/8489/5/be/src/exprs/scalar-expr.cc@260
PS5, Line 260: // Compute the alignment of this value. Values should be 
self-aligned for
> Thanks for the fix. The weird formatting comes from my vi setting. Anyway,
No problem, it's easy to miss in a large patch.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7098baff5f746bcc2965583ca99f6188997b733a
Gerrit-Change-Number: 8489
Gerrit-PatchSet: 5
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 16 Nov 2017 00:19:05 +
Gerrit-HasComments: Yes