[Impala-ASF-CR] IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set of characters.

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8349 )

Change subject: IMPALA-6059: Enhance ltrim()/rtrim() functions to trim any set 
of characters.
..


Patch Set 4:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc
File be/src/exprs/string-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@406
PS4, Line 406: StringFunctions::Ltrim
nit: Can you please group these 3 functions close to *TrimString variant so for 
better legibility ?


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@411
PS4, Line 411: static_cast(" ")
This seems unnecessary now. Can we just pass StringVal::null() in this case ?


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@418
PS4, Line 418: new bitset<256>()
context->Allocate<>() ?


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@439
PS4, Line 439: delete unique_chars;
context->Free();


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@443
PS4, Line 443: DoTrimStringImpl
Seems that this function can be merged with DoTrimString().


http://gerrit.cloudera.org:8080/#/c/8349/4/be/src/exprs/string-functions-ir.cc@475
PS4, Line 475: chars_to_trim.is_null
Shouldn't we skip if chars_to_trim.is_null ? We cannot make assumption about 
len if is_null is true.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8a5ae3f59762e70c3268a01e14ed57a9e36b8d79
Gerrit-Change-Number: 8349
Gerrit-PatchSet: 4
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 15 Dec 2017 07:57:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 15 Dec 2017 06:32:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 15:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8762/15/bin/rat_exclude_files.txt
File bin/rat_exclude_files.txt:

http://gerrit.cloudera.org:8080/#/c/8762/15/bin/rat_exclude_files.txt@120
PS15, Line 120: tests/shell/shell_case_sensitive.cmds
  : tests/shell/shell_case_sensitive2.cmds
Fix for the failure:
+ bin/check-rat-report.py bin/rat_exclude_files.txt rat.xml
UNAPPROVED: tests/shell/shell_case_sensitive.cmds
UNAPPROVED: tests/shell/shell_case_sensitive2.cmds
(https://jenkins.impala.io/job/rat-check-ub1604/359/consoleText)


http://gerrit.cloudera.org:8080/#/c/8762/15/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8762/15/shell/impala_shell.py@1144
PS15, Line 1144: self.orig_cmd = "describe"
Fix for the failure:
E   desc test_do_methods_639a0d4c.test_do_methods
E   ^
E   Encountered: DESC
E   Expected: ALTER, COMPUTE, CREATE, DELETE, DESCRIBE, DROP, EXPLAIN, GRANT, 
INSERT, INVALIDATE, LOAD, REFRESH, REVOKE, SELECT, SET, SHOW, TRUNCATE, UPDATE, 
UPSERT, USE, VALUES, WITH
(https://jenkins.impala.io/job/ubuntu-16.04-from-scratch/810)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 15
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 05:27:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Hello John Russell, Andre Araujo, Zoltan Borok-Nagy, Tim Armstrong, Impala 
Public Jenkins,

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

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

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

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M LICENSE.txt
M bin/rat_exclude_files.txt
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
6 files changed, 118 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/8762/14
--
To view, visit http://gerrit.cloudera.org:8080/8762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 14
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Hello John Russell, Andre Araujo, Zoltan Borok-Nagy, Tim Armstrong, Impala 
Public Jenkins,

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

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

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

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..

IMPALA-4664: Unexpected string conversion in Shell

Impala shell can accidentally convert certain
literal strings to lowercase. Impala shell splits
each command into tokens and then converts the
first token to lowercase to figure out how it
should execute the command. The splitting is done
by spaces only. Thus, if the user types a TAB
after the SELECT, the first token after the split
becomes the SELECT plus whatever comes after it.

Testing:
TestImpalaShellInteractive.test_case_sensitive_command
TestImpalaShellInteractive.test_unexpected_conversion_for_literal_string_to_lowercase
TestImpalaShell.test_var_substitution

Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
---
M LICENSE.txt
M shell/impala_shell.py
A tests/shell/shell_case_sensitive.cmds
A tests/shell/shell_case_sensitive2.cmds
M tests/shell/test_shell_interactive.py
5 files changed, 116 insertions(+), 49 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/62/8762/13
--
To view, visit http://gerrit.cloudera.org:8080/8762
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 13
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 14:

(6 comments)

Thanks for updating the interface. Looks much better now but I do have some 
questions about the reasoning about some of the constraints imposed in the code.

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/client-request-state.h
File be/src/service/client-request-state.h:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/client-request-state.h@428
PS14, Line 428: The timeout cannot be set to a higher
  :   /// value than FLAGS_idle_session_timeout.
Can you please clarify the reasoning behind this constraint ?


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-hs2-server.cc
File be/src/service/impala-hs2-server.cc:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-hs2-server.cc@338
PS14, Line 338: 
state->SetTimeout(state->set_query_options.idle_session_timeout);
Instead of calling state->SetTimeout() here and below, may be we should just  
one call site at line 346. We can store the target timeout value in a local 
variable on at line 319 and set it to FLAGS_idle_session_timeout by default. If 
it's overridden, we can update the local variable here.


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

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc@191
PS14, Line 191: It can be overridden by the query option"
  : " 'idle_session_timeout' for specific sessions, but they 
can only have shorter"
  : " timeouts.");
Is there a reason for this ? Won't that limit the usefulness of "set 
idle_session_timeout" as a user cannot dynamically extend the timeout period ?


http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.cc@1234
PS14, Line 1234:   if (requested_timeout == 0) {
   : session_timeout = FLAGS_idle_session_timeout;
   :   } else if (FLAGS_idle_session_timeout == 0) {
   : session_timeout = requested_timeout;
   :   } else {
   : session_timeout = min(FLAGS_idle_session_timeout, 
requested_timeout);
   :   }
Sorry, I find this logic a bit confusing.

If FLAGS_idle_session_timeout is positive to begin with, setting it to zero 
will fail.

If FLAGS_idle_session_timeout is zero to begin with, I can set it to arbitrary 
non-negative number.

Otherwise, I can set it to any number between [1, FLAGS_idle_session_timeout].

What's the reasoning behind the above ? One thing which I am not so sure is 
okay is that I cannot disable idle session timeout if 
FLAGS_idle_session_timeout is originally set to a non-negative integer. In 
addition, I cannot increase the idle_session_timeout online, which seems to 
limit its usefulness to be able to be overridden per-session.

May be it helps if Greg can chime in on the legitimate expected behavior.


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

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/query-options.cc@584
PS14, Line 584:   if (requested_timeout == 0) {
  : return Status(Substitute("Session timeout cannot be 
unlimited, "
  : "maximum value: $0 seconds", 
FLAGS_idle_session_timeout));
  :   }
  :   if (requested_timeout > FLAGS_idle_session_timeout) {
  : return Status(Substitute(
  : "Session timeout cannot be set longer than $0 
seconds",
  : FLAGS_idle_session_timeout));
  :   }
Please see comments elsewhere. I don't quite understand the reasoning behind 
these constraints.


http://gerrit.cloudera.org:8080/#/c/8490/14/fe/src/test/java/org/apache/impala/util/Metrics.java
File fe/src/test/java/org/apache/impala/util/Metrics.java:

http://gerrit.cloudera.org:8080/#/c/8490/14/fe/src/test/java/org/apache/impala/util/Metrics.java@1
PS14, Line 1: //Licensed to the Apache Software Foundation (ASF) under one
: //or more contributor license agreements.  See the NOTICE file
: //distributed with this work for additional information
: //regarding copyright ownership.  The ASF licenses this file
: //to you under the Apache License, Version 2.0 (the
: //"License"); you may not use this file except in compliance
: //with the License.  You may obtain a copy of the License at
: //
: //http://www.apache.org/licenses/LICENSE-2.0
: //
: //Unless required by applicable law or agreed to in writing,
: //software distributed under the 

[Impala-ASF-CR] IMPALA-6300: Fix decimal modulo overflow

[Taras Bobrovytsky (Code Review)]

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

Change subject: IMPALA-6300: Fix decimal modulo overflow
..


Patch Set 1:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@190
PS1, Line 190:   int y_lz = BitUtil::CountLeadingZeros(abs(y));
> Ouch
?


http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@261
PS1, Line 261: #ifdef DEBUG
> Is there some way we can avoid having different control flow on release and
Done. The compiler should optimize it away if it's false, right?


http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@361
PS1, Line 361: RESULT_T x = 0;
> Unnecessary change - can you revert?
Done


http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@571
PS1, Line 571: result = x % y;
> I think it would probably be faster and simpler to just use 64-bit arithmet
Done. I ran a benchmark and it turns that it is indeed better to simply use 
64-bit arithmetic.


http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@641
PS1, Line 641:   return true;
> Since this can never be true if callers respect the comment above, maybe th
Good idea. We can simply remove this. The DCHECK is inside SafeMultiply().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686
Gerrit-Change-Number: 8833
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Fri, 15 Dec 2017 02:42:09 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6300: Fix decimal modulo overflow

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8833 )

