[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@879
PS10, Line 879: Any values that do not fit within the new 
precision and scale are returned as
> Taras, is this really true? I would have expected that we round or error.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 04:44:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@50
PS10, Line 50:   This data type is suitable for financial and other 
arithmetic calculations where the
> We should have a better introduction. We would not be comparing it to FLOAT
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@56
PS10, Line 56:   The key differences between this version of 
DECIMAL and the previous
> We should move this table to the bottom. Instead of calling it DECIMAL_V1 a
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@293
PS10, Line 293:   If the precision of the result would be greater than 38, 
Impala truncates the result from
> Done
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@293
PS10, Line 293:   If the precision of the result would be greater than 38, 
Impala truncates the result from
> yes, "truncates and rounds"
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@668
PS10, Line 668:   expressions to DECIMAL as long as the 
overall number of digits and digits
> Done
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 04:43:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

Hello Greg Rahn, Taras Bobrovytsky, Alex Behm, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..

IMPALA-6522: [DOCS] Document Decimal V2

Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Cherry-picks: not for 2.x.
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_decimal.xml
2 files changed, 872 insertions(+), 708 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 12
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..


Patch Set 6: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 28 Apr 2018 04:30:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..

IMPALA-6920: fix inconsistencies with scanner thread tokens

The first scanner thread to start now takes a "required" token,
which always succeeds. Only additional threads try to get
"optional" tokens, which can fail. Previously threads always
requested optional tokens, which could fail and leave the scan
node without any running threads until its callback is invoked.

This allows us to remove the "reserved optional token" and
set_max_quota() interfaces from ThreadResourceManager. There should
be no behavioural changes in ThreadResourceMgr in cases when those
features are not used.

Also switch Kudu to using the same logic for implementing
NUM_SCANNER_THREADS (it was not switched over to the improved
HDFS scanner logic added in IMPALA-2831).

Do some cleanup in ThreadResourceMgr code while we're here:
* Fix some benign data races in ThreadResourceMgr by switching to
  AtomicInt* classes.
* Remove pointless object caching (TCMalloc will do better).
* Reduce dependencies on the thread-resource-mgr.h header.

Testing:
Ran core tests.

Ran a few queries under TSAN, checked that it didn't report any more
races in this code after fixing those data races.

I couldn't construct a regression test because there are no easily
testable consequences of the change - the main difference is that
some scanner threads start earlier when there is pressure on scanner
thread tokens but that is hard to construct a robust test around.

Change-Id: I16d31d72441aff7293759281d0248e641df43704
Reviewed-on: http://gerrit.cloudera.org:8080/10186
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins 
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/thread-resource-mgr-test.cc
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/thread-resource-mgr.h
13 files changed, 290 insertions(+), 337 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 7
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are 
compatible in FE
..


Patch Set 10: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 10
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 03:33:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are 
compatible in FE
..

IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

In this patch we implement strict decimal type checking in the FE in
various situations when DECIMAL_V2 is enabled. What is affected:
- Union. If we union two decimals and it is not possible to come up
  with a decimal that will be able to contain all the digits, an error
  is thrown. For example, the union(decimal(20, 10), decimal(20, 20))
  returns decimal(30, 20). However, for union(decimal(38, 0),
  decimal(38, 38)) the ideal return type would be decimal(76,38), but
  this is too large, so an error is thrown.
- Insert. If we are inserting a decimal value into a column where we are
  not guaranteed that all digits will fit, an error is thrown. For
  example, inserting a decimal(38,0) value into a decimal(38,38) column.
- Functions such as coalesce(). If we are unable to determine the output
  type that guarantees that all digits will fit from all the arguments,
  an error is thrown. For example,
  coalesce(decimal(38,38), decimal(38,0)) will throw an error.
- Hash Join. When joining on two decimals, if a type cannot be
  determined that both columns can be cast to, we throw an error.
  For example, join on decimal(38,0) and decimal(38,38) will result
  in an error.

To avoid these errors, you need to use CAST() on some of the decimals.

In this patch we also change the output decimal calculation of decimal
round, truncate and related functions. If these functions are a no-op,
the resulting decimal type is the same as the input type.

Testing:
- Ran a core build which passed.

Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Reviewed-on: http://gerrit.cloudera.org:8080/9930
Reviewed-by: Alex Behm 
Tested-by: Impala Public Jenkins 
---
M be/src/exprs/expr-test.cc
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/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.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/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/InsertStmt.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/ModifyStmt.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/StatementBase.java
M fe/src/main/java/org/apache/impala/analysis/TimestampArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/TypesUtil.java
M fe/src/main/java/org/apache/impala/catalog/Function.java
M fe/src/main/java/org/apache/impala/catalog/ScalarType.java
M fe/src/main/java/org/apache/impala/catalog/Type.java
M fe/src/main/java/org/apache/impala/planner/HashJoinNode.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/TypesUtilTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/complex-types-file-formats.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/mt-dop-validation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-loop-join.test
M testdata/workloads/functional-query/queries/QueryTest/decimal-exprs.test
M testdata/workloads/functional-query/queries/QueryTest/decimal.test
34 files changed, 550 insertions(+), 292 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 11
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-5392: Added all stack frames to ThreadInfo summary.

[Abhishek Sharma (Code Review)]

Abhishek Sharma has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10145 )

Change subject: IMPALA-5392: Added all stack frames to ThreadInfo summary.
..


Patch Set 8:

Tested the CatalogUI page with the new changes.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I80ab4aad03e0c1f01fecad6b87779531244c28b7
Gerrit-Change-Number: 10145
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Sharma 
Gerrit-Reviewer: Abhishek Sharma 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Charles Agnello 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Jim Apple 
Gerrit-Comment-Date: Sat, 28 Apr 2018 02:39:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] Warn about Hadoop / Java version incompatibility

[Impala Public Jenkins (Code Review)]

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

Change subject: Warn about Hadoop / Java version incompatibility
..

Warn about Hadoop / Java version incompatibility

Running Hadoop 3 with Java 7 can result in some obscure error messages.
This change adds a warning to impala-config.sh when using Hadoop 3 with
Java 7.

   Your development environment is configured for Hadoop 3 and Java 7.
   Hadoop 3 requires at least Java 8. Your JAVA binary currently points
   to /usr/lib/jvm/java-7-oracle-amd64/bin/java and reports the
   following version:

   java version "1.7.0_75"
   Java(TM) SE Runtime Environment (build 1.7.0_75-b13)
   Java HotSpot(TM) 64-Bit Server VM (build 24.75-b04, mixed mode)

Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Reviewed-on: http://gerrit.cloudera.org:8080/10222
Reviewed-by: Joe McDonnell 
Tested-by: Impala Public Jenkins 
---
M bin/impala-config.sh
1 file changed, 15 insertions(+), 0 deletions(-)

Approvals:
  Joe McDonnell: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Gerrit-Change-Number: 10222
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] Warn about Hadoop / Java version incompatibility

[Impala Public Jenkins (Code Review)]

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

Change subject: Warn about Hadoop / Java version incompatibility
..


Patch Set 6: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Gerrit-Change-Number: 10222
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Sat, 28 Apr 2018 01:55:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@280
PS10, Line 280:   Precision and scale in arithmetic operations and 
UNION:
> I think we should have a separate section for decimal assignments where the
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@293
PS10, Line 293:   If the precision of the result would be greater than 38, 
Impala truncates the result from
> truncates and rounds, right Taras?
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@544
PS10, Line 544:   DECIMAL cannot be implicitly 
converted to DECIMAL if
> I'd phrase this positively, i.e. describe in which cases the conversion is
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@668
PS10, Line 668:   expressions to DECIMAL as long as the 
overall number of digits and digits
> Sentence seem wrong. We follow same general procedure here, round from the
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 01:16:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

Hello Greg Rahn, Taras Bobrovytsky, Alex Behm, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..

IMPALA-6522: [DOCS] Document Decimal V2

Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Cherry-picks: not for 2.x.
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_decimal.xml
2 files changed, 864 insertions(+), 708 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 11
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

Alex and Tara, Please review the changes in the latest patch. Thank you!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 01:16:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Taras Bobrovytsky (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@50
PS10, Line 50:   This data type is suitable for financial and other 
arithmetic calculations where the
We should have a better introduction. We would not be comparing it to FLOAT. We 
should not mention that it's suitable for financial purposes.

How about this:
Precision is the maximum number of decimal digits and scale is the number of 
digits to the right of the decimal point. The data type is useful for storing 
and doing operations on precise decimal values.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@56
PS10, Line 56:   The key differences between this version of 
DECIMAL and the previous
We should move this table to the bottom. Instead of calling it DECIMAL_V1 and 
DECIMAL_v2 we should say "Decimal in Impala 2.x" and "Decimal in Impala 3.x". 
Say that the old behavior can be enabled by disabling the query option "set 
decimal_v2=false"


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@293
PS10, Line 293:   If the precision of the result would be greater than 38, 
Impala truncates the result from
> truncates and rounds, right Taras?
yes, "truncates and rounds"

Let's make a separate section called "Decimal Assignment" as Alex mentioned and 
put UNION in that section.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@668
PS10, Line 668:   expressions to DECIMAL as long as the 
overall number of digits and digits
> Taras?
Alex is right. This contradicts what it says on line 678.

should be "... as number fits within the specified target DECIMAL type without 
overflow"


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@879
PS10, Line 879: Any values that do not fit within the new 
precision and scale are returned as
> Taras, is this really true? I would have expected that we round or error.
No, we do not round. We will error if the query option ABORT_ON_ERROR is set to 
true. If that option is set to FALSE, we will get a NULL and warning that 
conversion failed.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 01:09:13 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..


Patch Set 6: Code-Review+2

carry


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 28 Apr 2018 00:46:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..


Patch Set 6:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 28 Apr 2018 00:46:13 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Tim Armstrong (Code Review)]

Hello Csaba Ringhofer, Dan Hecht,

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

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

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..

IMPALA-6920: fix inconsistencies with scanner thread tokens

The first scanner thread to start now takes a "required" token,
which always succeeds. Only additional threads try to get
"optional" tokens, which can fail. Previously threads always
requested optional tokens, which could fail and leave the scan
node without any running threads until its callback is invoked.

This allows us to remove the "reserved optional token" and
set_max_quota() interfaces from ThreadResourceManager. There should
be no behavioural changes in ThreadResourceMgr in cases when those
features are not used.

Also switch Kudu to using the same logic for implementing
NUM_SCANNER_THREADS (it was not switched over to the improved
HDFS scanner logic added in IMPALA-2831).

Do some cleanup in ThreadResourceMgr code while we're here:
* Fix some benign data races in ThreadResourceMgr by switching to
  AtomicInt* classes.
* Remove pointless object caching (TCMalloc will do better).
* Reduce dependencies on the thread-resource-mgr.h header.

Testing:
Ran core tests.

Ran a few queries under TSAN, checked that it didn't report any more
races in this code after fixing those data races.

I couldn't construct a regression test because there are no easily
testable consequences of the change - the main difference is that
some scanner threads start earlier when there is pressure on scanner
thread tokens but that is hard to construct a robust test around.