Change subject: IMPALA-6300: Fix decimal modulo overflow
..

IMPALA-6300: Fix decimal modulo overflow

In order to compute the modulo of two decimals, we need to bring the
underlying datatype to the same scale first. It turns out we could
overflow when scaling up one of the values.

In this patch we fix the problem by using a larger data type when we
detect that the scaled up value will not fit into the original data
type.

Testing:
- Added some expr tests that reproduce the issue.

Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686
---
M be/src/exprs/expr-test.cc
M be/src/runtime/decimal-value.h
M be/src/runtime/decimal-value.inline.h
M be/src/util/bit-util.h
4 files changed, 121 insertions(+), 74 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686
Gerrit-Change-Number: 8833
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8762 )

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 12: Code-Review-1

Let me take a look at the failure and provide a new patch set.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 02:23:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 12: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 12
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 02:12:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6290: limit ScannerContext to 1 buffer at a time.

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8814 )

Change subject: IMPALA-6290: limit ScannerContext to 1 buffer at a time.
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8814/4/be/src/exec/scanner-context.h
File be/src/exec/scanner-context.h:

http://gerrit.cloudera.org:8080/#/c/8814/4/be/src/exec/scanner-context.h@231
PS4, Line 231: ScannerContext
scan range?


http://gerrit.cloudera.org:8080/#/c/8814/4/be/src/exec/scanner-context.cc
File be/src/exec/scanner-context.cc:

http://gerrit.cloudera.org:8080/#/c/8814/4/be/src/exec/scanner-context.cc@238
PS4, Line 238:   // First this loop ensures that we've got all of the requested 
bytes in either
nit: Assuming that this comment covers the while loop to the empty line after 
it, I think that data is not necessarily in either one or the other buffer, but 
can be in both combined. Then more copying may happen below.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I74c5960a75f7d88b0e1de4199af731fb13e592f0
Gerrit-Change-Number: 8814
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 15 Dec 2017 02:09:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5191: Standardize column alias behavior

[Alex Behm (Code Review)]

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

Change subject: IMPALA-5191: Standardize column alias behavior
..