Change-Id: I16d31d72441aff7293759281d0248e641df43704
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/thread-resource-mgr-test.cc
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/thread-resource-mgr.h
13 files changed, 290 insertions(+), 337 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@271
PS10, Line 271:   Note that in memory and on disk for binary file formats 
such as Parquet or Avro,
> This seems to contradict what L262 says which claims that binary formats wi
This is a note about decimal(9) and decimal (10) - the difference in storage 
bytes.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@284
PS10, Line 284:   Unlike other numeric types, memory footprint of 
DECIMAL can increase as a
> I don't think this is correct or even needed.
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@289
PS10, Line 289:   For all arithmetic options, the resulting precision is 
maximum of 38.
> is at most 38
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@523
PS10, Line 523:   FLOAT when necessary even with a 
loss of precision. It can be
> loss of precision -> loss of information
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@545
PS10, Line 545:
> Add this:
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@613
PS10, Line 613:   DOUBLE), Impala raises an error and does 
not convert the value.
> does not implicitly convert the value.
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@618
PS10, Line 618:   DECIMAL and other types in 
INSERT statements:
> in decimal assignments like in INSERT and UNION statements or functions lik
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@668
PS10, Line 668:   expressions to DECIMAL as long as the 
overall number of digits and digits
> Sentence seem wrong. We follow same general procedure here, round from the
Taras?


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@690
PS10, Line 690: DECIMAL, an error returns.
> an error is returned
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@710
PS10, Line 710:   Casting any non-numeric value, such as 
'ABC', or NULL to
> Casting NULL to a decimal is allowed.
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@724
PS10, Line 724:   DECIMAL vs FLOAT or DOUBLE consideration:
> DECIMAL vs. FLOAT considerations:
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@728
PS10, Line 728:   The FLOAT and DOUBLE 
type numbers can cause problems or
> type numbers -> types
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@732
PS10, Line 732:   values for GROUP BY columns. The 
DECIMAL values can help
> The DECIMAL type
Done


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@934
PS10, Line 934:   Because DECIMAL type has a fixed size, 
the maximum and average size
> the DECIMAL type
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Sat, 28 Apr 2018 00:04:31 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Dan Hecht (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..


Patch Set 4: Code-Review+2

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10186/4/be/src/runtime/thread-resource-mgr.h
File be/src/runtime/thread-resource-mgr.h:

http://gerrit.cloudera.org:8080/#/c/10186/4/be/src/runtime/thread-resource-mgr.h@34
PS4, Line 34: Conceptually, there is a fixed pool of threads that are shared 
between
: /// query fragments.  If there is only one fragment running, it 
can use the
: /// entire pool, spinning up the maximum number of threads to 
saturate the
: /// hardware.  If there are multiple fragments, the CPU pool must 
be shared
: /// between them.
This seems to be using the term "pool" to mean something different than 
ThreadResourcePool.  is there an easy reword to make it clearer what 
ThreadResourcePool is? (seems to be the "sub" pool for a fragment instance?)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Apr 2018 23:52:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are 
compatible in FE
..


Patch Set 10:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 10
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 23:35:35 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Behm (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(18 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@271
PS10, Line 271:   Note that in memory and on disk for binary file formats 
such as Parquet or Avro,
This seems to contradict what L262 says which claims that binary formats will 
use fewer bytes.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@280
PS10, Line 280:   Precision and scale in arithmetic operations and 
UNION:
I think we should have a separate section for decimal assignments where the 
implicit decimal->decimal conversions are strict.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@284
PS10, Line 284:   Unlike other numeric types, memory footprint of 
DECIMAL can increase as a
I don't think this is correct or even needed.

If we add two INTs we get a BIGINT, so that also increases the memory footprint.

I suggest removing this paragraph.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@289
PS10, Line 289:   For all arithmetic options, the resulting precision is 
maximum of 38.
is at most 38


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@293
PS10, Line 293:   If the precision of the result would be greater than 38, 
Impala truncates the result from
truncates and rounds, right Taras?

This is also not quite right for UNION, so I think it's better to have a 
separate section for decimal assignment.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@512
PS10, Line 512:   Impala enforces strict conversion rules in 
INSERT statements or
This section seems to be for "decimal assignment" like I mentioned above.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@523
PS10, Line 523:   FLOAT when necessary even with a 
loss of precision. It can be
loss of precision -> loss of information


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@544
PS10, Line 544:   DECIMAL cannot be implicitly 
converted to DECIMAL if
I'd phrase this positively, i.e. describe in which cases the conversion is 
allowed.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@613
PS10, Line 613:   DOUBLE), Impala raises an error and does 
not convert the value.
does not implicitly convert the value.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@618
PS10, Line 618:   DECIMAL and other types in 
INSERT statements:
in decimal assignments like in INSERT and UNION statements or functions like 
COALESCE()


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@668
PS10, Line 668:   expressions to DECIMAL as long as the 
overall number of digits and digits
Sentence seem wrong. We follow same general procedure here, round from the back 
or error if there are not enough digits before the decimal point.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@690
PS10, Line 690: DECIMAL, an error returns.
an error is returned


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@710
PS10, Line 710:   Casting any non-numeric value, such as 
'ABC', or NULL to
Casting NULL to a decimal is allowed.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@724
PS10, Line 724:   DECIMAL vs FLOAT or DOUBLE consideration:
DECIMAL vs. FLOAT considerations:


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@728
PS10, Line 728:   The FLOAT and DOUBLE 
type numbers can cause problems or
type numbers -> types


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@732
PS10, Line 732:   values for GROUP BY columns. The 
DECIMAL values can help
The DECIMAL type


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@879
PS10, Line 879: Any values that do not fit within the new 
precision and scale are returned as
Taras, is this really true? I would have expected that we round or error.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@934
PS10, Line 934:   Because DECIMAL type has a fixed size, 
the maximum and average size
the DECIMAL type



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 

[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are 
compatible in FE
..


Patch Set 10: Verified-1

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 10
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 23:32:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3833: Fix invalid data handling in Sequence and RCFile scanners

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-3833: Fix invalid data handling in Sequence and RCFile 
scanners
..


Patch Set 15:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc
File be/src/exec/hdfs-rcfile-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc@91
PS15, Line 91: excceeded
typo: exceeded


http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc@91
PS15, Line 91:  8 million limit"
Let's use the constant instead of hardcoding it in the error message so that 
they can't get out of sync.


http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc@545
PS15, Line 545: if (max_tuples < 0) {
It seems like this is catching the problem too late. How does it get into this 
invalid state? The problem is in our own code so we shoudl fix it at the source 
rather than trying to catch it here.


http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/exec/hdfs-rcfile-scanner.cc@588
PS15, Line 588: if (col_end > row_group_end || column.start_offset < 0 
|| column.buffer_pos < 0
I think we should catch the invalid column metadata when we read it in 
GetCurrentKeyBuffer(), rather than letting things get into an invalid state and 
trying to catch it later.


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

http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/runtime/timestamp-parse-util.cc@370
PS15, Line 370:   if (UNLIKELY(str == NULL || len <= 0)) {
We're already checking len here but the problem is that we're trimming 
whitespace below. I think the right fix is to check if (len <= 0) immediately 
after stripping leading and trailing whitespace. That will be less error-prone 
than trying to catch it later


http://gerrit.cloudera.org:8080/#/c/8936/15/be/src/runtime/timestamp-parse-util.cc@477
PS15, Line 477: if (lazy_len > 0 && ParseFormatTokensByStr(_ctx)) {
Looks like you hit upon a different bug here - I can trigger that DCHECK with 
another query -

  select cast(' ' as timestamp);

I don't think it should affect release builds, but can you file a separate bug 
to fix that? I think we should fix that separately and add a specific 
regression text.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic9cfc38af3f30c65ada9734eb471dbfa6ecdd74a
Gerrit-Change-Number: 8936
Gerrit-PatchSet: 15
Gerrit-Owner: Pranay Singh
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Pranay Singh
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 27 Apr 2018 23:27:44 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6916: Implement COMMENT ON DATABASE

[Fredy Wijaya (Code Review)]

Fredy Wijaya has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10171 )

Change subject: IMPALA-6916: Implement COMMENT ON DATABASE
..


Patch Set 6:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/10171/5/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

http://gerrit.cloudera.org:8080/#/c/10171/5/common/thrift/JniCatalog.thrift@634
PS5, Line 634: struct TCommentOnParams {
> Is this enum needed? Can't we determine which comment we should update base
Done


http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java
File fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java:

http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java@30
PS5, Line 30:   }
> In Postgres you can remove a comment with:
Done. I updated the grammar to allow NULL, too.


http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3524
PS5, Line 3524: synchronized (metastoreDdlLock_) {
> I don't think this is a precondition. The database could have legitimately
Done. I'll throw an exception instead.


http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3528
PS5, Line 3528:   addSummary(response, "Updated database.");
> You need to increment and set the version of the db, not only the thrift re
Done. Yeah I tested with multiple impalads.


http://gerrit.cloudera.org:8080/#/c/10171/5/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

http://gerrit.cloudera.org:8080/#/c/10171/5/tests/metadata/test_ddl.py@197
PS5, Line 197:   def test_comment_on_database(self, vector, unique_database):
> test_comment_on_database
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcf909c18f97073346f6f603538bf921e69fbb00
Gerrit-Change-Number: 10171
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Fri, 27 Apr 2018 22:28:26 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6916: Implement COMMENT ON DATABASE

[Fredy Wijaya (Code Review)]

Fredy Wijaya has uploaded a new patch set (#6). ( 
http://gerrit.cloudera.org:8080/10171 )

Change subject: IMPALA-6916: Implement COMMENT ON DATABASE
..

IMPALA-6916: Implement COMMENT ON DATABASE

This patch implements updating comment on a database.

Syntax:
COMMENT ON DATABASE db IS 'comment'

Testing:
- Added new front-end tests
- Ran all front-end tests
- Added new end-to-end tests
- Ran end-to-end DDL tests

Change-Id: Ifcf909c18f97073346f6f603538bf921e69fbb00
---
M common/thrift/CatalogService.thrift
M common/thrift/JniCatalog.thrift
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
A fe/src/main/java/org/apache/impala/analysis/CommentOnDbStmt.java
A fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M tests/metadata/test_ddl.py
M tests/metadata/test_ddl_base.py
13 files changed, 217 insertions(+), 5 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifcf909c18f97073346f6f603538bf921e69fbb00
Gerrit-Change-Number: 10171
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@539
PS10, Line 539: OUBLE
> DOUBLE and FLOAT
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 22:14:48 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Speed up Python dependencies.

[Taras Bobrovytsky (Code Review)]

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

Change subject: Speed up Python dependencies.
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py@172
PS2, Line 172: if results:
> If the import above fails, then results = [] will never execute and this st
Nice catch. We can get rid of the "if results:" line



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 22:03:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Taras Bobrovytsky (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 10:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@539
PS10, Line 539: OUBLE
DOUBLE and FLOAT


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@540
PS10, Line 540: error returns.
Maybe add the following.

The reasoning is that the range of the floating point types is much wider than 
the range of the DECIMAL, so not every FLOAT or DOUBLE can be converted to a 
DECIMAL. However, every DECIMAL can be converted to a double or float with a 
loss of precision, which is why we allow implicit casting in this case. This is 
somewhat dangerous, but we made this design choice to match the behavior of 
other SQL engines.


http://gerrit.cloudera.org:8080/#/c/10066/10/docs/topics/impala_decimal.xml@545
PS10, Line 545:
Add this:
We do not allow this because calculations involving decimals are meant to be 
precise, so we are strict and require explicit casts. However, if an 
approximate type like FLOAT is involved, then our behavior is less strict.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 21:59:45 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6821: Push down limits into Kudu

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6821: Push down limits into Kudu
..


Patch Set 6: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe35e70065d8706b575e24fe20902cd405b49941
Gerrit-Change-Number: 10119
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 27 Apr 2018 21:55:10 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6821: Push down limits into Kudu

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6821: Push down limits into Kudu
..

IMPALA-6821: Push down limits into Kudu

This patch takes advantage of a recent change in Kudu (KUDU-16) that
exposes the ability to set limits on KuduScanners. Since each
KuduScanner corresponds to a scan token, and there will be multiple
scan tokens per query, this is just a performance optimization in
cases where the limit is smaller than the number of rows per token,
and Impala still needs to apply the limit on our side for cases where
the limit is greater than the number of rows per token.

Testing:
- Added e2e tests for various situations where limits are applied at
  a Kudu scan node.
- For the query 'select * from tpch_kudu.lineitem limit 1', a best
  case perf scenario for this change where the limit is highly
  effective, the time spent in the Kudu scan node was reduced from
  6.107ms to 3.498ms (avg over 3 runs).
- For the query 'select count(*) from (select * from
  tpch_kudu.lineitem limit 100) v', a worst case perf scenario for
  this change where the limit is ineffective, the time spent in the
  Kudu scan node was essentially unchanged, 32.815ms previously vs.
  29.532ms (avg over 3 runs).

Change-Id: Ibe35e70065d8706b575e24fe20902cd405b49941
Reviewed-on: http://gerrit.cloudera.org:8080/10119
Reviewed-by: Thomas Tauber-Marshall 
Tested-by: Impala Public Jenkins 
---
M be/src/exec/kudu-scanner.cc
A testdata/workloads/functional-query/queries/QueryTest/kudu_limit.test
M tests/query_test/test_kudu.py
3 files changed, 120 insertions(+), 0 deletions(-)

Approvals:
  Thomas Tauber-Marshall: Looks good to me, approved
  Impala Public Jenkins: Verified

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Ibe35e70065d8706b575e24fe20902cd405b49941
Gerrit-Change-Number: 10119
Gerrit-PatchSet: 7
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] Warn about Hadoop / Java version incompatibility

[Impala Public Jenkins (Code Review)]

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

Change subject: Warn about Hadoop / Java version incompatibility
..


Patch Set 6:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Gerrit-Change-Number: 10222
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 27 Apr 2018 21:45:39 +
Gerrit-HasComments: No

[Impala-ASF-CR] Speed up Python dependencies.

[Lars Volker (Code Review)]

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

Change subject: Speed up Python dependencies.
..


Patch Set 2: Code-Review-1

(3 comments)

I think this has a bug when multiprocessing fails to import.

http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py@137
PS2, Line 137: results = []
Hoist this out of the loop (see my comment below).


http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py@139
PS2, Line 139: # Multiprocessing was introduced in Python2.6; fail 
gracefully if
If it was introduced in 2.6, can we not rely on it (I thought that was the 
earliest Python version we support).


http://gerrit.cloudera.org:8080/#/c/10234/2/infra/python/deps/pip_download.py@172
PS2, Line 172: if results:
If the import above fails, then results = [] will never execute and this 
statement will fail (unless I'm missing something). Moving it out of the loop 
above also allows to get rid of the "if ".



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 21:44:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Speed up Python dependencies.

[Taras Bobrovytsky (Code Review)]

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

Change subject: Speed up Python dependencies.
..


Patch Set 2: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 21:34:17 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5392: Added all stack frames to ThreadInfo summary.

[Abhishek Sharma (Code Review)]

Abhishek Sharma has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/10145 )

Change subject: IMPALA-5392: Added all stack frames to ThreadInfo summary.
..

IMPALA-5392: Added all stack frames to ThreadInfo summary.

The current implementation uses ThreadInfo.toString,
which restricts the number of stack frames to 8.
As a part of this fix, this particular constraint is removed.
Now all stack frames are included in the summary.

Change-Id: I80ab4aad03e0c1f01fecad6b87779531244c28b7
---
M fe/src/main/java/org/apache/impala/common/JniUtil.java
1 file changed, 87 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I80ab4aad03e0c1f01fecad6b87779531244c28b7
Gerrit-Change-Number: 10145
Gerrit-PatchSet: 8
Gerrit-Owner: Abhishek Sharma 
Gerrit-Reviewer: Abhishek Sharma 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Charles Agnello 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] Speed up Python dependencies.

[Philip Zeyliger (Code Review)]

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

Change subject: Speed up Python dependencies.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@165
PS1, Line 165: results.append(pool.apply_async(download_package, 
args=[pkg_name.strip(), pkg_version.strip()]))
> You can use "git diff HEAD^ | impala-flake8 --diff" and similar patterns to
Done


http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@173
PS1, Line 173: x.get()
> What happens if the download fails for some reason? Will this be detected?
Yes, it works correctly, I believe.

>>> from multiprocessing.pool import ThreadPool
>>> p = ThreadPool()
>>> def f(a):
...   raise Exception("hey")
...
>>> x = p.apply_async(f, [1])
>>> x.get()
Traceback (most recent call last):
  File "", line 1, in 
  File "/Users/philip/anaconda/lib/python2.7/multiprocessing/pool.py", line 
567, in get
raise self._value
Exception: hey



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 20:17:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Speed up Python dependencies.

[Philip Zeyliger (Code Review)]

Hello Taras Bobrovytsky, Michael Brown, Joe McDonnell,

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

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

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

Change subject: Speed up Python dependencies.
..

Speed up Python dependencies.

This parallelizes downloading some Python libraries, giving a speedup of
$IMPALA_HOME/infra/python/deps/download_requirements.  I've seen this
take from 7-15 seconds before and from 2-5 seconds after.

Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
---
M infra/python/deps/pip_download.py
1 file changed, 19 insertions(+), 1 deletion(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 2
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] Speed up Python dependencies.

[Michael Brown (Code Review)]

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

Change subject: Speed up Python dependencies.
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@165
PS1, Line 165: results.append(pool.apply_async(download_package, 
args=[pkg_name.strip(), pkg_version.strip()]))
> long line
You can use "git diff HEAD^ | impala-flake8 --diff" and similar patterns to 
find problems like this.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 20:12:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

Hello Greg Rahn, Taras Bobrovytsky, Alex Behm, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..

IMPALA-6522: [DOCS] Document Decimal V2

Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Cherry-picks: not for 2.x.
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_decimal.xml
2 files changed, 851 insertions(+), 708 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 10
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@526
PS9, Line 526: 38,0
> We should be consistent about putting a space after the comma. Either put i
Done


http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@654
PS9, Line 654: is the INT
 : type with the precision 10.
> ... because all digits do not fit into DECIMAL(3,0)
Done


http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@681
PS9, Line 681:
> There should be no space before the open brace. Here and elsewhere.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 9
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 20:07:34 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6314: Add run time scalar subquery check for 
uncorrelated subqueries
..

IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries

If a scalar subquery is used with a binary predicate,
or, used in an arithmetic expression, it must return
only one row/column to be valid. If this cannot be
guaranteed at parse time through a single row aggregate
or limit clause, Impala fails the query like such.

E.g., currently the following query is not allowed:
SELECT bigint_col
FROM alltypesagg
WHERE id = (SELECT id FROM alltypesagg WHERE id = 1)

However, it would be allowed if the query contained
a LIMIT 1 clause, or instead of id it was max(id).

This commit makes the example valid by introducing a
runtime check to test if the subquery returns a single
row. If the subquery returns more than one row, it
aborts the query with an error.

I added a new node type, called CardinalityCheckNode. It
is created during planning on top of the subquery when
needed, then during execution it checks if its child
only returns a single row.

I extended the frontend tests and e2e tests as well.

Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Reviewed-on: http://gerrit.cloudera.org:8080/9005
Reviewed-by: Alex Behm 
Tested-by: Impala Public Jenkins 
---
M be/src/exec/CMakeLists.txt
A be/src/exec/cardinality-check-node.cc
A be/src/exec/cardinality-check-node.h
M be/src/exec/exec-node.cc
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/HdfsCachingOp.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.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/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
A fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
M 
testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test
M testdata/workloads/functional-query/queries/QueryTest/subquery.test
28 files changed, 1,130 insertions(+), 79 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Gerrit-Change-Number: 9005
Gerrit-PatchSet: 32
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6314: Add run time scalar subquery check for 
uncorrelated subqueries
..


Patch Set 31: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Gerrit-Change-Number: 9005
Gerrit-PatchSet: 31
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 27 Apr 2018 20:06:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] Speed up Python dependencies.

[Taras Bobrovytsky (Code Review)]

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

Change subject: Speed up Python dependencies.
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py
File infra/python/deps/pip_download.py:

http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@165
PS1, Line 165: results.append(pool.apply_async(download_package, 
args=[pkg_name.strip(), pkg_version.strip()]))
long line


http://gerrit.cloudera.org:8080/#/c/10234/1/infra/python/deps/pip_download.py@173
PS1, Line 173: x.get()
What happens if the download fails for some reason? Will this be detected?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
Gerrit-Change-Number: 10234
Gerrit-PatchSet: 1
Gerrit-Owner: Philip Zeyliger 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 20:03:56 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6340,IMPALA-6518: Check that decimal types are compatible in FE

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6340,IMPALA-6518: Check that decimal types are 
compatible in FE
..


Patch Set 10:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id406f4189e01a909152985fabd5cca7a1527a568
Gerrit-Change-Number: 9930
Gerrit-PatchSet: 10
Gerrit-Owner: Taras Bobrovytsky 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:58:54 +
Gerrit-HasComments: No

[Impala-ASF-CR] Speed up Python dependencies.

[Philip Zeyliger (Code Review)]

Philip Zeyliger has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/10234


Change subject: Speed up Python dependencies.
..

Speed up Python dependencies.

This parallelizes downloading some Python libraries, giving a speedup of
$IMPALA_HOME/infra/python/deps/download_requirements.  I've seen this
take from 7-15 seconds before and from 2-5 seconds after.

Change-Id: I7cbf622adb7d037f1a53c519402dcd8ae3c0fe30
---
M infra/python/deps/pip_download.py
1 file changed, 18 insertions(+), 1 deletion(-)



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

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

[Impala-ASF-CR](2.x) IMPALA-6892: CheckHashAndDecrypt() includes file and host

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6892: CheckHashAndDecrypt() includes file and host
..

IMPALA-6892: CheckHashAndDecrypt() includes file and host

The error text with AES-GCM enabled looks like:

  Error reading 44 bytes from scratch file
  '/tmp/impala-scratch/0:0_d43635d0-8f55-485e-8899-907af289ac86' on
  backend tarmstrong-box:22000 at offset 0: verification of read data
  failed.
  OpenSSL error in EVP_DecryptFinal:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:

Testing:
Added a backend test to exercise the code path and verify the error
code.

Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Reviewed-on: http://gerrit.cloudera.org:8080/10204
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins 
Reviewed-on: http://gerrit.cloudera.org:8080/10232
---
M be/src/exec/exec-node.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M common/thrift/generate_error_codes.py
4 files changed, 70 insertions(+), 4 deletions(-)

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

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: merged
Gerrit-Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Gerrit-Change-Number: 10232
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](2.x) IMPALA-6892: CheckHashAndDecrypt() includes file and host

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6892: CheckHashAndDecrypt() includes file and host
..


Patch Set 1: Verified+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Gerrit-Change-Number: 10232
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:47:37 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6131: Track time of last statistics update in metadata

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6131: Track time of last statistics update in metadata
..


Patch Set 4:

(6 comments)

> - At this point every kind of COMPUTE STATS update
 > impala.lastComputeStatsTime, including INCREMENTAL, TABLESAMPLE,
 > and calls that only COMPUTE STATS for specific columns. I can
 > exclude some of these, but I think that the current behavior should
 > cause the "least surprise".
That seems reasonable to me, but please check with Alex.

 > - I think that the tests should be reorganized, but I would like to
 > finalize the behavior before doing that.
Yes. :)

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

http://gerrit.cloudera.org:8080/#/c/10116/4/fe/src/main/java/org/apache/impala/catalog/KuduTable.java@247
PS4, Line 247: msClient.alter_table(msTable_.getDbName(),
nit: single line?


http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/10116/3/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@2857
PS3, Line 2857:* command if the metadata is not completely in-sync. This 
affects both Hive and
> Is the missing field explanation really necessary? I think that the current
lgtm


http://gerrit.cloudera.org:8080/#/c/10116/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/10116/4/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@782
PS4, Line 782: if (params.isSetTable_stats()) updateTableStats(params, 
msTbl);
 :
 : if (params.isSetTable_stats()) {
This could be one check now


http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py
File tests/metadata/test_last_ddl_time_update.py:

http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py@24
PS4, Line 24: # Checks different statements effect the last DDL time and last 
compute stats time.
nit: Checks that...


http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py@50
PS4, Line 50: # compute statistics the fill table property 
impala.lastComputeStatsTime
nit: "to fill the.."?


http://gerrit.cloudera.org:8080/#/c/10116/4/tests/metadata/test_last_ddl_time_update.py@167
PS4, Line 167: False, False
Can you think of ways to make these calls more readable? We could have to 
constants, e.g. DDL=1 and STATS=2 and then pass a list of changed values. These 
would then read

  self.run_test("bla", unique_database, TBL_NAME, [DDL, STATS])

There might be better ways I cannot think of right now.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I59a671ac29d352bd92ce40d5cb6662bb23f146b5
Gerrit-Change-Number: 10116
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:46:15 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Warn about Hadoop / Java version incompatibility

[Joe McDonnell (Code Review)]

Joe McDonnell has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10222 )

Change subject: Warn about Hadoop / Java version incompatibility
..


Patch Set 6: Code-Review+2

Thanks for making this change


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Gerrit-Change-Number: 10222
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:45:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Taras Bobrovytsky (Code Review)]

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..


Patch Set 9:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml
File docs/topics/impala_decimal.xml:

http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@526
PS9, Line 526: 38,0
We should be consistent about putting a space after the comma. Either put it 
everywhere in the DOC or don't put it everywhere.


http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@654
PS9, Line 654: is the INT
 : type with the precision 10.
... because all digits do not fit into DECIMAL(3,0)


http://gerrit.cloudera.org:8080/#/c/10066/9/docs/topics/impala_decimal.xml@681
PS9, Line 681:
There should be no space before the open brace. Here and elsewhere.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 9
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:21:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Warn about Hadoop / Java version incompatibility

[Lars Volker (Code Review)]

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

Change subject: Warn about Hadoop / Java version incompatibility
..


Patch Set 4:

(1 comment)

Updated the commit message in PS5, rebased in PS6. Do you want to have a final 
look?

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

http://gerrit.cloudera.org:8080/#/c/10222/4//COMMIT_MSG@11
PS4, Line 11: This change adds a warning to impala-config.sh when using Hadoop 
3 with
: Java 7.
:
: It also catches failure of the minicluster start and prints an
: additional warning when running with Hadoop 3 and Java 7.
:
:Start of the minicluster failed. If the error looks similar to
:"Unsupported major.minor version 52.0", make sure you are 
running at
:least Java 8.
:Your JAVA binary currently points to
:/usr/lib/jvm/java-7-oracle-amd64/bin/java and reports the 
following
:version:
:
:java version "1.7.0_75"
:Java(TM) SE Runtime Environment (build 1.7.0_75-b13)
:Java HotSpot(TM) 64-Bit Server VM (build 24.75-b04, mixed mode)
> Update this message to reflect removal of minicluster check.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Gerrit-Change-Number: 10222
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 27 Apr 2018 19:11:11 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Warn about Hadoop / Java version incompatibility

[Lars Volker (Code Review)]

Hello Joe McDonnell,

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

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

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

Change subject: Warn about Hadoop / Java version incompatibility
..

Warn about Hadoop / Java version incompatibility

Running Hadoop 3 with Java 7 can result in some obscure error messages.
This change adds a warning to impala-config.sh when using Hadoop 3 with
Java 7.

   Your development environment is configured for Hadoop 3 and Java 7.
   Hadoop 3 requires at least Java 8. Your JAVA binary currently points
   to /usr/lib/jvm/java-7-oracle-amd64/bin/java and reports the
   following version:

   java version "1.7.0_75"
   Java(TM) SE Runtime Environment (build 1.7.0_75-b13)
   Java HotSpot(TM) 64-Bit Server VM (build 24.75-b04, mixed mode)

Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
---
M bin/impala-config.sh
1 file changed, 15 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Gerrit-Change-Number: 10222
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] Warn about Hadoop / Java version incompatibility

[Lars Volker (Code Review)]

Hello Joe McDonnell,

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

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

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

Change subject: Warn about Hadoop / Java version incompatibility
..

Warn about Hadoop / Java version incompatibility

Running Hadoop 3 with Java 7 can result in some obscure error messages.
This change adds a warning to impala-config.sh when using Hadoop 3 with
Java 7.

   Your development environment is configured for Hadoop 3 and Java 7.
   Hadoop 3 requires at least Java 8. Your JAVA binary currently points
   to /usr/lib/jvm/java-7-oracle-amd64/bin/java and reports the
   following version:

   java version "1.7.0_75"
   Java(TM) SE Runtime Environment (build 1.7.0_75-b13)
   Java HotSpot(TM) 64-Bit Server VM (build 24.75-b04, mixed mode)

Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
---
M bin/impala-config.sh
1 file changed, 15 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ib16feb406afec83fd2380308a5d24a2793d246fd
Gerrit-Change-Number: 10222
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Csaba Ringhofer (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..


Patch Set 4: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Apr 2018 18:52:45 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

Hello Greg Rahn, Taras Bobrovytsky, Alex Behm, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..

IMPALA-6522: [DOCS] Document Decimal V2

Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Cherry-picks: not for 2.x.
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_decimal.xml
2 files changed, 850 insertions(+), 708 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 9
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

Hello Greg Rahn, Taras Bobrovytsky, Alex Behm, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..

IMPALA-6522: [DOCS] Document Decimal V2

Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Cherry-picks: not for 2.x.
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_decimal.xml
2 files changed, 845 insertions(+), 708 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 8
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests

[Fredy Wijaya (Code Review)]

Fredy Wijaya has uploaded a new patch set (#8). ( 
http://gerrit.cloudera.org:8080/10135 )

Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..

IMPALA-6802 (part 1): Clean up authorization tests

The first patch of this patch is to introduce a new mechanism of testing
authorization that tests authorization at every hierarchy. This patch
rewrites the authorization tests for select statements.

Testing:
- Added new authorization tests
- Ran all front-end tests

Cherry-picks: not for 2.x

Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
---
M fe/src/test/java/org/apache/impala/analysis/AnalyzerTest.java
A fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
M fe/src/test/java/org/apache/impala/testutil/ImpaladTestCatalog.java
3 files changed, 727 insertions(+), 3 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 

[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests

[Fredy Wijaya (Code Review)]

Fredy Wijaya has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10135 )

Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..


Patch Set 8:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:

http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: .ok(onColumn("functional", "alltypes", new 
String[]{"id", "bool_col",
> In my mind the auth tests should check whether the authorization rules are
Done. I created a separate test in the AnalyzerTest to test solely on different 
SQL variants.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 8
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Fri, 27 Apr 2018 18:19:50 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.h
File be/src/runtime/thread-resource-mgr.h:

http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.h@99
PS3, Line 99:
> nit: extra space - several copy pasted comments contain double spaces, so t
Yeah a lot of old comments have this but we standardised on not doing this. I 
was actually taught to do this myself when I initially learned to type so it 
doesn't look wrong to me.

Fixed the method comments.


http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.h@189
PS3, Line 189: Set to NULL when unregistered.
> Is there a reason behind this? Seems to be an object pool heritage, where R
It's useful to assert that the resource pool was unregistered (it goes along 
with the DCHECK in the destructor).


http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.cc
File be/src/runtime/thread-resource-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.cc@55
PS3, Line 55:   DCHECK(pools_.find(pool.get()) == pools_.end());
> Is this DCHECK still useful after removing object caching?
Yeah it isn't really very useful.


http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.cc@133
PS3, Line 133: not
> nit: double not
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Apr 2018 17:59:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Tim Armstrong (Code Review)]

Hello Csaba Ringhofer,

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

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

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..

IMPALA-6920: fix inconsistencies with scanner thread tokens

The first scanner thread to start now takes a "required" token,
which always succeeds. Only additional threads try to get
"optional" tokens, which can fail. Previously threads always
requested optional tokens, which could fail and leave the scan
node without any running threads until its callback is invoked.

This allows us to remove the "reserved optional token" and
set_max_quota() interfaces from ThreadResourceManager. There should
be no behavioural changes in ThreadResourceMgr in cases when those
features are not used.

Also switch Kudu to using the same logic for implementing
NUM_SCANNER_THREADS (it was not switched over to the improved
HDFS scanner logic added in IMPALA-2831).

Do some cleanup in ThreadResourceMgr code while we're here:
* Fix some benign data races in ThreadResourceMgr by switching to
  AtomicInt* classes.
* Remove pointless object caching (TCMalloc will do better).
* Reduce dependencies on the thread-resource-mgr.h header.

Testing:
Ran core tests.

Ran a few queries under TSAN, checked that it didn't report any more
races in this code after fixing those data races.

I couldn't construct a regression test because there are no easily
testable consequences of the change - the main difference is that
some scanner threads start earlier when there is pressure on scanner
thread tokens but that is hard to construct a robust test around.

Change-Id: I16d31d72441aff7293759281d0248e641df43704
---
M be/src/exec/blocking-join-node.cc
M be/src/exec/hdfs-scan-node.cc
M be/src/exec/hdfs-scan-node.h
M be/src/exec/kudu-scan-node.cc
M be/src/exec/kudu-scan-node.h
M be/src/runtime/fragment-instance-state.cc
M be/src/runtime/io/disk-io-mgr-internal.h
M be/src/runtime/io/disk-io-mgr.h
M be/src/runtime/runtime-state.cc
M be/src/runtime/runtime-state.h
M be/src/runtime/thread-resource-mgr-test.cc
M be/src/runtime/thread-resource-mgr.cc
M be/src/runtime/thread-resource-mgr.h
13 files changed, 270 insertions(+), 318 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 4
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 

[Impala-ASF-CR] IMPALA-6821: Push down limits into Kudu

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6821: Push down limits into Kudu
..


Patch Set 6:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe35e70065d8706b575e24fe20902cd405b49941
Gerrit-Change-Number: 10119
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 27 Apr 2018 17:58:48 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6821: Push down limits into Kudu

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/10119 )

Change subject: IMPALA-6821: Push down limits into Kudu
..


Patch Set 6: Code-Review+2

gvo failed due to:
- clang-format issue that has been fixed (added KUDU_RETURN_IF_ERROR)
- unrelated test failure

carrying forward


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibe35e70065d8706b575e24fe20902cd405b49941
Gerrit-Change-Number: 10119
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Fri, 27 Apr 2018 17:57:52 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6821: Push down limits into Kudu

[Thomas Tauber-Marshall (Code Review)]

Hello Alex Behm, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6821: Push down limits into Kudu
..

IMPALA-6821: Push down limits into Kudu

This patch takes advantage of a recent change in Kudu (KUDU-16) that
exposes the ability to set limits on KuduScanners. Since each
KuduScanner corresponds to a scan token, and there will be multiple
scan tokens per query, this is just a performance optimization in
cases where the limit is smaller than the number of rows per token,
and Impala still needs to apply the limit on our side for cases where
the limit is greater than the number of rows per token.

Testing:
- Added e2e tests for various situations where limits are applied at
  a Kudu scan node.
- For the query 'select * from tpch_kudu.lineitem limit 1', a best
  case perf scenario for this change where the limit is highly
  effective, the time spent in the Kudu scan node was reduced from
  6.107ms to 3.498ms (avg over 3 runs).
- For the query 'select count(*) from (select * from
  tpch_kudu.lineitem limit 100) v', a worst case perf scenario for
  this change where the limit is ineffective, the time spent in the
  Kudu scan node was essentially unchanged, 32.815ms previously vs.
  29.532ms (avg over 3 runs).

Change-Id: Ibe35e70065d8706b575e24fe20902cd405b49941
---
M be/src/exec/kudu-scanner.cc
A testdata/workloads/functional-query/queries/QueryTest/kudu_limit.test
M tests/query_test/test_kudu.py
3 files changed, 120 insertions(+), 0 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ibe35e70065d8706b575e24fe20902cd405b49941
Gerrit-Change-Number: 10119
Gerrit-PatchSet: 6
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6522: [DOCS] Document Decimal V2

[Alex Rodoni (Code Review)]

Hello Greg Rahn, Taras Bobrovytsky, Alex Behm, Dan Hecht, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-6522: [DOCS] Document Decimal V2
..

IMPALA-6522: [DOCS] Document Decimal V2

Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Cherry-picks: not for 2.x.
---
M docs/impala_keydefs.ditamap
M docs/topics/impala_decimal.xml
2 files changed, 854 insertions(+), 708 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ic436ff80c9ad05cfada97280cd47552879214a3d
Gerrit-Change-Number: 10066
Gerrit-PatchSet: 7
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-6920: fix inconsistencies with scanner thread tokens

[Csaba Ringhofer (Code Review)]

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

Change subject: IMPALA-6920: fix inconsistencies with scanner thread tokens
..


Patch Set 3:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.h
File be/src/runtime/thread-resource-mgr.h:

http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.h@99
PS3, Line 99:
nit: extra space - several copy pasted comments contain double spaces, so this 
may be intentional


http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.h@189
PS3, Line 189: Set to NULL when unregistered.
Is there a reason behind this? Seems to be an object pool heritage, where 
Reset() used to reset all the members with the exception of parent_.


http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.cc
File be/src/runtime/thread-resource-mgr.cc:

http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.cc@55
PS3, Line 55:   DCHECK(pools_.find(pool.get()) == pools_.end());
Is this DCHECK still useful after removing object caching?


http://gerrit.cloudera.org:8080/#/c/10186/3/be/src/runtime/thread-resource-mgr.cc@133
PS3, Line 133: not
nit: double not



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I16d31d72441aff7293759281d0248e641df43704
Gerrit-Change-Number: 10186
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Comment-Date: Fri, 27 Apr 2018 17:17:33 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6916: Implement COMMENT ON DATABASE

[Alex Behm (Code Review)]

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

Change subject: IMPALA-6916: Implement COMMENT ON DATABASE
..


Patch Set 5:

(5 comments)

http://gerrit.cloudera.org:8080/#/c/10171/5/common/thrift/JniCatalog.thrift
File common/thrift/JniCatalog.thrift:

http://gerrit.cloudera.org:8080/#/c/10171/5/common/thrift/JniCatalog.thrift@634
PS5, Line 634: enum TCommentScope {
Is this enum needed? Can't we determine which comment we should update based on 
which fields are set in TCommentOnParams? We'll eventually need db, table, 
column, function. Seems like the enum is redundant.


http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java
File fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java:

http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/analysis/CommentOnStmt.java@30
PS5, Line 30: Preconditions.checkNotNull(comment);
In Postgres you can remove a comment with:
COMMENT ON DATABASE  IS NULL;

We should allow that, too.


http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
File fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java:

http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3524
PS5, Line 3524: Preconditions.checkNotNull(db);
I don't think this is a precondition. The database could have legitimately been 
deleted while this COMMENT ON operation is ongoing.


http://gerrit.cloudera.org:8080/#/c/10171/5/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@3528
PS5, Line 3528:   addDbToCatalogUpdate(db, response.result);
You need to increment and set the version of the db, not only the thrift 
representation.

Have you tested whether this db modification gets propagated to all impalads 
and not just the coordinator?

Changes like this are really tricky to get right. Let me think some more about 
the concurrency implications of this. Will get back with more details.


http://gerrit.cloudera.org:8080/#/c/10171/5/tests/metadata/test_ddl.py
File tests/metadata/test_ddl.py:

http://gerrit.cloudera.org:8080/#/c/10171/5/tests/metadata/test_ddl.py@197
PS5, Line 197:   def test_alter_database_comment(self, vector, unique_database):
test_comment_on_database



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifcf909c18f97073346f6f603538bf921e69fbb00
Gerrit-Change-Number: 10171
Gerrit-PatchSet: 5
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Fri, 27 Apr 2018 17:16:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6802 (part 1): Clean up authorization tests

[Alex Behm (Code Review)]

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

Change subject: IMPALA-6802 (part 1): Clean up authorization tests
..


Patch Set 6:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java
File fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java:

http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@47
PS5, Line 47: import static org.junit.Assert.fail;
> Done. Avoiding calling catalog_.reset() and making Sentry RPCs makes the wh
That's more like it! Fantastic.


http://gerrit.cloudera.org:8080/#/c/10135/5/fe/src/test/java/org/apache/impala/analysis/AuthorizationTestV2.java@349
PS5, Line 349: "functional.alltypes", onTable("functional", 
"alltypes", allExcept(
> Shouldn't this be a separate test case since this is to test a table with a
In my mind the auth tests should check whether the authorization rules are 
applied correctly.
A table reference with implicit or explicit aliases still refers to the same 
underlying table,
so it's a variant of the same test, but not really a different auth test.

Ideally, we should separate testing different SQL variants of referencing the 
same underlying entity
from the auth tests themselves.

For exampe, we could have a single test that asserts the following things 
generate the same
PrivilegeRequests during analysis.
- table reference with implicit/explicit aliases
- qualified/unqualified table references
- qualified/unqualified column references
- qualified/unqualified star

Then when we do the auth tests, we can focus on whatever variant is most 
convenient.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9cd5713607c423f644451af5ebb3494d3a728e3b
Gerrit-Change-Number: 10135
Gerrit-PatchSet: 6
Gerrit-Owner: Fredy Wijaya 
Gerrit-Reviewer: Adam Holley 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Comment-Date: Fri, 27 Apr 2018 16:51:01 +
Gerrit-HasComments: Yes

[Impala-ASF-CR](2.x) IMPALA-6892: CheckHashAndDecrypt() includes file and host

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6892: CheckHashAndDecrypt() includes file and host
..


Patch Set 1:

Oh no problem, I was just providing a reason for carrying the +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Gerrit-Change-Number: 10232
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Apr 2018 16:03:42 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) IMPALA-6892: CheckHashAndDecrypt() includes file and host

[Michael Brown (Code Review)]

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

Change subject: IMPALA-6892: CheckHashAndDecrypt() includes file and host
..


Patch Set 1:

Thanks Tim. It did look trivial but I wanted someone more familiar with the 
code to handle it.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Gerrit-Change-Number: 10232
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Apr 2018 15:59:43 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) IMPALA-6892: CheckHashAndDecrypt() includes file and host

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6892: CheckHashAndDecrypt() includes file and host
..


Patch Set 1:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Gerrit-Change-Number: 10232
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Comment-Date: Fri, 27 Apr 2018 15:55:49 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) IMPALA-6892: CheckHashAndDecrypt() includes file and host

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-6892: CheckHashAndDecrypt() includes file and host
..


Patch Set 1: Code-Review+2

trivial conflict resolution (nearby lines changed)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: comment
Gerrit-Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Gerrit-Change-Number: 10232
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 27 Apr 2018 15:56:11 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries

[Impala Public Jenkins (Code Review)]

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

Change subject: IMPALA-6314: Add run time scalar subquery check for 
uncorrelated subqueries
..


Patch Set 31:

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


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Gerrit-Change-Number: 9005
Gerrit-PatchSet: 31
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 27 Apr 2018 15:56:08 +
Gerrit-HasComments: No

[Impala-ASF-CR](2.x) IMPALA-6892: CheckHashAndDecrypt() includes file and host

[Tim Armstrong (Code Review)]

Hello Impala Public Jenkins,

I'd like you to do a code review. Please visit

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

to review the following change.


Change subject: IMPALA-6892: CheckHashAndDecrypt() includes file and host
..

IMPALA-6892: CheckHashAndDecrypt() includes file and host

The error text with AES-GCM enabled looks like:

  Error reading 44 bytes from scratch file
  '/tmp/impala-scratch/0:0_d43635d0-8f55-485e-8899-907af289ac86' on
  backend tarmstrong-box:22000 at offset 0: verification of read data
  failed.
  OpenSSL error in EVP_DecryptFinal:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:
  139634997483216:error:0607C083:digital envelope
  routines:EVP_CIPHER_CTX_ctrl:no cipher set:evp_enc.c:610:

Testing:
Added a backend test to exercise the code path and verify the error
code.

Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Reviewed-on: http://gerrit.cloudera.org:8080/10204
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins 
---
M be/src/exec/exec-node.cc
M be/src/runtime/tmp-file-mgr-test.cc
M be/src/runtime/tmp-file-mgr.cc
M common/thrift/generate_error_codes.py
4 files changed, 70 insertions(+), 4 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: 2.x
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0652d6cdfbb4e543dd0ca46b7cc65edc4e41a2d8
Gerrit-Change-Number: 10232
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Impala Public Jenkins 

[native-toolchain-CR] Improve build-kudu-only.sh

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has abandoned this change. ( 
http://gerrit.cloudera.org:8080/10227 )

Change subject: Improve build-kudu-only.sh
..


Abandoned

decided to go in a different direction
--
To view, visit http://gerrit.cloudera.org:8080/10227
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: native-toolchain
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I6b76e6fbd2f7be333a0f9580128ec9f681b3ddf8
Gerrit-Change-Number: 10227
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries

[Zoltan Borok-Nagy (Code Review)]

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

Change subject: IMPALA-6314: Add run time scalar subquery check for 
uncorrelated subqueries
..


Patch Set 29:

(1 comment)

Thanks!
I rebased the patch set to contain IMPALA-6934.

http://gerrit.cloudera.org:8080/#/c/9005/29/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
File fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java:

http://gerrit.cloudera.org:8080/#/c/9005/29/fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java@484
PS29, Line 484: if (isRuntimeScalar) {
> Thanks! I think we should keep the tests added in this patch.
OK, I kept the tests.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Gerrit-Change-Number: 9005
Gerrit-PatchSet: 29
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 27 Apr 2018 11:38:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries

[Zoltan Borok-Nagy (Code Review)]

Hello Attila Jeges, Dimitris Tsirogiannis, Tim Armstrong, Csaba Ringhofer, Alex 
Behm, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-6314: Add run time scalar subquery check for 
uncorrelated subqueries
..

IMPALA-6314: Add run time scalar subquery check for uncorrelated subqueries

If a scalar subquery is used with a binary predicate,
or, used in an arithmetic expression, it must return
only one row/column to be valid. If this cannot be
guaranteed at parse time through a single row aggregate
or limit clause, Impala fails the query like such.

E.g., currently the following query is not allowed:
SELECT bigint_col
FROM alltypesagg
WHERE id = (SELECT id FROM alltypesagg WHERE id = 1)

However, it would be allowed if the query contained
a LIMIT 1 clause, or instead of id it was max(id).

This commit makes the example valid by introducing a
runtime check to test if the subquery returns a single
row. If the subquery returns more than one row, it
aborts the query with an error.

I added a new node type, called CardinalityCheckNode. It
is created during planning on top of the subquery when
needed, then during execution it checks if its child
only returns a single row.

I extended the frontend tests and e2e tests as well.

Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
---
M be/src/exec/CMakeLists.txt
A be/src/exec/cardinality-check-node.cc
A be/src/exec/cardinality-check-node.h
M be/src/exec/exec-node.cc
M common/thrift/PlanNodes.thrift
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateViewStmt.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/HdfsCachingOp.java
M fe/src/main/java/org/apache/impala/analysis/InPredicate.java
M fe/src/main/java/org/apache/impala/analysis/IsNullPredicate.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/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
A fe/src/main/java/org/apache/impala/planner/CardinalityCheckNode.java
M fe/src/main/java/org/apache/impala/planner/DistributedPlanner.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeSubqueriesTest.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/subquery-rewrite.test
M 
testdata/workloads/functional-query/queries/QueryTest/nested-types-subplan.test
M testdata/workloads/functional-query/queries/QueryTest/subquery.test
28 files changed, 1,130 insertions(+), 79 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0f52b93a60eeacedd242a2f17fa6b99c4fc38e06
Gerrit-Change-Number: 9005
Gerrit-PatchSet: 31
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Attila Jeges 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Reviewer: Zoltan Borok-Nagy 

[Impala-ASF-CR] IMPALA-6934: Wrong results with EXISTS subquery containing ORDER BY, LIMIT, and OFFSET

[Zoltan Borok-Nagy (Code Review)]

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

Change subject: IMPALA-6934: Wrong results with EXISTS subquery containing 
ORDER BY, LIMIT, and OFFSET
..


Patch Set 2:

Thanks, Alex!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9693623d3d0a8446913261252f8e4a07935645e0
Gerrit-Change-Number: 10218
Gerrit-PatchSet: 2
Gerrit-Owner: Zoltan Borok-Nagy 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Zoltan Borok-Nagy 
Gerrit-Comment-Date: Fri, 27 Apr 2018 09:59:39 +
Gerrit-HasComments: No