Patch Set 6:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8801/6/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@289
PS6, Line 289:   Analyzer analyzer, boolean substituteChildren) throws 
AnalysisException {
* Adding this new flag here and in the Expr substitute() functions is a pretty 
invasive change for this simple fix.
* We only ever call this
substituteOrdinalsAliases() with substituteChildren==false because that's what 
the new behavior or, so the existence of this flag is even confusing
* This function is the only place where we call the Expr substitute functions 
with substituteChildren==false, so modifying the Expr APIs does not make sense 
to me
* The simpler alternative is to only substitute exprs that are a SlotRef, i.e. 
in L305 you can continue if the expr is not a SlotRef, otherwise substitute 
just as before (i.e. the fix for this JIRA can be a one-line change)
* Please remove the substituteChildren flag here and elsewhere because that the 
change is too invasive for a simple change


http://gerrit.cloudera.org:8080/#/c/8801/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
File fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java:

http://gerrit.cloudera.org:8080/#/c/8801/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@956
PS6, Line 956: AnalyzesOk("with w_test as (select '1' as one, 2 as two, '3' 
as three) "
These tests have nothing to do with analytic exprs, so they don't belong in 
TestAnalyticExprs()?

I think it's best to rename TestOrdinals() to TestAliasesAndOrdinals() and 
consolidate the relevant tests in there.


http://gerrit.cloudera.org:8080/#/c/8801/6/fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java@961
PS6, Line 961: AnalyzesOk("select int_col / 2 as x from functional.alltypes 
group by x");
Would be good to add tests like these if not already present:

* select count(*) a from t group by a - should error
* select count(*) a from t having a - should error
* select count(*) > 10 a from t having a - should work
* select count(*) a from t order by a - should work
* select sum() over(order by id) a from t group by a - should error
* select sum() over(order by id) a from t order by a - should work ok

Etc. You get the idea.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 01:40:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality for NumericLiteral::localEquals().

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6114: Require type equality for 
NumericLiteral::localEquals().
..

IMPALA-6114: Require type equality for NumericLiteral::localEquals().

This patch fixes a regression introduced as part of IMPALA-1788, where
an expression like 'CAST(0 AS DECIMAL(14))' is rewritten as a
NumericLiteral expression of type DECIMAL(14,0). The query had
another NumericLiteral of type TINYINT. While analyzing the DISTINCT
aggregation clause of the SELECT query, AggregateInfo::create() removes
duplicate expressions from groupingExprs.

NumericLiteral::localEquals() is used to check for equality. Now
since the method does not consider expression types, a TINYINT literal
is considered to be duplicate of a DECIMAL literal. This results in a
query like the following to fail:

SELECT DISTINCT CAST(0 AS DECIMAL(14), 0 FROM functional.alltypes

We propose to fix the issue by accounting for types as well
when comparing analyzed numeric literals.

A test case has been added to AnalyzeStmtsTest.

Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Reviewed-on: http://gerrit.cloudera.org:8080/8448
Reviewed-by: Alex Behm 
Tested-by: Impala Public Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
2 files changed, 11 insertions(+), 1 deletion(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 7
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 

[Impala-ASF-CR] IMPALA-6114: Require type equality for NumericLiteral::localEquals().

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6114: Require type equality for 
NumericLiteral::localEquals().
..


Patch Set 6: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 6
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Fri, 15 Dec 2017 01:28:12 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE
..


Patch Set 4:

(1 comment)

The code and tests look good to me, had one outstanding question that came out 
of a discussion.

http://gerrit.cloudera.org:8080/#/c/8820/4//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8820/4//COMMIT_MSG@7
PS4, Line 7: IMPALA-5654: Disallow setting Kudu table name in CREATE TABLE
Gabor mentioned to me offline that we might want to catch it in alter table too 
(unsure if that is already disallowed).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieca037498abf8f5fde67b77e824b720482cdbe6f
Gerrit-Change-Number: 8820
Gerrit-PatchSet: 4
Gerrit-Owner: Gabor Kaszab 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 01:00:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-3703: Store query context in thread-local variables
..


Patch Set 12: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8621/12/be/src/common/thread-debug-info-test.cc
File be/src/common/thread-debug-info-test.cc:

http://gerrit.cloudera.org:8080/#/c/8621/12/be/src/common/thread-debug-info-test.cc@30
PS12, Line 30:
Not a big deal, but there's a bit more vertical whitespace in these tests than 
we usually prefer.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 15 Dec 2017 00:54:51 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 2:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 15 Dec 2017 00:28:09 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3942: Fix unexpected conversion of string literal in front-end

[Kim Jin Chul (Code Review)]

Kim Jin Chul has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8818 )

Change subject: IMPALA-3942: Fix unexpected conversion of string literal in 
front-end
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8818/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java
File fe/src/main/java/org/apache/impala/analysis/StringLiteral.java:

http://gerrit.cloudera.org:8080/#/c/8818/1/fe/src/main/java/org/apache/impala/analysis/StringLiteral.java@49
PS1, Line 49: boolean useSingleQuote = true;
> Just considering what options we have here:
Your suggestion is possible. Actually I tried what you suggested firstly which 
might be same as Jim's proposal, but I had to handle some corner cases because 
value_ contains can have non of/single/double quotes. I still have the ongoing 
code locally, but the change is bigger than this and I am not sure any hole 
still exists. If you and Jim still think "keeping a given string literal at  
value_", please let me know pros and cons compared with this version.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibc4b5f5d8ffaa8feb96a466959427a04b3b06fec
Gerrit-Change-Number: 8818
Gerrit-PatchSet: 1
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 15 Dec 2017 00:10:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6300: Fix decimal modulo overflow

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6300: Fix decimal modulo overflow
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h
File be/src/runtime/decimal-value.inline.h:

http://gerrit.cloudera.org:8080/#/c/8833/1/be/src/runtime/decimal-value.inline.h@261
PS1, Line 261: #ifdef DEBUG
Is there some way we can avoid having different control flow on release and 
debug builds. E.g. a SafeMultiply overload that takes a bool may_overflow param?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27c7f25f68353c19c315e1639311ec06b2dea686
Gerrit-Change-Number: 8833
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zach Amsden 
Gerrit-Comment-Date: Thu, 14 Dec 2017 23:38:02 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 23:21:52 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5315: Cast to timestamp fails for YYYY-M-D format

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-5315: Cast to timestamp fails for -M-D format
..


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/7009/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/7009/10//COMMIT_MSG@39
PS10, Line 39: Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
I think we need some additional tests for bad data and some query tests that 
scan tables with mixed-format timestamps to test that it correctly switches to 
a different context.

I was able to crash things with the current patch fairly easily. Repro:

drop table if exists timestamp_strs;
create table timestamp_strs as select '2001-1-2' as str;
insert into timestamp_strs select '2001-10-2';
insert into timestamp_strs select * from timestamp_strs;
insert into timestamp_strs select * from timestamp_strs;
insert into timestamp_strs select * from timestamp_strs;
insert into timestamp_strs select * from timestamp_strs;
insert into timestamp_strs select '2001-foo-bar';
select str, cast(str as timestamp) from timestamp_strs;


http://gerrit.cloudera.org:8080/#/c/7009/10/be/src/runtime/timestamp-parse-util.h
File be/src/runtime/timestamp-parse-util.h:

http://gerrit.cloudera.org:8080/#/c/7009/10/be/src/runtime/timestamp-parse-util.h@267
PS10, Line 267:   static DateTimeFormatContext LAZY_CTX;
Having this as a static variable doesn't seem right, since it will be shared 
between multiple threads, which will updated it. I don't see a particularly 
easy place to put the last context so that it's local to the thread/column 
that's executing Parse().

I'm wondering if it's just easiest to have lazy_ctx be a local variable that we 
create on the fly - we'd take a 50% perf hit for the lazy formats according to 
your benchmark but it's not clear to me that the optimisation is worth the 
extra complexity of having to manage LAZY_CTX.


http://gerrit.cloudera.org:8080/#/c/7009/10/be/src/runtime/timestamp-parse-util.cc
File be/src/runtime/timestamp-parse-util.cc:

http://gerrit.cloudera.org:8080/#/c/7009/10/be/src/runtime/timestamp-parse-util.cc@378
PS10, Line 378: LIKELY
LIKELY doesn't seem right here - I think we can just remove it.


http://gerrit.cloudera.org:8080/#/c/7009/10/be/src/runtime/timestamp-parse-util.cc@381
PS10, Line 381: if (LAZY_CTX.fmt != NULL) dt_ctx = _CTX;
nit: we use braces on both branches if there is an else.


http://gerrit.cloudera.org:8080/#/c/7009/10/be/src/runtime/timestamp-parse-util.cc@383
PS10, Line 383: LAZY_CTX.Reset(str, len);
nit: missing indentation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib9a184a09d7e7783f04d47588537612c2ecec28f
Gerrit-Change-Number: 7009
Gerrit-PatchSet: 10
Gerrit-Owner: Vincent Tran 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vincent Tran 
Gerrit-Comment-Date: Thu, 14 Dec 2017 23:21:32 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/8836 )

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..

IMPALA-6284: Mark the intermediate decimal avg struct as packed

We saw some failures on the exhaustive release build because the
compiler assumed that the pointer to the intermediate struct that is
used for computing decimal average was aligned.

To fix the problem, we mark the struct with a "packed" attribute so
that the compiler does not expect it to be aligned.

Testing:
- Ran the failing test locally on an release build and it passed.

Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
---
M be/src/exprs/aggregate-functions-ir.cc
M fe/src/main/java/org/apache/impala/catalog/BuiltinsDb.java
2 files changed, 6 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 2
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Taras Bobrovytsky (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8836/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8836/1/be/src/exprs/aggregate-functions-ir.cc@388
PS1, Line 388:   DCHECK_EQ(dst->len, sizeof(DecimalAvgState));
> Did you test on a debug build? I think there's a constant in the FE that ne
Fixed. The BE tests on debug build passed after the fix (they were failing 
before).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 23:00:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4835: Part 1: simplify I/O mgr mem mgmt and cancellation
..


Patch Set 15: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If5cb42437d11c13bc4a55c3ab426b66777332bd1
Gerrit-Change-Number: 8414
Gerrit-PatchSet: 15
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:53:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8621 )

Change subject: IMPALA-3703: Store query context in thread-local variables
..


Patch Set 12: Code-Review+1

Thank you for making all the changes! I think it looks very good now.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 12
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:42:40 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4664: Unexpected string conversion in Shell

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4664: Unexpected string conversion in Shell
..


Patch Set 11: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifdce9781d1d97596c188691b62a141b9bd137610
Gerrit-Change-Number: 8762
Gerrit-PatchSet: 11
Gerrit-Owner: Kim Jin Chul 
Gerrit-Reviewer: Andre Araujo 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:39:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6177: Cleanup incomplete handcrafted IRs before finalizing module

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6177: Cleanup incomplete handcrafted IRs before 
finalizing module
..


Patch Set 10: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: If975cfb3906482b36dd6ede32ca81de6fcee1d7f
Gerrit-Change-Number: 8541
Gerrit-PatchSet: 10
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:38:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6222: Add details to error msg on failure to get min reservation

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6222: Add details to error msg on failure to get min 
reservation
..


Patch Set 4: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Gerrit-Change-Number: 8781
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 22:34:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6222: Add details to error msg on failure to get min reservation

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6222: Add details to error msg on failure to get min 
reservation
..

IMPALA-6222: Add details to error msg on failure to get min reservation

This patch adds the following details to the error message encountered
on failure to get minimum memory reservation:
- which ReservationTracker hit its limit
- top 5 admitted queries that are consuming the most memory under the
ReservationTracker that hit its limit

Testing:
- added tests to reservation-tracker-test.cc that verify the error
message returned for different cases.
- tested "initial reservation failed" condition manually to verify
the error message returned.

Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Reviewed-on: http://gerrit.cloudera.org:8080/8781
Reviewed-by: Bikramjeet Vig 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/initial-reservations.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M common/thrift/generate_error_codes.py
7 files changed, 176 insertions(+), 39 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Gerrit-Change-Number: 8781
Gerrit-PatchSet: 5
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6290: limit ScannerContext to 1 buffer at a time.

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6290: limit ScannerContext to 1 buffer at a time.
..


Patch Set 3:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/hdfs-scanner.h
File be/src/exec/hdfs-scanner.h:

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/hdfs-scanner.h@97
PS3, Line 97: /// This class also encapsulates row batch management.  
Subclasses should call CommitRows()
> nit: long line (and the new Gerrit UI breaks it now, making it look bad).
Done


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h
File be/src/exec/scanner-context.h:

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@143
PS3, Line 143: /// (the scan range could be longer than the file).
> Can we extend this comment with what the implication are? Is the stream sti
Done


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@146
PS3, Line 146: /// If true, the stream has reached the end of the file.
> Can we extend this comment with what the implication are? Is the stream sti
Done


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@161
PS3, Line 161: bool ReadBoolean(bool* boolean, Status* status);
> Should we add WARN_UNUSED_RESULT while you're here?
Done. The calling convention of these functions is a bit weir d to avoid 
overhead of status, because returning false implies !status->ok() and 
vice-versa, so some caller were ignoring the return value. I kept the calling 
convention and adjusted the callers to use a pattern that won't trip the 
warning.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@193
PS3, Line 193: /// Release resources from previous reads in this stream. If 
'done' is true all
> This only releases buffers that are completely read, right? In that case, w
Updated this and the other ReleaseCompleteResources() comment.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@224
PS3, Line 224: initial
> "initial" implies that there are more, no?
We create extra scan ranges to read past the end of the stream. Agree the 
adjective is confusing so reworded ot be less ambiguous.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@254
PS3, Line 254: // We always want output_buffer_bytes_left_ to be non-NULL, 
so we can avoid a NULL check
> nit: long line
Done


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@298
PS3, Line 298: 2
> Huh?
Fat finger error.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.h@320
PS3, Line 320: Always
 :   /// returns the current I/O buffer to the I/O manager.
> This only seems true when the buffer has been read/skipped completely.
Done


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc
File be/src/exec/scanner-context.cc:

http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@101
PS3, Line 101: buffer_range->ReturnBuffer(move(io_buffer_));
> Can we reset io_buffer_pos_ here, too? It looks to me below like we're usin
I don't think we should be inferring eos from the state of 'io_buffer_' - where 
did you see that? I think we should only be checking it immediately after the 
GetNext() calls.

I factored out this logic into a ReturnIoBuffer() that resets the related 
members though.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@182
PS3, Line 182:   if (eosr()) return Status::OK();
> Can you explain here (or in the header at either this function or scan_rang
eosr() mean that all the data in the range was returned to the client. 
scan_range_eosr_ is true and eosr() is false when the last buffer is in the 
scan range but hasn't been fully returned to the client.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@226
PS3, Line 226: Status ScannerContext::Stream::GetBytesInternal(int64_t 
requested_len,
> This method still looks very confusing to me. Can you think of ways to make
I added some comments to try and make it clearer. I think the current code 
structure is reasonable with more explanation - it first ensures that it has 
all the data, then figures out how to return it.

I don't think separating the case 1/2 branches earlier results in clearer code, 
because if we don't have a current io_buffer_, we need to fetch the next I/O 
buffer to figure out if it has all the required bytes.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@228
PS3, Line 228:   DCHECK_GT(requested_len, boundary_buffer_bytes_left_);
> Can you also add a DCHECK to document and assert requested_len > io_buffer_
Done. That precondition isn't exactly right but I added one.


http://gerrit.cloudera.org:8080/#/c/8814/3/be/src/exec/scanner-context.cc@260
PS3, Line 260: num_bytes
> This is the 

[Impala-ASF-CR] IMPALA-6290: limit ScannerContext to 1 buffer at a time.

[Tim Armstrong (Code Review)]

Hello Lars Volker,

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

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

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

Change subject: IMPALA-6290: limit ScannerContext to 1 buffer at a time.
..

IMPALA-6290: limit ScannerContext to 1 buffer at a time.

This is a prerequisite for constraining the number of buffers per scan
range. Before this patch, calling ReadBytes(), SkipBytes(), etc could
cause an arbitrary number of I/O buffers to accumulate in
'completed_io_buffers_'. E.g. if we allocated 3 * 8MB I/O buffers for
a range and then called ReadBytes(30MB), we would hit resource
exhaustion as soon as 3 buffers were accumulated in
'completed_io_buffers_'.

The fix is to avoid accumulating any buffers in 'completed_io_buffers_'.
Instead of adding them to 'completed_io_buffers_', completed buffers
are just returned to the I/O manager. It turned out that this did not
weaken the ScannerContext's guarantees about memory lifetime, because
ScannerContext::GetBytesInternal() cleared 'boundary_buffer_' each
time it was called regardless. I checked that this behaviour wasn't
a bug by inspecting the scanner code. I could not find any cases
where scanners depended on returned memory remaining valid beyond
the next Read*()/Get*()/Skip*() call on the stream.

This change makes that lifetime explicit in the comments. A
side-effect of this fix is that scanners do not need to call
ReleaseCompletedResources() in CommitRows() and means that the
ScannerContext only ever needs to hold one I/O buffer at a time.

This change also reimplements SkipBytes() to avoid it accumulating
memory in the boundary buffer for large skip sizes.

Also clarifies some of the invariants in ScannerContext. E.g. some
places assumed io_buffer_ != NULL, but that is no longer needed.

Testing:
Ran core tests with ASAN and exhaustive tests with DEBUG.

Change-Id: I74c5960a75f7d88b0e1de4199af731fb13e592f0
---
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/hdfs-parquet-scanner.cc
M be/src/exec/hdfs-scanner.cc
M be/src/exec/hdfs-scanner.h
M be/src/exec/hdfs-text-scanner.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/exec/scanner-context.inline.h
M 
testdata/workloads/functional-query/queries/DataErrorsTest/hdfs-scan-node-errors.test
10 files changed, 295 insertions(+), 201 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I74c5960a75f7d88b0e1de4199af731fb13e592f0
Gerrit-Change-Number: 8814
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

[Zoltan Borok-Nagy (Code Review)]

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8621 )

Change subject: IMPALA-3703: Store query context in thread-local variables
..


Patch Set 11:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@58
PS11, Line 58:
> nit: remove this empty line.
Done


http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@73
PS11, Line 73:   const int64_t front_length = THREAD_NAME_SIZE - 
tail_length - 4; // 4 is the length
> I think if the comment doesn't fit into the end of a line it's better to pu
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@96
PS10, Line 96: THREAD_NAME_TAIL_LENGTH - 4; // 4 is the length of "..." and 
'\0'
> THREAD_NAME_FRONT_LENGTH still shows up to the right, no? I think it's best
Oops, now I removed it really.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:38:16 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6114: Require type equality of NumericLiteral::localEquals().

[Zoram Thanga (Code Review)]

Zoram Thanga has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8448 )

Change subject: IMPALA-6114: Require type equality of 
NumericLiteral::localEquals().
..


Patch Set 5:

Thanks for the comments. Please have a look at patch set #6


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia88d54088dfd128b103759dc01103b6c35bf6257
Gerrit-Change-Number: 8448
Gerrit-PatchSet: 5
Gerrit-Owner: Zoram Thanga 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Zoram Thanga 
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:31:20 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8621 )

Change subject: IMPALA-3703: Store query context in thread-local variables
..


Patch Set 11:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@58
PS11, Line 58:
nit: remove this empty line.


http://gerrit.cloudera.org:8080/#/c/8621/11/be/src/common/thread-debug-info.h@73
PS11, Line 73:   const int64_t front_length = THREAD_NAME_SIZE - 
tail_length - 4; // 4 is the length
I think if the comment doesn't fit into the end of a line it's better to put it 
above the line instead of breaking it up.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@96
PS10, Line 96: THREAD_NAME_TAIL_LENGTH - 4; // 4 is the length of "..." and 
'\0'
> I removed THREAD_NAME_FRONT_LENGTH.
THREAD_NAME_FRONT_LENGTH still shows up to the right, no? I think it's best to 
have NAME_SIZE and TAIL_LENGTH and then compute the rest in SetThreadName().



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 21:07:59 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

[Gabor Kaszab (Code Review)]

Gabor Kaszab has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 14: Code-Review+1

Thanks for sharing your view, Zoli!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 14
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 19:16:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3887: Wait for HDFS replication in data loading

[Philip Zeyliger (Code Review)]

Philip Zeyliger has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8846 )

Change subject: IMPALA-3887: Wait for HDFS replication in data loading
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8846/1/testdata/bin/create-load-data.sh
File testdata/bin/create-load-data.sh:

http://gerrit.cloudera.org:8080/#/c/8846/1/testdata/bin/create-load-data.sh@453
PS1, Line 453:   while : ; do
I think we need a number of tries or a timeout here. If HDFS is stuck forever, 
the build would just stall.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I88dfb7165b7515b3e96111436be490f2068ec322
Gerrit-Change-Number: 8846
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Thu, 14 Dec 2017 19:14:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3887: Wait for HDFS replication in data loading

[Tianyi Wang (Code Review)]

Tianyi Wang has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8846


Change subject: IMPALA-3887: Wait for HDFS replication in data loading
..

IMPALA-3887: Wait for HDFS replication in data loading

When the data loading finishes, it is possible for some HDFS blocks to
be under replicated. If impala gets the metadata before the replication
is done, some tests may fail. This patch adds a replication waiting step
in the data loading script.

Change-Id: I88dfb7165b7515b3e96111436be490f2068ec322
---
M testdata/bin/create-load-data.sh
1 file changed, 13 insertions(+), 0 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I88dfb7165b7515b3e96111436be490f2068ec322
Gerrit-Change-Number: 8846
Gerrit-PatchSet: 1
Gerrit-Owner: Tianyi Wang 

[Impala-ASF-CR] IMPALA-6222: Add details to error msg on failure to get min reservation

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6222: Add details to error msg on failure to get min 
reservation
..


Patch Set 4:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Gerrit-Change-Number: 8781
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 18:59:34 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6222: Add details to error msg on failure to get min reservation

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8781 )

Change subject: IMPALA-6222: Add details to error msg on failure to get min 
reservation
..


Patch Set 4: Code-Review+2

carrying over Tim's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Gerrit-Change-Number: 8781
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 18:52:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6222: Add details to error msg on failure to get min reservation

[Bikramjeet Vig (Code Review)]

Bikramjeet Vig has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8781 )

Change subject: IMPALA-6222: Add details to error msg on failure to get min 
reservation
..


Patch Set 3:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8781/3/be/src/runtime/mem-tracker.cc
File be/src/runtime/mem-tracker.cc:

http://gerrit.cloudera.org:8080/#/c/8781/3/be/src/runtime/mem-tracker.cc@324
PS3, Line 324:   std::priority_queue, 
vector>,
> nit: add a using up the top of the file or in common/names.h instead of usi
Done


http://gerrit.cloudera.org:8080/#/c/8781/3/be/src/runtime/mem-tracker.cc@340
PS3, Line 340: int limit) {
> nit: weird wrapping.
4 extra spaces before std::greater as it is a part of the template parameter 
list of std::priority_queue

"int limit" at the same level as  "std::priority_queue" as both are a part of 
the function parameter list



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Gerrit-Change-Number: 8781
Gerrit-PatchSet: 3
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 18:48:40 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6222: Add details to error msg on failure to get min reservation

[Bikramjeet Vig (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-6222: Add details to error msg on failure to get min 
reservation
..

IMPALA-6222: Add details to error msg on failure to get min reservation

This patch adds the following details to the error message encountered
on failure to get minimum memory reservation:
- which ReservationTracker hit its limit
- top 5 admitted queries that are consuming the most memory under the
ReservationTracker that hit its limit

Testing:
- added tests to reservation-tracker-test.cc that verify the error
message returned for different cases.
- tested "initial reservation failed" condition manually to verify
the error message returned.

Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
---
M be/src/runtime/bufferpool/reservation-tracker-test.cc
M be/src/runtime/bufferpool/reservation-tracker.cc
M be/src/runtime/bufferpool/reservation-tracker.h
M be/src/runtime/initial-reservations.cc
M be/src/runtime/mem-tracker.cc
M be/src/runtime/mem-tracker.h
M common/thrift/generate_error_codes.py
7 files changed, 176 insertions(+), 39 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic4675fe923b33fdc4ddefd1872e6d6b803993d74
Gerrit-Change-Number: 8781
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5948: Change Kudu RPC port to 27000

[Michael Ho (Code Review)]

Michael Ho has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8841 )

Change subject: IMPALA-5948: Change Kudu RPC port to 27000
..


Patch Set 1:

You may want to update it here too: 
https://github.com/apache/impala/blob/master/bin/start-impala-cluster.py#L192


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf5ccedfd9bc1eff9786e4b019c1bb68bf757300
Gerrit-Change-Number: 8841
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Ho 
Gerrit-Comment-Date: Thu, 14 Dec 2017 18:40:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5948: Change Kudu RPC port to 27000

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8841 )

Change subject: IMPALA-5948: Change Kudu RPC port to 27000
..


Patch Set 1: Code-Review+1

I confirmed that port 27000 is not used by major Hadoop distributions by 
looking at these pages:

https://www.cloudera.com/documentation/enterprise/latest/topics/cm_ig_ports.html
https://docs.hortonworks.com/HDPDocuments/HDP2/HDP-2.6.3/bk_reference/content/reference_chap2.html
http://doc.mapr.com/display/MapR/Ports+Used+by+MapR


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf5ccedfd9bc1eff9786e4b019c1bb68bf757300
Gerrit-Change-Number: 8841
Gerrit-PatchSet: 1
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 14 Dec 2017 18:12:28 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 1:

UBSAN might also be able to detect problems like this (although I haven't 
investigated)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 17:54:02 +
Gerrit-HasComments: No

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

[Pranay Singh (Code Review)]

Hello Taras Bobrovytsky, Joe McDonnell, Tim Armstrong, Bikramjeet Vig, Impala 
Public Jenkins,

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

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

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

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

IMPALA-5522:Use tracked memory for DictDecoder and DictEncoder

Currently DictDecoder class and DictEncoder class uses std::vector
to store the tables mapping codeword to value and vice-versa. It is
hard to detect the memory usage by these tables when they becomes
very large, since this memory is not accounted by Impala's memory
mangement infrastructure.

This patch uses the memory tracker of HdfsScanner to track the memory used
by dictionary in DictDecoder class. Similary it uses memory tracker of
HdfsTableSink to track the memory used by dictionary in DictEncoder class.

Memory for the dictionary, stored as std::vector is still allocated
from std:allocator but the amount allocated is accounted by
introducing a counter which is incremented and decremented as the
memory is consumed and released by vector.

Testing
---
Ran all the backend and end-end tests with no failures.

Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
---
M be/src/exec/hdfs-parquet-table-writer.cc
M be/src/exec/parquet-column-readers.cc
M be/src/exec/parquet-column-readers.h
M be/src/util/dict-encoding.h
M be/src/util/dict-test.cc
5 files changed, 173 insertions(+), 24 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/34/8034/19
--
To view, visit http://gerrit.cloudera.org:8080/8034
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I02a3b54f6c107d19b62ad9e1c49df94175964299
Gerrit-Change-Number: 8034
Gerrit-PatchSet: 19
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Taras Bobrovytsky (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 1:

I'm not really sure what a new test for this would look like. We already have 
tests that trigger the problem on an exhaustive release build.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 17:50:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8836/1/be/src/exprs/aggregate-functions-ir.cc
File be/src/exprs/aggregate-functions-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8836/1/be/src/exprs/aggregate-functions-ir.cc@388
PS1, Line 388:   DCHECK_EQ(dst->len, sizeof(DecimalAvgState));
Did you test on a debug build? I think there's a constant in the FE that needs 
to be updated too (DECIMAL_AVG_INTERMEDIATE_SIZE). This DCHECK is meant to 
assert that the fe and be sizes are the same.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 17:50:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5948: Change Kudu RPC port to 27000

[Joe McDonnell (Code Review)]

Joe McDonnell has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8841


Change subject: IMPALA-5948: Change Kudu RPC port to 27000
..

IMPALA-5948: Change Kudu RPC port to 27000

The current default for krpc_port is 29000, which conflicts
with Sentry's WebUI. This changes the default to 27000,
which is currently free.

Core tests pass with this change.

Change-Id: Iaf5ccedfd9bc1eff9786e4b019c1bb68bf757300
---
M be/src/common/global-flags.cc
1 file changed, 1 insertion(+), 1 deletion(-)



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

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

[Impala-ASF-CR] IMPALA-6284: Mark the intermediate decimal avg struct as packed

[Alex Behm (Code Review)]

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

Change subject: IMPALA-6284: Mark the intermediate decimal avg struct as packed
..


Patch Set 1:

Test?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id25ec6e20dde3f50fb37a22135b355ad251809e0
Gerrit-Change-Number: 8836
Gerrit-PatchSet: 1
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 14 Dec 2017 17:28:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5310: Part 3: Use SAMPLED NDV() in COMPUTE STATS.

[Alex Behm (Code Review)]

Alex Behm has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8840


Change subject: IMPALA-5310: Part 3: Use SAMPLED_NDV() in COMPUTE STATS.
..

IMPALA-5310: Part 3: Use SAMPLED_NDV() in COMPUTE STATS.

Modifies COMPUTE STATS TABLESAMPLE to use the new SAMPLED_NDV()
function.

Testing:
- modified/improved existing functional tests
- core/hdfs run passed

Change-Id: I6ec0831f77698695975e45ec0bc0364c765d819b
---
M be/src/exec/catalog-op-executor.cc
M common/thrift/JniCatalog.thrift
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
D 
testdata/workloads/functional-query/queries/QueryTest/compute-stats-tablesample.test
M tests/common/impala_test_suite.py
M tests/custom_cluster/test_stats_extrapolation.py
M tests/query_test/test_aggregation.py
8 files changed, 183 insertions(+), 259 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I6ec0831f77698695975e45ec0bc0364c765d819b
Gerrit-Change-Number: 8840
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm 

[Impala-ASF-CR] IMPALA-5191: Standardize column alias behavior

[Zoltan Borok-Nagy (Code Review)]

Hello Taras Bobrovytsky, Tim Armstrong, Alex Behm,

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

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

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

Change subject: IMPALA-5191: Standardize column alias behavior
..

IMPALA-5191: Standardize column alias behavior

We should not perform alias substitution in the
subexpressions of GROUP BY, HAVING, and ORDER BY
to be more standard conformant.

=== Allowed ===
SELECT int_col / 2 AS x
FROM functional.alltypes
GROUP BY x;

SELECT int_col / 2 AS x
FROM functional.alltypes
ORDER BY x;

SELECT NOT bool_col AS nb
FROM functional.alltypes
GROUP BY nb
HAVING nb;

=== Not allowed ===
SELECT int_col / 2 AS x
FROM functional.alltypes
GROUP BY x / 2;

SELECT int_col / 2 AS x
FROM functional.alltypes
ORDER BY -x;

SELECT int_col / 2 AS x
FROM functional.alltypes
GROUP BY x
HAVING x > 3;

A flag called substituteChildren is added to the
alias substitution methods. If the flag is false,
the methods won't substitute the aliases of child
expressions.

Some extra checks were added to AnalyzeExprsTest.java.

I had to update other tests to make them pass
since the new behavior is more restrictive.

I added alias.test to the end-to-end tests.

Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
A testdata/workloads/functional-query/queries/QueryTest/alias.test
M tests/query_test/test_queries.py
12 files changed, 145 insertions(+), 29 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0f82483b486acf6953876cfa672b0d034f3709a8
Gerrit-Change-Number: 8801
Gerrit-PatchSet: 6
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-2248: Make idle session timeout a query option

[Zoltan Borok-Nagy (Code Review)]

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8490 )

Change subject: IMPALA-2248: Make idle_session_timeout a query option
..


Patch Set 14:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.h
File be/src/service/impala-server.h:

http://gerrit.cloudera.org:8080/#/c/8490/14/be/src/service/impala-server.h@452
PS14, Line 452:   friend class SessionState;
> Well, I feel here that making SessionState a friend class so that the Regis
I actually made the register/unregister functions private in response to 
Michael's comment.

I think it's reasonable that timeouts can only be registered via 
SessionState::SetTimeout().

I agree that it is unfortunate that SessionState now accesses all members of 
ImpalaServer. However, friend declarations only extend the access of private 
fields, but do not break the access boundary.
Making a member public allows everybody to depend on that member. But, it might 
be not a huge issue in application code.

SessionState is defined inside the class definition of ImpalaServer, therefore 
making it a friend is not a huge problem either in my opinion.

It is aligned with the google style guide as well:
https://google.github.io/styleguide/cppguide.html#Friends



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I32e2775f80da387b0df4195fe2c5435b3f8e585e
Gerrit-Change-Number: 8490
Gerrit-PatchSet: 14
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 14:06:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6319: Fix alloc/free mismatch.

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8838 )

Change subject: IMPALA-6319: Fix alloc/free mismatch.
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia695201e61d8afc23636f826264635c85d3a228a
Gerrit-Change-Number: 8838
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Thu, 14 Dec 2017 13:58:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

[Zoltan Borok-Nagy (Code Review)]

Zoltan Borok-Nagy has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/8621 )

Change subject: IMPALA-3703: Store query context in thread-local variables
..


Patch Set 10:

(11 comments)

Thank you very much!

http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@11
PS10, Line 11: in debug sessions. It needs to be allocated on the stack in
> mention "on the stack of each thread"
Done


http://gerrit.cloudera.org:8080/#/c/8621/10//COMMIT_MSG@22
PS10, Line 22: fully-fledged core
> Replace with "core dump written by Impala"?
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc
File be/src/common/thread-debug-info-test.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@20
PS10, Line 20: #include 
> Still needed?
No, removed it and others as well.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info-test.cc@30
PS10, Line 30: using boost::split;
> still needed?
No, removed it.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h
File be/src/common/thread-debug-info.h:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@83
PS10, Line 83:   void PrintUniqueIdToMember(const TUniqueId& id, char* member) {
> We could inline this now as it's only used once.
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@90
PS10, Line 90:   static void InitializeThreadDebugInfo(ThreadDebugInfo* 
thread_debug_info);
> I think these could benefit from a brief comment since they're defined in a
Added short comments about them.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/common/thread-debug-info.h@96
PS10, Line 96:   static constexpr int64_t THREAD_NAME_FRONT_LENGTH = 
THREAD_NAME_SIZE -
> I think moving this computation to SetThreadName() would make it slightly e
I removed THREAD_NAME_FRONT_LENGTH.
Should I remove THREAD_NAME_TAIL_LENGTH as well?


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

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/blocking-join-node.cc@199
PS10, Line 199:   ThreadDebugInfo* thread_debug_info = 
GetThreadDebugInfo();
> You could also do GetThreadDebugInfo()->SetInstanceId(state->fragment_insta
yes, previously I set more member (query id).

Done.


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc
File be/src/exec/hdfs-scan-node.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/exec/hdfs-scan-node.cc@353
PS10, Line 353:   ThreadDebugInfo* thread_debug_info = GetThreadDebugInfo();
> You could shorten it a bit to GetThreadDebugInfo()->SetInstanceId(state->fr
Done


http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc
File be/src/runtime/fragment-instance-state.cc:

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/fragment-instance-state.cc@232
PS10, Line 232:   ThreadDebugInfo* thread_debug_info = 
GetThreadDebugInfo();
  :   thread_debug_info->SetInstanceId(this->instance_id());
> You could shorten this to GetThreadDebugInfo()->SetInstanceId(this->instanc
Done


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

http://gerrit.cloudera.org:8080/#/c/8621/10/be/src/runtime/query-state.cc@377
PS10, Line 377: thread_debug_info
> Inline the call to GetThreadDebugInfo() here, too?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 10
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Thu, 14 Dec 2017 13:42:43 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3703: Store query context in thread-local variables

[Zoltan Borok-Nagy (Code Review)]

Hello Lars Volker, Gabor Kaszab, Philip Zeyliger, Tim Armstrong, Dan Hecht,

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

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

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

Change subject: IMPALA-3703: Store query context in thread-local variables
..

IMPALA-3703: Store query context in thread-local variables

This commit introduces the ThreadDebugInfo class which can
hold information about the current thread that can be useful
in debug sessions. It needs to be allocated on the stack of
each thread in order to include it to minidumps.

Currently a ThreadDebugInfo object is created in
Thread::SuperviseThread. This object is available in all
the child stack frames through the global function
GetThreadDebugInfo().

ThreadDebugInfo has members for the thread name and instance
id. These are fixed size char buffers.

If you have a core dump written by Impala, you can locate the
ThreadDebugInfo for the current thread through the global
pointer impala::thread_debug_info.

In a core file that has been created from a minidump, we need
to select the stack frame that allocated the ThreadDebugInfo
object in order to inspect it. It is currently allocated in
Thread::SuperviseThread().

We can use printf in gdb to print the members, e.g.:
printf "%s\n" thread_debug_info.instance_id

Currently the thread name and instance id is stored.

I created some tests in thread-debug-info-test.cc

Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
---
M be/src/common/CMakeLists.txt
A be/src/common/thread-debug-info-test.cc
A be/src/common/thread-debug-info.cc
A be/src/common/thread-debug-info.h
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/query-state.cc
M be/src/util/thread.cc
9 files changed, 252 insertions(+), 2 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/21/8621/11
--
To view, visit http://gerrit.cloudera.org:8080/8621
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I566f7f1db5117c498e86e0bd05b33bdcff624609
Gerrit-Change-Number: 8621
Gerrit-PatchSet: 11
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Zoltan Borok-Nagy