[Impala-ASF-CR] IMPALA-4167: Support insert plan hints for CREATE TABLE AS SELECT

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4167: Support insert plan hints for CREATE TABLE AS 
SELECT
..


Patch Set 3:

(13 comments)

http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@9
PS3, Line 9: Adding support for "clustered", "noclustered", "shuffle" and 
"noshuffle"
How about "This change adds support..."


http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@12
PS3, Line 12: example:
Capitalize


http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@19
PS3, Line 19: Sort by partition columns before insert to make the insert more 
efficient.
Mention where exactly the sort happens (locally vs distributed).


http://gerrit.cloudera.org:8080/#/c/8400/3//COMMIT_MSG@26
PS3, Line 26: exchenge
spelling


http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup
File fe/src/main/cup/sql-parser.cup:

http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup@1054
PS3, Line 1054: RESULT = new CreateTableAsSelectStmt(new 
CreateTableStmt(tbl_def),
nit: Can you wrap the lines at 90 chars?


http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup@1167
PS3, Line 1167: tbl_def_without_col_defs ::=
Does that now also allow hints like "CREATE /*+ clustered */ TABLE FOO LIKE 
PARQUET...?


http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/cup/sql-parser.cup@1168
PS3, Line 1168:   KW_CREATE external_val:external opt_plan_hints:hints KW_TABLE 
if_not_exists_val:if_not_exists
long line


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

http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/main/java/org/apache/impala/analysis/TableDef.java@155
PS3, Line 155:   TableDef(TableName tableName, boolean isExternal, boolean 
ifNotExists, List hints) {
long line


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

http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1633
PS3, Line 1633: Only
What does "only" mean here?


http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1647
PS3, Line 1647:   // HBase tables cannot be tested, as currently Impala 
cannot create HBase tables.
It's weird that this explanation occurs in the middle of the function. Can you 
move it to the top?


http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1653
PS3, Line 1653:
Why is there a newline?


http://gerrit.cloudera.org:8080/#/c/8400/3/fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java@1677
PS3, Line 1677: "select * from functional.alltypes");
Please also add tests that have additional hints in the select clause.


http://gerrit.cloudera.org:8080/#/c/8400/3/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test
File testdata/workloads/functional-planner/queries/PlannerTest/ddl.test:

http://gerrit.cloudera.org:8080/#/c/8400/3/testdata/workloads/functional-planner/queries/PlannerTest/ddl.test@207
PS3, Line 207: # IMPALA-4167: clustered CTAS into partitioned table adds sort 
node.
Can you add tests for noclustered and for sort by()?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8d74bca999da8ae1bb89427c70841f33e3c56ab0
Gerrit-Change-Number: 8400
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Gabor Kaszab 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Tue, 14 Nov 2017 21:50:05 +
Gerrit-HasComments: Yes

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

[Lars Volker (Code Review)]

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

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


Patch Set 2:

(4 comments)

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

http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-5624: ProcessStateInfo::ReadProcFileDescriptorInfo() should 
not fork a process
Please describe in this line what the change does, not what it should do, i.e. 
"prevent fork".


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


http://gerrit.cloudera.org:8080/#/c/8546/2//COMMIT_MSG@18
PS2, Line 18: "expected value" in advance, and the number of file desciptors 
can change anytime.
Couldn't we test that a reasonable minimum number of file descriptors is 
returned, e.g. >0?


http://gerrit.cloudera.org:8080/#/c/8546/2/be/src/util/process-state-info.h
File be/src/util/process-state-info.h:

http://gerrit.cloudera.org:8080/#/c/8546/2/be/src/util/process-state-info.h@99
PS2, Line 99:   /// Number of file descriptors.
Please clarify what kinds of file descriptors this counts.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ibffae8069a62e100abbfa7d558b49040b095ddc0
Gerrit-Change-Number: 8546
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 14 Nov 2017 19:09:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4985: use parquet stats of nested types for dynamic pruning

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4985: use parquet stats of nested types for dynamic 
pruning
..


Patch Set 2:

(9 comments)

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

http://gerrit.cloudera.org:8080/#/c/8480/2//COMMIT_MSG@13
PS2, Line 13: value
Should this read "type"?


http://gerrit.cloudera.org:8080/#/c/8480/2//COMMIT_MSG@16
PS2, Line 16: the scalar value must be on a path
: to the root of the nested value where every node on the path
: is required
I'm not sure I'm following the reasoning behind this. Please see my comments in 
the tests.


http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1109
PS1, Line 1109: tScanRanges = 0;
> hmm, looks like a proxy for estimating when a scan range will definitely be
Sounds good, thanks for the clarification.


http://gerrit.cloudera.org:8080/#/c/8480/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/8480/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@567
PS2, Line 567:   tryComputeMinMaxPredicate(analyzer, pred);
nit: this looks like it could go on a single line now.


http://gerrit.cloudera.org:8080/#/c/8480/2/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@575
PS2, Line 575: tryComputeMinMaxPredicate(analyzer, pred);
nit: single line?


http://gerrit.cloudera.org:8080/#/c/8480/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test
File 
testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test:

http://gerrit.cloudera.org:8080/#/c/8480/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test@9
PS2, Line 9: row_regex: .*NumStatsFilteredRowGroups: 2 .*
While you're here, do you mind converting them to the aggregation(..) syntax? 
Then you could lift the restriction of 'num_nodes = 1' in test_nested_types.py.


http://gerrit.cloudera.org:8080/#/c/8480/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test@58
PS2, Line 58: where bottom.item < -2;
This looks like a c error from the query above. Can you double check that the 
tests run as you expect them to?


http://gerrit.cloudera.org:8080/#/c/8480/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test@98
PS2, Line 98: where a.item.e < -10
This may be seem like an ignorant question, but doesn't this predicate make the 
bottom field required? In general, does having a predicate on a field mean that 
it must not be null?


http://gerrit.cloudera.org:8080/#/c/8480/2/testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test@107
PS2, Line 107: left outer join c.nested_struct.c.d cn, cn.item a where a.item.e 
< -10;
Same here, if a row group has no values in nested_struct.c.d.item.item.e that 
are < -10, then their rows will not show up in the result, no?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c99e20cb080b504442cd5376ea3e046016158fe
Gerrit-Change-Number: 8480
Gerrit-PatchSet: 2
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 14 Nov 2017 18:54:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4985: use parquet stats of nested types for dynamic pruning

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4985: use parquet stats of nested types for dynamic 
pruning
..


Patch Set 1:

(8 comments)

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

http://gerrit.cloudera.org:8080/#/c/8480/1//COMMIT_MSG@12
PS1, Line 12: A nested value is defined to
: be on a path of one or more nested types that is rooted at a
: table column.
I don't understand what that sentence means. Can you try to clarify the 
distinction between nested value and nested type?


http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
File fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java:

http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@435
PS1, Line 435:   // Checks if slot refers to an array "pos" pseudo-column.
Can you add a comment explaining why checking for getColumn() == null is not 
sufficient?


http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@441
PS1, Line 441: isMapStruct
I think it would be clearer to add a isArrayStruct() method to 
CollectionStructType to emphasize that that's what we're checking.


http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@564
PS1, Line 564:
nit: the surrounding code seems to omit this space.


http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@567
PS1, Line 567:   for (Expr pred: entry.getValue()) {
 : if (pred instanceof BinaryPredicate) {
 :   tryComputeBinaryMinMaxPredicate(analyzer, 
(BinaryPredicate) pred);
 : } else if (pred instanceof InPredicate) {
 :   tryComputeInListMinMaxPredicate(analyzer, 
(InPredicate) pred);
 : }
 :   }
This looks like a duplication of the above loop. Adding additional predicates 
in the future may require changing both loops. Have you considered factoring it 
into it's own method?


http://gerrit.cloudera.org:8080/#/c/8480/1/fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java@1109
PS1, Line 1109: slot.getColumn() == null
Is this another check for a pos slot?


http://gerrit.cloudera.org:8080/#/c/8480/1/testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test
File 
testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test:

http://gerrit.cloudera.org:8080/#/c/8480/1/testdata/workloads/functional-query/queries/QueryTest/nested-types-parquet-stats.test@141
PS1, Line 141: Basics test
I'm not sure I understand what Basics means. Can you elaborate? I think we 
often order tests by ascending complexity so that the simpler ones fail before 
the complex ones.


http://gerrit.cloudera.org:8080/#/c/8480/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test
File testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test:

http://gerrit.cloudera.org:8080/#/c/8480/1/testdata/workloads/functional-query/queries/QueryTest/parquet-stats.test@460
PS1, Line 460: 
Does this remove the trailing newline?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I0c99e20cb080b504442cd5376ea3e046016158fe
Gerrit-Change-Number: 8480
Gerrit-PatchSet: 1
Gerrit-Owner: Vuk Ercegovac 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Tue, 14 Nov 2017 00:10:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding
..


Patch Set 14: Code-Review+2

Yes, I had another look at the delta since PS12 and it looks good to me.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35de0cf80c86f501c4a39270afc8fb8111552ac6
Gerrit-Change-Number: 8267
Gerrit-PatchSet: 14
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 13 Nov 2017 22:52:55 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 17: Code-Review+2

Carrying Michael's +2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 17
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 01 Nov 2017 21:32:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6054: Parquet dictionary pages should be freed on dictionary construction

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6054: Parquet dictionary pages should be freed on 
dictionary construction
..


Patch Set 1:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.h
File be/src/exec/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.h@376
PS1, Line 376:   virtual bool DictionaryReferencesBuffer() const { return 
false; }
Add comment. Make pure if possible. Alternatively, you could do something 
similar to PageContainsTupleData().


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

http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@272
PS1, Line 272:   virtual bool DictionaryReferencesBuffer() const {
add "override"


http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@541
PS1, Line 541: some
how about rewording this to "Column readers for types that require ..."


http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@542
PS1, Line 542:   inline bool DictionaryReferencesBufferInline() const {
nit: single line


http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@945
PS1, Line 945: tmp_buffer
better name


http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@947
PS1, Line 947:   int uncompressed_size = decompressor_.get() != nullptr
Move this to L954


http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@951
PS1, Line 951:   if (decompressor_.get() == nullptr && 
!DictionaryReferencesBuffer()) {
Can you add a brief comment to explain how the 4 different cases are handled?


http://gerrit.cloudera.org:8080/#/c/8436/1/be/src/exec/parquet-column-readers.cc@987
PS1, Line 987: if (DictionaryReferencesBuffer()) {
 :   memcpy(dict_values, data_, data_size);
 : }
Single line :)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4d9d5f4da1028d961155dafdac0028a1c3641004
Gerrit-Change-Number: 8436
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Wed, 01 Nov 2017 19:16:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..


Patch Set 6:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8411/6//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8411/6//COMMIT_MSG@10
PS6, Line 10: makes
> remove
Done


http://gerrit.cloudera.org:8080/#/c/8411/6//COMMIT_MSG@11
PS6, Line 11: .
> ..to be consistent with Hive...?
Done


http://gerrit.cloudera.org:8080/#/c/8411/6/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/8411/6/fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java@a2784
PS6, Line 2784:
> Just for my understanding, this change is only for add/drop partitions righ
Correct. This one is called from code that updates the stats through 
"applyAlterPartition()", so I left it in place.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Tue, 31 Oct 2017 00:05:00 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

Hello Bharath Vissapragada, Alex Behm,

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

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

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..

IMPALA-6124: Fix alter table ddl updates and test

Impala would previously update the ddl time of a table when dropping a
partition but not when adding one. This change removes updates to the
ddl time when partitions are added or removed to be consistent with
Hive.

Additionally the check in the ddl update test would fail if some
operations took longer than 20 seconds. Instead, this change makes sure
that the ddl time increases as intended.

To test this change I ran test_last_ddl_time_update in exhaustive mode
and also ran a private S3 build.

Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M tests/metadata/test_last_ddl_time_update.py
2 files changed, 6 insertions(+), 9 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 7
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

Hello Bharath Vissapragada, Alex Behm,

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

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

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..

IMPALA-6124: Fix alter table ddl updates and test

Impala would previously update the ddl time of a table when dropping a
partition but not when adding one. This change makes removed updates to
the ddl time when partitions are added or removed.

Additionally the check in the ddl update test would fail if some
operations took longer than 20 seconds. Instead, this change makes sure
that the ddl time increases as intended.

To test this change I ran test_last_ddl_time_update in exhaustive mode
and also ran a private S3 build.

Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M tests/metadata/test_last_ddl_time_update.py
2 files changed, 6 insertions(+), 9 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 6
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..


Patch Set 5:

To make things consistent I removed the ddl updates from operations that add or 
drop partitions and updated the commit message.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 30 Oct 2017 23:41:51 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

Hello Bharath Vissapragada, Alex Behm,

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

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

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..

IMPALA-6124: Fix alter table ddl updates and test

Impala would previously update the ddl time of a table when dropping a
partition but not when adding one. This change makes it also update the
ddl time when adding a partition.

Additionally the check in the ddl update test would fail if some
operations took longer than 20 seconds. Instead, this change makes sure
that the ddl time increases as intended.

To test this change I ran test_last_ddl_time_update in exhaustive mode
and also ran a private S3 build.

Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M tests/metadata/test_last_ddl_time_update.py
2 files changed, 6 insertions(+), 9 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 5
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

Hello Bharath Vissapragada, Alex Behm,

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

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

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..

IMPALA-6124: Fix alter table ddl updates and test

Impala would previously update the ddl time of a table when dropping a
partition but not when adding one. This change makes it also update the
ddl time when adding a partition.

Additionally the check in the ddl update test would fail if some
operations took longer than 20 seconds. Instead, this change makes sure
that the ddl time increases as intended.

To test this change I ran test_last_ddl_time_update in exhaustive mode
and also ran a private S3 build.

Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M tests/metadata/test_last_ddl_time_update.py
2 files changed, 6 insertions(+), 8 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..


Patch Set 3:

> (1 comment)
 >
 > The patch looks good to me, but I'm still curious how it worked
 > before. Tried digging into the HMS code to see how the ddlTime is
 > updated, but no clues.

Thank you for having a look. It didn't work before. The test was broken and 
didn't catch this. I found it after fixing the test. :)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 30 Oct 2017 19:10:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

Lars Volker has removed a vote on this change.

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..


Removed Verified+1 by Lars Volker 
--
To view, visit http://gerrit.cloudera.org:8080/8411
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: deleteVote
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..


Patch Set 3: Verified+1

The private S3 build passed.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 30 Oct 2017 03:55:07 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..


Patch Set 3:

Running another private S3 build now.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Sun, 29 Oct 2017 20:18:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..

IMPALA-6124: Fix alter table ddl updates and test

Impala would previously update the ddl time of a table when dropping a
partition but not when adding one. This change makes it also update the
ddl time when adding a partition.

Additionally the check in the ddl update test would fail if some
operations took longer than 20 seconds. Instead, this change makes sure
that the ddl time increases as intended.

To test this change I ran test_last_ddl_time_update in exhaustive mode
and also ran a private S3 build.

Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M tests/metadata/test_last_ddl_time_update.py
2 files changed, 6 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6124: Fix alter table ddl updates and test

[Lars Volker (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-6124: Fix alter table ddl updates and test
..

IMPALA-6124: Fix alter table ddl updates and test

Impala would previously update the ddl time of a table when dropping a
partition but not when adding one. This change makes it also update the
ddl time when adding a partition.

Additionally the check in the ddl update test would fail if some
operations took longer than 20 seconds. Instead, this change makes sure
that the ddl time increases as intended.

Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
---
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M tests/metadata/test_last_ddl_time_update.py
2 files changed, 6 insertions(+), 2 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 2
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-6124: Make assertion in ddl update test resilient to long runtime

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6124: Make assertion in ddl update test resilient to 
long runtime
..


Patch Set 1: Verified-1

The test failed. Let discuss in the JIRA.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Sun, 29 Oct 2017 18:35:49 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6124: Make assertion in ddl update test resilient to long runtime

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6124: Make assertion in ddl update test resilient to 
long runtime
..


Patch Set 1:

I'm running a private S3 test to validate this.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Sat, 28 Oct 2017 16:58:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6124: Make assertion in ddl update test resilient to long runtime

[Lars Volker (Code Review)]

Lars Volker has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8411


Change subject: IMPALA-6124: Make assertion in ddl update test resilient to 
long runtime
..

IMPALA-6124: Make assertion in ddl update test resilient to long runtime

The check in the ddl update test would fail if some operations took
longer than 20 seconds. Instead, this change makes sure that the ddl
time increases as intended.

Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
---
M tests/metadata/test_last_ddl_time_update.py
1 file changed, 2 insertions(+), 2 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3126252e7709304d3e1fa4bb06a0b847180bd6cf
Gerrit-Change-Number: 8411
Gerrit-PatchSet: 1
Gerrit-Owner: Lars Volker 

[Impala-ASF-CR] IMPALA-6123: Fix column order of a query test in test inline view limit

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6123: Fix column order of a query test in 
test_inline_view_limit
..


Patch Set 2: Code-Review+2

I'll +2 this to unblock our exhaustive tests. Please follow up with Alex on 
whether this is problem was expected b/c of IMPALA-886.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11667872b8788a8b0040bf9252bf07b987b5d330
Gerrit-Change-Number: 8409
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 28 Oct 2017 02:35:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6123: Fix column order of a query test in test inline view limit

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6123: Fix column order of a query test in 
test_inline_view_limit
..


Patch Set 1:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/8409/1//COMMIT_MSG@10
PS1, Line 10: because the output column order is different from
: the expected one for it corresponding hbase table
how about: ... because Impala returns columns from HBase tables in a different 
order (IMPALA-886).



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I11667872b8788a8b0040bf9252bf07b987b5d330
Gerrit-Change-Number: 8409
Gerrit-PatchSet: 1
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 28 Oct 2017 01:19:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] DRAFT IMPALA-5185: Skip pages based on Parquet::Statistics

[Lars Volker (Code Review)]

Lars Volker has abandoned this change. ( http://gerrit.cloudera.org:8080/7354 )

Change subject: DRAFT IMPALA-5185: Skip pages based on Parquet::Statistics
..


Abandoned

IMPALA-5185 has been closed as Won't Fix.
--
To view, visit http://gerrit.cloudera.org:8080/7354
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I8eec838c5baf22167049f570dd0ef9762c5ae0a6
Gerrit-Change-Number: 7354
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Pooja Nilangekar 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] DRAFT IMPALA-5185: Skip pages based on Parquet::Statistics

[Lars Volker (Code Review)]

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

Change subject: DRAFT IMPALA-5185: Skip pages based on Parquet::Statistics
..


Patch Set 3:

Yes, thanks for the reminder. I'll push the change to a private branch 
somewhere and link it in the JIRA so that we can re-use parts of it in the 
future if we want to.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8eec838c5baf22167049f570dd0ef9762c5ae0a6
Gerrit-Change-Number: 7354
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Mostafa Mokhtar 
Gerrit-Reviewer: Pooja Nilangekar 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 25 Oct 2017 19:36:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 15: Code-Review+1

Thank you for the continued effort. I like the exceptions. Let's give Mike a 
bit of time to look at the latest PS and then I think this is ready to be 
submitted.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 15
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Wed, 25 Oct 2017 17:14:13 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6076: Parquet BIT PACKED deprecation warning

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6076: Parquet BIT_PACKED deprecation warning
..


Patch Set 1: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I02dd4009089a264b28376492b1b40361d767d5d9
Gerrit-Change-Number: 8370
Gerrit-PatchSet: 1
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Tue, 24 Oct 2017 18:07:29 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 14:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8038/14//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8038/14//COMMIT_MSG@30
PS14, Line 30:
This seems to lack a _


http://gerrit.cloudera.org:8080/#/c/8038/14/tests/shell/test_shell_commandline.py
File tests/shell/test_shell_commandline.py:

http://gerrit.cloudera.org:8080/#/c/8038/14/tests/shell/test_shell_commandline.py@389
PS14, Line 389: formatting
This does not report a formatting error, so I'm inclined to remove the "Check 
formatting:" part. What do you think?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 14
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 24 Oct 2017 17:35:19 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding
..


Patch Set 12: Code-Review+1

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8267/12/be/src/util/dict-test.cc
File be/src/util/dict-test.cc:

http://gerrit.cloudera.org:8080/#/c/8267/12/be/src/util/dict-test.cc@255
PS12, Line 255:   {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 1, 2, 
3, 4, 5, 6, 7, 8, 9, 10,
> I wanted to make sure that it tested error propagation in UnpackAndDecode32
Ah, that makes sense. Thx.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35de0cf80c86f501c4a39270afc8fb8111552ac6
Gerrit-Change-Number: 8267
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 23 Oct 2017 22:24:41 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding
..


Patch Set 12:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8267/12/be/src/util/dict-test.cc
File be/src/util/dict-test.cc:

http://gerrit.cloudera.org:8080/#/c/8267/12/be/src/util/dict-test.cc@255
PS12, Line 255:   {1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 1, 2, 
3, 4, 5, 6, 7, 8, 9, 10,
What is the benefit of this test? Won't it stop when it tries to decode the 
first '10', just like the previous one? Wouldn't it be better to put 32 
successful decodes in front of the first failure?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35de0cf80c86f501c4a39270afc8fb8111552ac6
Gerrit-Change-Number: 8267
Gerrit-PatchSet: 12
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:36:07 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding
..


Patch Set 11:

(4 comments)

One more questions, looks good to me otherwise.

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.inline.h
File be/src/util/bit-packing.inline.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.inline.h@56
PS8, Line 56:   switch (bit_width) {
> I restructured it a bit more to separate out the macro definition from the
Much better, thx.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc
File be/src/util/dict-test.cc:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc@193
PS8, Line 193: // Make sure that SetData() resets the dictionary decoder, 
including the embedded RLE
> I originally added it because I had a bug here that was somewhat tricky to
Thank you for clarifying this.


http://gerrit.cloudera.org:8080/#/c/8267/11/be/src/util/dict-test.cc
File be/src/util/dict-test.cc:

http://gerrit.cloudera.org:8080/#/c/8267/11/be/src/util/dict-test.cc@234
PS11, Line 234:   // Generate a dictionary with 9 values (requires 3 bits to 
encode).
I don't understand how 3 bits can encode 9 different values. Should these be 8 
values instead?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc
File be/src/util/rle-test.cc:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc@120
PS8, Line 120: num_vals,
> I don't feel strongly either way and I'm probably not that consistent. I've
Sounds good.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I35de0cf80c86f501c4a39270afc8fb8111552ac6
Gerrit-Change-Number: 8267
Gerrit-PatchSet: 11
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 23 Oct 2017 21:12:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala
..


Patch Set 17: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-Change-Number: 7954
Gerrit-PatchSet: 17
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 20 Oct 2017 23:13:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 11:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py
File shell/option_parser.py:

http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@36
PS11, Line 36: convert_loaded_shell_option
I still struggle to understand the function name. How about 
parse_shell_option(option, default_value).


http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@37
PS11, Line 37:   """Converts values 'True', 'False' and 'None' to their 
corresponding python values.
Mention that this parses string values into corresponding python types. The 
comment should also explain what happens if the parsing fails.


http://gerrit.cloudera.org:8080/#/c/8038/11/shell/option_parser.py@64
PS11, Line 64: values
types



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 11
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Fri, 20 Oct 2017 22:50:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding
..


Patch Set 10:

(21 comments)

Mostly minor stuff that I came across when reading through the change.

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

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.h@89
PS8, Line 89: in
nit: double word


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.h@91
PS8, Line 91: a
nit: an


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.h@98
PS8, Line 98:   bool FillCacheBitPacked(int batch_size, int* num_cached_levels);
This seems to be left over from refactoring but doesn't have an implementation. 
Remove?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.h@106
PS8, Line 106: in
nit: extra word


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.h@116
PS8, Line 116:   parquet::Encoding::type encoding_ = parquet::Encoding::PLAIN;
Can you add a comment to this one, too?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.h@128
PS8, Line 128:   string filename_;
Can you add a comment for completeness here and include that it's only used for 
error reporting?


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

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.cc@154
PS8, Line 154: there enough
nit: missing word


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/exec/parquet-column-readers.cc@354
PS8, Line 354: since we are only populating top-level tuples.
I don't understand that part of the comment. Should it mean "We only need the 
repetition levels for populating the position slot if we are not populating 
top-level tuples."?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.h
File be/src/util/bit-packing.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.h@67
PS8, Line 67:   static std::pair UnpackValues(const 
uint8_t* __restrict__ in,
> I feel like I created an artificial distinction by making some methods priv
I like the suggestion to make them public except for the helper.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc
File be/src/util/dict-test.cc:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc@193
PS8, Line 193: // Make sure that SetData() clears any buffered literals.
It looks like these are cleared in the underlying RleBatchDecoder inside the 
DictDecoder, which took me a while to understand. Can you expand the comment to 
make it more clear why this is an interesting test? Maybe it'd even be better 
to add a test for that behavior to rle-test.cc?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc@223
PS8, Line 223:   }
The first test in this file calls pool.FreeAll(). Is this needed here too?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc@232
PS8, Line 232: 8
These are 9 values. Am I missing something?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc@246
PS8, Line 246:   vector> test_cases{
"using TestCase = pair<..>" could make this more concise.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/dict-test.cc@277
PS8, Line 277: had
nit: have


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h
File be/src/util/rle-encoding.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@93
PS8, Line 93:   RleBatchDecoder(uint8_t* buffer, int buffer_len, int bit_width) 
{
> It looks like the counts could actually be int32_t since that's what they'r
sgtm


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc
File be/src/util/rle-test.cc:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc@78
PS8, Line 78: for (int i = 0; i < 8; ++i) {
nit: single line


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc@120
PS8, Line 120: BATCH_SIZE
My feeling is that we use lower case for constants within local scopes, but I 
may be wrong.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc@171
PS8, Line 171: if (!decoder->GetLiteralValues(num_literals_to_output, 
vals)) {
nit: single line


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc@319
PS8, Line 319: for (int i = 0; i < num_values; i++) {
nit: single line


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-test.cc@346
PS8, Line 346:   EXPECT_EQ(0, decoded_values[i]);
nit: single line


http://gerrit.cloudera.org:8080/#/c/8267/8/testdata/data/README
File testdata/data/README:

[Impala-ASF-CR] IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4177,IMPALA-6039: batched bit reading and rle decoding
..


Patch Set 8:

(18 comments)

Thank you for reworking this code. I did a pass over the low level files and 
left mostly questions and minor suggestions. I still need to look at the 
integration into the column readers and the tests and benchmarks.

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.h
File be/src/util/bit-packing.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.h@67
PS8, Line 67:   static std::pair UnpackValues(const 
uint8_t* __restrict__ in,
Could this one be private?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.h@81
PS8, Line 81:   static std::pair UnpackAndDecodeValues(
Could this one be private?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.h@86
PS8, Line 86:   /// Unpack exactly 32 values of 'bit_width' from 'in' to 'out'. 
'in' must point to
Is there a benefit of having those public over making them private and make the 
tests that use them a friend?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.h@114
PS8, Line 114: 'bit_width'
Is this comment still correct? From looking at the implementation I also 
thought that this returns at most 31 values and that the caller has to make 
sure to set num_values accordingly.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.inline.h
File be/src/util/bit-packing.inline.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-packing.inline.h@56
PS8, Line 56: return UnpackValues(in, in_bytes, num_values, 
out);
nit: I think adding a blank line between the definition of the macro and 
calling it may increase readability. Feel free to ignore though.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-stream-utils.h
File be/src/util/bit-stream-utils.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-stream-utils.h@33
PS8, Line 33: /// TODO: replace uses with the more efficient BatchedBitReader.
Would that mean replacing the Writer with the BatchedReader?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-stream-utils.inline.h
File be/src/util/bit-stream-utils.inline.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/bit-stream-utils.inline.h@115
PS8, Line 115:   auto result = BitPacking::UnpackAndDecodeValues(bit_width, 
buffer_pos_,
You could use std::tie to unpack the result.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h
File be/src/util/rle-encoding.h:

http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@93
PS8, Line 93:   uint32_t NextNumRepeats();
Could we convert all the uint32_t to int64_t while we're here? There's already 
a JIRA to do this and I think we generally try to avoid unsigned types unless 
there's a specific reason.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@105
PS8, Line 105:   /// copying the values to 'values'.
Can you add a comment on the return value, here and below?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@134
PS8, Line 134: decoded
nit: decode


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@137
PS8, Line 137:   static constexpr int LITERAL_BUFFER_LEN = 32;
out of curiosity, does this need constexpr instead of const? The latter should 
be sufficient to specify the array size below.


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@145
PS8, Line 145: exhaustive
nit: exhausted?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@153
PS8, Line 153:   bool FillLiteralBuffer() WARN_UNUSED_RESULT;
Do we give any guarantees what the caller can do after this returns false? 
Should we mention that they have to call Reset() to get the object back into a 
usable state?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@159
PS8, Line 159:   /// Output buffered literals and decrement 'literal_count_'.
Should we mention that it also increments 'literal_buffer_pos_'?

What is the return value?


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@494
PS8, Line 494: boundery
nit: typo


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@498
PS8, Line 498: int num_read = bit_reader_.UnpackBatch(bit_width_, 
num_to_bypass, values + num_consumed);
nit: long line


http://gerrit.cloudera.org:8080/#/c/8267/8/be/src/util/rle-encoding.h@585
PS8, Line 585: if (UNLIKELY(literal_count_ == 0)) return;
This line doesn't seem to have any effect? Is it there for perf reasons? If so, 
can you add a brief comment?


http://gerrit.cloudera.org:8080/#/c/8267/7/testdata/data/README
File testdata/data/README:

http://gerrit.cloudera.org:8080/#/c/8267/7/testdata/data/README@121
PS7, Line 121: write
nit: writer

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 7:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py
File shell/option_parser.py:

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@58
PS7, Line 58: return config_filename
> Thanks, that is a bug actually.
As discussed in person, can you please add a test for this case?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Thu, 19 Oct 2017 22:40:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4918: Support getting column comments via HS2

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4918: Support getting column comments via HS2
..


Patch Set 3: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d33dfd031b5344d7136695b623cec76143ada5c
Gerrit-Change-Number: 8315
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 19 Oct 2017 17:16:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6067: Enable s3 access via IAM roles for EC2 VMs
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh
File bin/impala-config.sh:

http://gerrit.cloudera.org:8080/#/c/8294/1/bin/impala-config.sh@256
PS1, Line 256: if [[ -z ${AWS_ACCESS_KEY_ID-} ]]; then
We may want to include a check here that "set -x" has not been enabled?

# Prevent leaking the AWS keys to the log
set_x=0
if set +o | grep -q "set -o xtrace"; then
  set_x=1
  set +x
fi

DO STUFF

# Restore xtrace flag
if [[ $set_x -eq 1 ]]; then
  set -x
fi



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I14cd9d4453a91baad3c379aa7e4944993fca95ae
Gerrit-Change-Number: 8294
Gerrit-PatchSet: 1
Gerrit-Owner: Laszlo Gaal 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laszlo Gaal 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Wed, 18 Oct 2017 17:32:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4918: Support getting column comments via HS2

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4918: Support getting column comments via HS2
..


Patch Set 2:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8315/2//COMMIT_MSG@9
PS2, Line 9: Fill the 'comments'/'remarks' field during HS2 column metadata 
requests.
Please include in the commit message how you tested this. I think this also 
should include tests for other tables formats (Kudu, HBase). Those could be end 
to end tests. It would also be good to re-visit the existing "ALTER TABLE" 
tests and make sure they test adding comments.


http://gerrit.cloudera.org:8080/#/c/8315/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
File fe/src/test/java/org/apache/impala/common/FrontendTestBase.java:

http://gerrit.cloudera.org:8080/#/c/8315/2/fe/src/test/java/org/apache/impala/common/FrontendTestBase.java@189
PS2, Line 189: dummyTable.addColumn(new Column(colDef.getColName(), 
colDef.getType(), colDef.getComment(), i));
Long line.


http://gerrit.cloudera.org:8080/#/c/8315/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java
File fe/src/test/java/org/apache/impala/service/JdbcTest.java:

http://gerrit.cloudera.org:8080/#/c/8315/2/fe/src/test/java/org/apache/impala/service/JdbcTest.java@460
PS2, Line 460: assertEquals("Incorrect table comment", "", 
rs.getString("REMARKS"));
This looks like a bug in getTables() to me, but Alex and Dimitris would know 
better.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I1d33dfd031b5344d7136695b623cec76143ada5c
Gerrit-Change-Number: 8315
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Wed, 18 Oct 2017 16:44:55 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 7:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py
File shell/impala_shell.py:

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@686
PS7, Line 686: tmp_set
"_set" implies this is a set, but it's actually a dict. I think 
"new_query_options" might be a better name.


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/impala_shell.py@1332
PS7, Line 1332:   There are two types of options: shell options and 
query_options. Both can be set in
nit: missing article


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py
File shell/option_parser.py:

http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@28
PS7, Line 28: # EXPLAIN_LEVEL=2
are these case sensitive? Otherwise I'd opt for consistency with the previous 
section.


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@36
PS7, Line 36: def convert_loaded_shell_option(option, value, default_options, 
config_filename):
:   """Converts some values based on their type in default_options
:
:  Setting 'config_filename' in the config file would have no 
effect,
:  so its original value is kept.
From looking at the signature and the comment I have no idea what this method 
does. Can you clarify it so it makes sense without further context? The comment 
also should explain what the return value is.


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@42
PS7, Line 42: if default_options[option] in [True, False]:
How about "if type(...) is bool:" ?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@58
PS7, Line 58: return config_filename
What if none of the cases match?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@66
PS7, Line 66:   filtered and some values are converted (False, True, None).
Can you explain what exactly gets converted into what? Strings into Python 
values?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@72
PS7, Line 72:   Returns a pair of dictionaries (shell_options, query_options)
Can you add a comment explaining what the keys and values of those dictionaries 
are?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@91
PS7, Line 91: upper
Is this needed? If so, can you add a comment explaining why?


http://gerrit.cloudera.org:8080/#/c/8038/7/shell/option_parser.py@167
PS7, Line 167:   "The following sections are used: 
[impala], "
I think we should mention that the section titles are case sensitive



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-Change-Number: 8038
Gerrit-PatchSet: 7
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: anujphadke 
Gerrit-Comment-Date: Tue, 17 Oct 2017 23:08:54 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6049: breakpad tests: skip all tests with local filesystem

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6049: breakpad tests: skip all tests with local 
filesystem
..


Patch Set 1: Code-Review+2

Thanks for fixing this. I fixed it in the same way but couldn't get a local 
build+test to run for unrelated reasons.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib4a3ff29dd85c79c4c3b3e3afb699861e408aa95
Gerrit-Change-Number: 8272
Gerrit-PatchSet: 1
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Fri, 13 Oct 2017 17:09:33 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613:   if (seconds < numeric_limits::min() ||
 :   seconds > numeric_limits::max()) {
 : // TimeStampVal() takes int64_t.
 : return TimestampVal::null();
> I have checked with compiler explorer, and both clang 3.9.1 and gcc 4.9.2
Yes



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 12 Oct 2017 16:42:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5983: Fix crash in to/from utc timestamp("10:00:00", 'MSK')

[Lars Volker (Code Review)]

Lars Volker has submitted this change and it was merged. ( 
http://gerrit.cloudera.org:8080/8139 )

Change subject: IMPALA-5983: Fix crash in to/from_utc_timestamp("10:00:00", 
'MSK')
..

IMPALA-5983: Fix crash in to/from_utc_timestamp("10:00:00", 'MSK')

Moscow timezone is handled differrently than other timezones,
and it was possible to cause unhandled boost exception by
trying to convert "dateless" timestamps like "10:00:00".

These timestamps cannot be handled by timestamp conversion
generally, because daylight saving time logic needs month
and day to work correctly. The condition HasDateOrTime()
incorrectly suggested that these timestamps can be handled,
so I made the condition stricter.

Change-Id: I592389333a16a15a680beed389e2702dfc16fe1d
Reviewed-on: http://gerrit.cloudera.org:8080/8139
Tested-by: Impala Public Jenkins
Reviewed-by: Lars Volker 
---
M be/src/exprs/expr-test.cc
M be/src/exprs/timestamp-functions.cc
2 files changed, 7 insertions(+), 2 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Lars Volker: Looks good to me, approved

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I592389333a16a15a680beed389e2702dfc16fe1d
Gerrit-Change-Number: 8139
Gerrit-PatchSet: 6
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-5983: Fix crash in to/from utc timestamp("10:00:00", 'MSK')

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5983: Fix crash in to/from_utc_timestamp("10:00:00", 
'MSK')
..


Patch Set 5: Code-Review+2

Carrying my +2 after the rebase.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I592389333a16a15a680beed389e2702dfc16fe1d
Gerrit-Change-Number: 8139
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 12 Oct 2017 16:39:25 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-2494: Support for byte array encoded decimals in Parquet scanner

[Lars Volker (Code Review)]

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

Change subject: IMPALA-2494: Support for byte array encoded decimals in Parquet 
scanner
..


Patch Set 2:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-column-stats.inline.h
File be/src/exec/parquet-column-stats.inline.h:

http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-column-stats.inline.h@89
PS2, Line 89:   switch(parquet_type){
> Do you mean to have a Decode wrapper around the templatized Decode methods?
The former, so that the interface of Decode() is simpler. How this is 
implemented seems more a concern of the decoder than the column stats.


http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h
File be/src/exec/parquet-common.h:

http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h@237
PS2, Line 237: ByteSize
> Do you mean to have something like
I think this particular call here will always return sizeof(int32_t) (line 
220). I'd just put that here, since your change explicitly documents that as a 
template parameter.


http://gerrit.cloudera.org:8080/#/c/7822/2/be/src/exec/parquet-common.h@338
PS2, Line 338: template <>
> thats kinda difficult because DecimalUtil::DecodeFromFixedLenByteArray is a
I'm not sure I'm following: it looks like the next three methods are exactly 
the same. Couldn't you move them into a new method DecodeDecimalValue(const 
uint8_t* buffer, const uint8_t* buffer_end, int fixed_len_size, T* v) and then 
call it and return in one line here? I may be missing something though :)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I2c0e881045109f337fecba53fec21f9cfb9e619e
Gerrit-Change-Number: 7822
Gerrit-PatchSet: 2
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 12 Oct 2017 00:33:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..


Patch Set 4: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:36:32 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613:   if (seconds < numeric_limits::min() ||
 :   seconds > numeric_limits::max()) {
 : // TimeStampVal() takes int64_t.
 : return TimestampVal::null();
> Good question - my guess is that the compiler is smart enough to eliminate
Looks like it should, but it may be worth confirming, e.g. with the compiler 
explorer.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Wed, 11 Oct 2017 18:36:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators-ir.cc@613
PS3, Line 613:   if (seconds < numeric_limits::min() ||
 :   seconds > numeric_limits::max()) {
 : // TimeStampVal() takes int64_t.
 : return TimestampVal::null();
Is this branch now also evaluated for Decimal[4/8]Values? If so, will it have a 
perf impact?


http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h
File be/src/exprs/decimal-operators.h:

http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h@166
PS3, Line 166: //
nit: Keep comment formatting consistent with the rest of the file.


http://gerrit.cloudera.org:8080/#/c/8051/3/be/src/exprs/decimal-operators.h@168
PS3, Line 168: TDecimal
This naming convention usually indicates Thrift structures throughout the 
codebase. Did you follow some other example here? Otherwise "const T& 
decimal_value" seems more consistent.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Wed, 11 Oct 2017 15:21:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6023: Fix broken breakpad test

[Lars Volker (Code Review)]

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

Change subject: IMPALA-6023: Fix broken breakpad test
..


Patch Set 4:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8240/3/tests/custom_cluster/test_breakpad.py
File tests/custom_cluster/test_breakpad.py:

http://gerrit.cloudera.org:8080/#/c/8240/3/tests/custom_cluster/test_breakpad.py@149
PS3, Line 149: @pytest.mark.execute_serially
> This was here due to needing DCHECK behavior before, right? What about abor
Done


http://gerrit.cloudera.org:8080/#/c/8240/3/tests/custom_cluster/test_breakpad.py@174
PS3, Line 174:
> Change to:
Done


http://gerrit.cloudera.org:8080/#/c/8240/3/tests/custom_cluster/test_breakpad.py@333
PS3, Line 333:
> Some blank lines at the end of the file.
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ifb5af3e72963280a6677a99aa6a0e5785443bb0c
Gerrit-Change-Number: 8240
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 
Gerrit-Comment-Date: Tue, 10 Oct 2017 17:26:24 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6023: Fix broken breakpad test

[Lars Volker (Code Review)]

Hello Michael Brown, Sailesh Mukil,

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

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

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

Change subject: IMPALA-6023: Fix broken breakpad test
..

IMPALA-6023: Fix broken breakpad test

We have a test to make sure that hitting a DCHECK will write a minidump.
We used to pass "-beeswax_port=1" to the server to trigger a DCHECK. A
while ago, this DCHECK seems to have been removed, but we still called
abort() if the ImpalaServer failed to start. This masked the slightly
altered behavior and the test still succeeded.

However, the fix for IMPALA-4786 changed the behavior to call exit(1)
instead of abort() if the ImpalaServer failed to start.

To fix the test, we change it to pass an unresolvable hostname to
impalad, which will result in a call to abort().

This change also splits the breakpad tests into core and exhaustive sets
to make sure that tests which depend on other parts of Impala are
included in every core run.

Change-Id: Ifb5af3e72963280a6677a99aa6a0e5785443bb0c
---
M tests/custom_cluster/test_breakpad.py
1 file changed, 33 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ifb5af3e72963280a6677a99aa6a0e5785443bb0c
Gerrit-Change-Number: 8240
Gerrit-PatchSet: 4
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Sailesh Mukil 

[Impala-ASF-CR] IMPALA-5983: Fix crash in to/from utc timestamp("10:00:00", 'MSK')

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5983: Fix crash in to/from_utc_timestamp("10:00:00", 
'MSK')
..


Patch Set 4: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I592389333a16a15a680beed389e2702dfc16fe1d
Gerrit-Change-Number: 8139
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 09 Oct 2017 23:15:31 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 14: Code-Review+2

This has 4 +1's now, I'm aggregating them into a +2.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 14
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 09 Oct 2017 23:11:00 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-6023: Fix broken breakpad test

[Lars Volker (Code Review)]

Lars Volker has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/8240


Change subject: IMPALA-6023: Fix broken breakpad test
..

IMPALA-6023: Fix broken breakpad test

We have a test to make sure that hitting a DCHECK will write a minidump.
We used to pass "-beeswax_port=1" to the server to trigger a DCHECK. A
while ago, this DCHECK seems to have been removed, but we still called
abort() if the ImpalaServer failed to start. This masked the slightly
altered behavior and the test still succeeded.

However, the fix for IMPALA-4786 changed the behavior to call exit(1)
instead of abort() if the ImpalaServer failed to start.

To fix the test, we change it to pass an unresolvable hostname to
impalad, which will result in a call to abort().

This change also splits the breakpad tests into core and exhaustive sets
to make sure that tests which depend on other parts of Impala are
included in every core run.

Change-Id: Ifb5af3e72963280a6677a99aa6a0e5785443bb0c
---
M tests/custom_cluster/test_breakpad.py
1 file changed, 35 insertions(+), 22 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ifb5af3e72963280a6677a99aa6a0e5785443bb0c
Gerrit-Change-Number: 8240
Gerrit-PatchSet: 3
Gerrit-Owner: Lars Volker 

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..


Patch Set 2:

(6 comments)

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

http://gerrit.cloudera.org:8080/#/c/8051/2//COMMIT_MSG@9
PS2, Line 9: The timestamp conversion from negative fractional Decimal types
   : (interpreted as unix timestamp) was wrong - the whole part
   : was rounded toward zero, and fractional part was being added
   : instead of being subtracted.
The commit message should include a brief description of how change fixes it.


http://gerrit.cloudera.org:8080/#/c/8051/2//COMMIT_MSG@14
PS2, Line 14: Example for the wrong behaivour:
Spelling. Please consider setting up an automatic spell checker in your text 
editor.


http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc
File be/src/exprs/decimal-operators-ir.cc:

http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc@617
PS2, Line 617:   if(dv.is_negative()) nanoseconds *= -1;
I think it might be easier to read if you extract the sign in in line 610 and 
then just multiply nanoseconds with the sign during its initialization or in 
the call to FromUnixTimeNanos().


http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc@625
PS2, Line 625: int64_t
why is this 64bit?


http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/decimal-operators-ir.cc@640
PS2, Line 640: int128_t
why is this 128bit?


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

http://gerrit.cloudera.org:8080/#/c/8051/2/be/src/exprs/expr-test.cc@2363
PS2, Line 2363:   TestTimestampValue("cast(cast(-123.45 as decimal(9,2)) as 
timestamp)",
If you include a negative example in the commit message, it should also be 
included in the tests.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-Change-Number: 8051
Gerrit-PatchSet: 2
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 09 Oct 2017 19:58:38 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5529: [DOCS] New trunc() signatures

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5529: [DOCS] New trunc() signatures
..


Patch Set 4:

I lack context here, but I'll ask Thomas to have another look. He was one of 
the reviewers of the original change.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ice4753dee4f7b8e09c35508a9cad1e36f4ab2826
Gerrit-Change-Number: 8189
Gerrit-PatchSet: 4
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Kim Jin Chul 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 06 Oct 2017 19:29:40 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3504: [DOCS] Document utc timestamp()

[Lars Volker (Code Review)]

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

Change subject: IMPALA-3504: [DOCS] Document utc_timestamp()
..


Patch Set 1: Code-Review+2

This looks good to me. Zoltan should also have a look at this. Unfortunately 
he's in CET so I suggest to move on with this and then fix any issues he finds 
in a subsequent commit.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2662fc79d588f22a24a5067429a57b3c0d0f0f0
Gerrit-Change-Number: 8190
Gerrit-PatchSet: 1
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 06 Oct 2017 19:02:24 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3504: [DOCS] Document utc timestamp()

[Lars Volker (Code Review)]

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

Change subject: IMPALA-3504: [DOCS] Document utc_timestamp()
..


Patch Set 1:

> OK, looks like the those other *utc* names are not directly
 > related. Let's confine this gerrit to strictly the utc_timestamp()
 > function.

John, will you push a new PS here or is this waiting for review?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ia2662fc79d588f22a24a5067429a57b3c0d0f0f0
Gerrit-Change-Number: 8190
Gerrit-PatchSet: 1
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Fri, 06 Oct 2017 17:27:16 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 10:

I'd like to give Dan and Alex the chance to have a look, too.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-Change-Number: 7805
Gerrit-PatchSet: 10
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Sat, 30 Sep 2017 00:20:31 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 5: Code-Review+2


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 5
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:52:58 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4736: Add SIGUSR1 behavior to help string for 'minidump path' flag

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4736: Add SIGUSR1 behavior to help string for 
'minidump_path' flag
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8164/4/be/src/common/global-flags.cc
File be/src/common/global-flags.cc:

http://gerrit.cloudera.org:8080/#/c/8164/4/be/src/common/global-flags.cc@103
PS4, Line 103: only
Remove "only", since it conflicts with the following sentence.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I8f4da8ff12c10e4a84a339ddb4036d1682b3a2a0
Gerrit-Change-Number: 8164
Gerrit-PatchSet: 4
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Fri, 29 Sep 2017 16:32:57 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5994: Failure in star expansion on struct fields

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5994: Failure in star expansion on struct fields
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 2
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Comment-Date: Thu, 28 Sep 2017 22:36:48 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5994: Failure in star expansion on struct fields

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5994: Failure in star expansion on struct fields
..


Patch Set 1:

(3 comments)

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

http://gerrit.cloudera.org:8080/#/c/8169/1/fe/src/main/java/org/apache/impala/catalog/StructField.java@37
PS1, Line 37: name_ = name.toLowerCase();
Can you add a brief comment why we need to do this?


http://gerrit.cloudera.org:8080/#/c/8169/1/tests/query_test/test_nested_types.py
File tests/query_test/test_nested_types.py:

http://gerrit.cloudera.org:8080/#/c/8169/1/tests/query_test/test_nested_types.py@96
PS1, Line 96: fielD
nit: This seems easy to miss. Can you pick something more obvious, like 
camelField?


http://gerrit.cloudera.org:8080/#/c/8169/1/tests/query_test/test_nested_types.py@100
PS1, Line 100: F
Does the uppercase F matter here?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iacd9714ac2301a55ee8b64f0102f6f156fb0370e
Gerrit-Change-Number: 8169
Gerrit-PatchSet: 1
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 28 Sep 2017 21:58:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5983: Fix crash in to/from utc timestamp("10:00:00", 'MSK')

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5983: Fix crash in to/from_utc_timestamp("10:00:00", 
'MSK')
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8139/3/tests/query_test/test_timezones.py
File tests/query_test/test_timezones.py:

http://gerrit.cloudera.org:8080/#/c/8139/3/tests/query_test/test_timezones.py@45
PS3, Line 45: # timestamp conversions of "dateless" times should return 
null (and not crash,
As discussed in person, please move the tests to test-exprs.cc.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I592389333a16a15a680beed389e2702dfc16fe1d
Gerrit-Change-Number: 8139
Gerrit-PatchSet: 3
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 28 Sep 2017 21:19:21 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5307: part 1: don't transfer disk I/O buffers out of 
parquet
..


Patch Set 6: Code-Review+1

(2 comments)

http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/hdfs-parquet-scanner.cc@245
PS5, Line 245:   context_->ReleaseCompletedResources(nullptr, true);
> I can obviously fix it if you feel strongly but it just doesn't seem that i
Ah, I understand.


http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h
File be/src/exec/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h@477
PS5, Line 477:   int64_t size, const char* err_ctx, uint8_t** buffer);
> Done. Also switch to const char* so we don't need to create a std::string f
Thx. ctx somewhat reminds me of the various context objects that we use 
elsewhere, but I don't feel strongly about it.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2
Gerrit-Change-Number: 8085
Gerrit-PatchSet: 6
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 27 Sep 2017 16:20:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5983: Fix crash in to/from utc timestamp("10:00:00", 'MSK')

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5983: Fix crash in to/from_utc_timestamp("10:00:00", 
'MSK')
..


Patch Set 1:

Thanks for fixing this. Can you add some tests?


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I592389333a16a15a680beed389e2702dfc16fe1d
Gerrit-Change-Number: 8139
Gerrit-PatchSet: 1
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Tue, 26 Sep 2017 16:16:19 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5307: part 1: don't transfer disk I/O buffers out of parquet

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5307: part 1: don't transfer disk I/O buffers out of 
parquet
..


Patch Set 5:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/8085/5//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/8085/5//COMMIT_MSG@7
PS5, Line 7: IMPALA-5307
Can you add a line at the bottom what the other part(s) would look like?


http://gerrit.cloudera.org:8080/#/c/8085/5//COMMIT_MSG@56
PS5, Line 56: 
+++---++-++++-+---+
Nit: You could make the second column smaller to make this more readable, and 
add a bottom delimiter line to indicate it was truncated on purposed and not by 
mistake.


http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/hdfs-parquet-scanner.cc
File be/src/exec/hdfs-parquet-scanner.cc:

http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/hdfs-parquet-scanner.cc@245
PS5, Line 245:   context_->ReleaseCompletedResources(nullptr, true);
I think it's best to change the whole file at once, or only change occurrences 
where necessary. This looks like it may be left from a previous patchset.


http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h
File be/src/exec/parquet-column-readers.h:

http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h@476
PS5, Line 476:   Status AllocateUncompressedDataPage(
Should we call this "AllocateUncompressedDataBuffer"? Otherwise it sounds to me 
like it'll only be needed for uncompressed pages.


http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h@477
PS5, Line 477:   int64_t size, const std::string& desc, uint8_t** buffer);
Maybe err_desc, err_detail, or detail? "desc" reminds me of descriptors.


http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.h@485
PS5, Line 485: IsStringType
This does not say "VarLenStringType" but above in a comment you refer to 
var-len data. Can you clarify one of them?


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

http://gerrit.cloudera.org:8080/#/c/8085/5/be/src/exec/parquet-column-readers.cc@1075
PS5, Line 1075:   uncompressed_size, "uncompressed variable-length 
data", _buffer));
DCHECK(copy_buffer != nullptr); And maybe initialize it to nullptr, so that 
it's explicit what the allocation will do.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I767c1e2dabde7d5bd7a4d5c1ec6d14801b8260d2
Gerrit-Change-Number: 8085
Gerrit-PatchSet: 5
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Mon, 25 Sep 2017 23:30:35 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5965: avoid per-value switch on NeedsConversionInline() in parquet

[Lars Volker (Code Review)]

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

Change subject: IMPALA-5965: avoid per-value switch on NeedsConversionInline() 
in parquet
..


Patch Set 2: Code-Review+1


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I04fb4ca73978d0100e1eb9835b87d37540b8b645
Gerrit-Change-Number: 8117
Gerrit-PatchSet: 2
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Lars Volker 
Gerrit-Comment-Date: Thu, 21 Sep 2017 23:06:44 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 5: Code-Review+2

Rebased, carrying Tim's +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 3:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/8049/3/bin/clean.sh
File bin/clean.sh:

Line 33: pushd ${IMPALA_HOME}/ext-data-source
> Some of these are still missing quotes
Thank you for catching these. I had only fixed the {}.


Line 35: ${IMPALA_HOME}/bin/mvn-quiet.sh clean
> Here
Done


Line 40: pushd ${IMPALA_FE_DIR}
> Here too.
Done


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

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

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..

IMPALA-5927: Fix enable_distcc for zsh

enable_distcc didn't work on zsh anymore since it relies on automatic
variable splitting, which only works in bash.

This change moves clean-up actions for cmake files into an own bash
script.

This change also unifies variable quoting in clean.sh.

Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
---
A bin/clean-cmake.sh
M bin/clean.sh
M bin/distcc/distcc_env.sh
3 files changed, 50 insertions(+), 23 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 2:

(2 comments)

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

Line 12: This change makes zsh temporarily emulate sh to make enable_distcc work
> Update
Done


http://gerrit.cloudera.org:8080/#/c/8049/2/bin/clean.sh
File bin/clean.sh:

Line 77: $IMPALA_HOME/bin/clean-cmake.sh
> nit: maybe quote "$IMPALA_HOME" for consistency
I cleaned up the file. Do you want to have another look?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..

IMPALA-5927: Fix enable_distcc for zsh

enable_distcc didn't work on zsh anymore since it relies on automatic
variable splitting, which only works in bash.

This change moves clean-up actions for cmake files into an own bash
script.

This change also unifies variable quoting in clean.sh.

Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
---
A bin/clean-cmake.sh
M bin/clean.sh
M bin/distcc/distcc_env.sh
3 files changed, 49 insertions(+), 22 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 7:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> Do you mean replacing the make_pair in L142 or replacing the make_tuple in 
See my reply below.


Line 171:   });
> I'm not sure how these are related. I think value-parameterized-tests is us
I thought that every CASE would be a parameter value, stored as a struct, 
templatized if necessary.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 7:

(5 comments)

I'm still trying to see if there are ways to simplify the code with patterns we 
use more commonly. If you disagree, let's catch up in person, since I'm not set 
on what's the right way forward here.

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

PS6, Line 137:   };
 :   TestByteCaseSet(options, case_set_i64);
 :   TestByteCaseSet(options, case_set_i32);
 : }
 : 
 : // Test an enum testcase. May or may not accept integer
 : template
 : void TestEnumCase(TQueryOptions& options,
> I'm not sure which part I should look at. The complication here is that I n
Can you use a templatiezed class to store the test cases instead of a tuple? 
Additionally you could use a small factory method to hide the macros.


Line 171:   });
> Not that easy. Let me explain:
Could you use 
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md#value-parameterized-tests
 with a templatized parameter class to achieve the same?


http://gerrit.cloudera.org:8080/#/c/7805/7/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 79: auto ok = MakeOk(options, test_case.first);
I think it would be helpful to see that these are functions here. Can you 
remove the auto?


Line 81: auto lb = test_case.second.first;
Do these need to be auto?


Line 103: for (auto& value : common_value) {
const? Please check elsewhere, too.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#2).

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..

IMPALA-5927: Fix enable_distcc for zsh

enable_distcc didn't work on zsh anymore since it relies on automatic
variable splitting, which only works in bash.

This change makes zsh temporarily emulate sh to make enable_distcc work
again. I tested it on zsh and bash.

Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
---
A bin/clean-cmake.sh
M bin/clean.sh
M bin/distcc/distcc_env.sh
3 files changed, 37 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8049/1/bin/distcc/distcc_env.sh
File bin/distcc/distcc_env.sh:

Line 119:   if type emulate >/dev/null 2>/dev/null; then emulate -L sh; fi
> This logic is also duplicated in clean.sh. How about factoring this functio
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 1:

MJ, do you prefer one option with a comma separated list of key=value pairs, or 
using several options similar to --var ?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 6:

(9 comments)

Thank you for working on this. I left some comments inline. Overall I feel that 
we're still not completely decided on which new features of C++11 and new 
paradigms they allow we want to use. I'd be happy to follow up on this on dev@.

http://gerrit.cloudera.org:8080/#/c/7805/6/be/src/service/query-options-test.cc
File be/src/service/query-options-test.cc:

Line 20: #include 
We are generally trying to reduce our dependencies on Boost libraries. Can you 
replace some of these with C++11 equivalents (e.g. for_each))?


Line 39: constexpr auto i32_max = numeric_limits::max();
We currently use auto only for iterators and const& in loops.


Line 73: TEST(QueryOptions, SetByteOptions) {
Can you add a comment explaining what the test does overall? Same for the other 
tests.


Line 96:   auto process_case_set = [&](auto& case_set) {
I find this part hard to follow and its use of lambdas seems to deviate from 
our current best practices somewhat. Can you try to refactor some parts using 
plain functions to generate context objects instead?


PS6, Line 97: kase
Can you think of a better name?


PS6, Line 99: minus_1_to_0
Why is this needed? Is the added level of indirection worth it?


PS6, Line 137:   // Expands to {"entry", prefix::entry},
 : #define ENTRY(_, prefix, entry) {BOOST_PP_STRINGIZE(entry), 
prefix::entry},
 :   // ENTRIES(prefix, (a)(b)) expands to {"a", prefix::a}, {"b", 
prefix::b},
 : #define ENTRIES(prefix, name) BOOST_PP_SEQ_FOR_EACH(ENTRY, 
prefix, name)
 :   // A case is a pair: (keydef, [(enum_string, enum_value)])
 : #define CASE(key, enumtype, enums) make_pair(key, \
 :   vector\
 :   {ENTRIES(enumtype, BOOST_PP_TUPLE_TO_SEQ(enums))})
I find this very hard to understand. Can you have a look at the Google Test 
documentation on how to define test cases?


Line 171: fusion::for_each(case_set, [, accept_integer](auto& kase) 
{
Can you use C++11 for_each instead?


Line 263: for (auto& key_def : {key_def_min, key_def_default}) {
const auto&?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tianyi Wang 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4850 [DOCS] Create table "comment comes after "partioned by"

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4850 [DOCS] Create table "comment comes after "partioned 
by"
..


Patch Set 2:

> Lars, did you clarify this with Laurel? What to do with this
 > change?

I think I missed this comment as I wasn't added as a reviewer on this change. 
Apologies for the delay.

SORT BY (a) requires it's ordering columns to be in parentheses, just like 
PARTITIONED BY(a).

Laurel, how can I help? Do you want to discuss the change in person?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I543ff1dbfe1ab8a7e0a0a668130ab060e3af0a5f
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Laurel Hale 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Laurel Hale 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5425: Add test for validating input when setting query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5425: Add test for validating input when setting query 
options
..


Patch Set 6:

Is this ready for review?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I510e02bb0776673d8cbfc22b903831882c6908d7
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tianyi Wang 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] Increment version to 2.11.0-SNAPSHOT

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: Increment version to 2.11.0-SNAPSHOT
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I2a60fbc5f2c1a9ba9697e6f1114bdf18997aa92c
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5664: Unix time to timestamp conversions may crash Impala

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5664: Unix time to timestamp conversions may crash Impala
..


Patch Set 2: Code-Review+1

Thanks for fixing this. LGTM, let's see what others say.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I77b2f6284d3a597f57e61c17a67c959eff9e38ff
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5736: Add impala-shell argument to set default query options

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5736: Add impala-shell argument to set default query 
options
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8038/1/shell/option_parser.py
File shell/option_parser.py:

PS1, Line 160:   parser.add_option("--var", dest="keyval", action="append",
 : help="Define variable(s) to be used within 
the Impala session."
 :  " It must follow the pattern 
\"KEY=VALUE\","
 :  " KEY starts with an alphabetic 
character and"
 :  " contains alphanumeric characters or 
underscores.")
 :   parser.add_option("--query_options", dest="query_options",
 : help="Sets default query options. The format 
is comma separated"
 :   
> We have talked about this with Lars, and decided to stick with the comma se
My thinking was that we already follow the k=v,k=v syntax for tools like 
start-impala-cluster.py. However I see how consistency with --var is a nice 
thing to have and I'm ok with picking that route.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I26a3b67230c80a99bd246b6af205d558fec9a986
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/3//COMMIT_MSG
Commit Message:

Line 7: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
> We should fix this msg, not correct anymore, sorry
Ah, I saw this too late and the change got submitted in the meantime. I don't 
think it warrants a force push, let me know if you think otherwise. Feel free 
to -2 my changes if I miss something similar in the future. :)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 3: Code-Review+2

Carrying Alex's +2.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/2/be/src/service/child-query.cc
File be/src/service/child-query.cc:

Line 139: VLOG_QUERY << status;
> VLOG_QUERY << "Cancelling and closing child query. Failed to get query id: 
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

[Lars Volker (Code Review)]

Hello Alex Behm,

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

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

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

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..

IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

ChildQuery::Cancel() prints a binary ID into the log which can show up
as random characters. One fix is to print it as a hex string.

I tested this by running test_cancellation::test_cancel_insert and
making sure the ID is printed as hex.

This change also removes PrintAsHex() which was broken and unused.

Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
---
M be/src/service/child-query.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
3 files changed, 10 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..


Patch Set 1:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/8050/1/be/src/service/child-query.cc
File be/src/service/child-query.cc:

Line 131:   const string& guid = hs2_handle_.operationId.guid;
> Why not use ImpalaServer::THandleIdentifierToTUniqueId() to convert to a TU
Thanks, I didn't know what one.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

[Lars Volker (Code Review)]

Lars Volker has uploaded a new patch set (#2).

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..

IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

ChildQuery::Cancel() prints a binary ID into the log which can show up
as random characters. One fix is to print it as a hex string.

I tested this by running test_cancellation::test_cancel_insert and
making sure the ID is printed as hex.

This change also removes PrintAsHex() which was broken and unused.

Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
---
M be/src/service/child-query.cc
M be/src/util/debug-util.cc
M be/src/util/debug-util.h
3 files changed, 9 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Alex Behm 

[Impala-ASF-CR](asf-site) Update download and signature links for 2.10.0 release.

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: Update download and signature links for 2.10.0 release.
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id29b30a06d4e0f64c08460cc9e58688ea8bf3f8d
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: Bharath Vissapragada 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5668: Fix cast(X as timestamp) for negative subsecond Decimals

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5668: Fix cast(X as timestamp) for negative subsecond 
Decimals
..


Patch Set 1:

As discussed via email, let's add tests after IMPALA-5664 has been fixed.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I8216f4c0f100c1bd68891cd6048236bfe4c205f0
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Csaba Ringhofer 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

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

Change subject: IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()
..

IMPALA-5923: Print binary ID as hex in ChildQuery::Cancel()

ChildQuery::Cancel() prints a binary ID into the log which can show up
as random characters. One fix is to print it as a hex string.

I tested this by running test_cancellation::test_cancel_insert and
making sure the ID is printed as hex.

Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
---
M be/src/service/child-query.cc
M be/src/util/debug-util.cc
2 files changed, 3 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ie1a9516d5c03524e2585255700bb84e8a301a7ee
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 

[Impala-ASF-CR] IMPALA-5927: Fix enable distcc for zsh

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

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

Change subject: IMPALA-5927: Fix enable_distcc for zsh
..

IMPALA-5927: Fix enable_distcc for zsh

enable_distcc didn't work on zsh anymore since it relies on automatic
variable splitting, which only works in bash.

This change makes zsh temporarily emulate sh to make enable_distcc work
again. I tested it on zsh and bash.

Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
---
M bin/distcc/distcc_env.sh
1 file changed, 2 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I88284e4f68c309bb46ce4b5a842ccc576cd487ed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 

[Impala-ASF-CR] IMPALA-5890: Abort queries if scanner hits IO errors

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5890: Abort queries if scanner hits IO errors
..


Patch Set 10:

(2 comments)

Changed to INTERNAL_ERROR for the possible error in GetNextBuffer().

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

PS10, Line 200: DISK_IO_ERROR
> Seems to me that INTERNAL_ERROR should be in the list of unrecoverable erro
Done


PS10, Line 200: DISK_IO_ERROR
> Makes sense to me.
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5890: Abort queries if scanner hits IO errors

[Lars Volker (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5890: Abort queries if scanner hits IO errors
..

IMPALA-5890: Abort queries if scanner hits IO errors

Prior to this fix, an error in ScannerContext::Stream::GetNextBuffer()
could leave the stream in an inconsistent state:

- The DiskIoMgr hits EOF unexpected, cancels the scan range and enqueues
a buffer with eosr set.
- The ScannerContext::Stream tries to read more bytes, but since it has
hit eosr, it tries to read beyond the end of the scan range using
DiskIoMgr::Read().
- The previous read error resulted in a new file handle being opened.
The now truncated, smaller file causes the seek to fail.
- Then during error handling, the BaseSequenceScanner calls SkipToSync()
and trips over the NULL pointer in in the IO buffer.

In my reproduction this only happens with the file handle cache enabled,
which causes Impala to see two different sized handles: the one from the
cache when the query starts, and the one after reopening the file.

To fix this, we change the I/O manager to always return DISK_IO_ERROR
for errors and we abort a query if we receive such an error in the
scanner.

This change also fixes GetBytesInternal() to maintain the invariant that
the output buffer points to the boundary buffer whenever the latter
contains some data.

I tested this by running the repro from the JIRA and impalad did not
crash but aborted the queries. I also ran the repro with
abort_on_error=1, and with the file handle cache disabled.

Text files are not affected by this problem, since the
text scanner doesn't try to recover from errors during ProcessRange()
but wraps it in RETURN_IF_ERROR instead. With this change queries abort
with the same error.

Parquet files are also not affected since they have the metadata at the
end. Truncated files immediately fail with this error:
WARNINGS: File 
'hdfs://localhost:20500/test-warehouse/tpch.partsupp_parquet/foo.0.parq'
has an invalid version number: 

Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
---
M be/src/common/status.h
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/runtime-state.cc
M common/thrift/generate_error_codes.py
9 files changed, 104 insertions(+), 55 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
Gerrit-PatchSet: 12
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5890: Abort queries if scanner hits IO errors

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5890: Abort queries if scanner hits IO errors
..


Patch Set 10:

(5 comments)

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

PS10, Line 154: if the scan range has returned eosr before
> if that was the case, why can we have !eosr at line 152?
I think this was an old comment and I couldn't convince myself anymore that it 
was correct. I also discovered that the DCHECK should read (!status.ok || ...) 
because if the status is OK, that implies the buffer must not be NULL).


PS10, Line 155: must
> why "must"? or do you mean "can't"?
Yes, can't is what I meant to say. Removed the comment though.


PS10, Line 155: this is the first time the function
  : // is called 
> why is that significant? and why is it okay to have io_buffer_==NULL in tha
Removed the comment.


PS10, Line 200: DISK_IO_ERROR
> why not INTERNAL_ERROR, since that's the code that indicates an internal er
I can change that, but then we also have to abort the query in 
GetNextInternal() on INTERNAL_ERROR. I think we re-used DISK_IO_ERROR here to 
keep the set of aborting errors small. Would you prefer to switch to 
INTERNAL_ERROR?


PS10, Line 351:   if (boundary_buffer_bytes_left_ > 0 &&
  :   (output_buffer_pos_ != _buffer_pos_ ||
  :   output_buffer_bytes_left_ != 
_buffer_bytes_left_)) {
  : return false;
> this is effectively a double negative statement, which makes it harder to r
Done. I think in the else case (boundary_buffer_bytes_left_ == 0) the buffer 
pointers could point to either buffer.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-5890: Abort queries if scanner hits IO errors

[Lars Volker (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5890: Abort queries if scanner hits IO errors
..

IMPALA-5890: Abort queries if scanner hits IO errors

Prior to this fix, an error in ScannerContext::Stream::GetNextBuffer()
could leave the stream in an inconsistent state:

- The DiskIoMgr hits EOF unexpected, cancels the scan range and enqueues
a buffer with eosr set.
- The ScannerContext::Stream tries to read more bytes, but since it has
hit eosr, it tries to read beyond the end of the scan range using
DiskIoMgr::Read().
- The previous read error resulted in a new file handle being opened.
The now truncated, smaller file causes the seek to fail.
- Then during error handling, the BaseSequenceScanner calls SkipToSync()
and trips over the NULL pointer in in the IO buffer.

In my reproduction this only happens with the file handle cache enabled,
which causes Impala to see two different sized handles: the one from the
cache when the query starts, and the one after reopening the file.

To fix this, we change the I/O manager to always return DISK_IO_ERROR
for errors and we abort a query if we receive such an error in the
scanner.

This change also fixes GetBytesInternal() to maintain the invariant that
the output buffer points to the boundary buffer whenever the latter
contains some data.

I tested this by running the repro from the JIRA and impalad did not
crash but aborted the queries. I also ran the repro with
abort_on_error=1, and with the file handle cache disabled.

Text files are not affected by this problem, since the
text scanner doesn't try to recover from errors during ProcessRange()
but wraps it in RETURN_IF_ERROR instead. With this change queries abort
with the same error.

Parquet files are also not affected since they have the metadata at the
end. Truncated files immediately fail with this error:
WARNINGS: File 
'hdfs://localhost:20500/test-warehouse/tpch.partsupp_parquet/foo.0.parq'
has an invalid version number: 

Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
---
M be/src/common/status.h
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/runtime-state.cc
M common/thrift/generate_error_codes.py
9 files changed, 97 insertions(+), 55 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
Gerrit-PatchSet: 11
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-5890: Abort queries if scanner hits IO errors

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-5890: Abort queries if scanner hits IO errors
..


Patch Set 10:

I fixed more test errors and will run an exhaustive build in parallel now.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5890: Abort queries if scanner hits IO errors

[Lars Volker (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-5890: Abort queries if scanner hits IO errors
..

IMPALA-5890: Abort queries if scanner hits IO errors

Prior to this fix, an error in ScannerContext::Stream::GetNextBuffer()
could leave the stream in an inconsistent state:

- The DiskIoMgr hits EOF unexpected, cancels the scan range and enqueues
a buffer with eosr set.
- The ScannerContext::Stream tries to read more bytes, but since it has
hit eosr, it tries to read beyond the end of the scan range using
DiskIoMgr::Read().
- The previous read error resulted in a new file handle being opened.
The now truncated, smaller file causes the seek to fail.
- Then during error handling, the BaseSequenceScanner calls SkipToSync()
and trips over the NULL pointer in in the IO buffer.

In my reproduction this only happens with the file handle cache enabled,
which causes Impala to see two different sized handles: the one from the
cache when the query starts, and the one after reopening the file.

To fix this, we change the I/O manager to always return DISK_IO_ERROR
for errors and we abort a query if we receive such an error in the
scanner.

This change also fixes GetBytesInternal() to maintain the invariant that
the output buffer points to the boundary buffer whenever the latter
contains some data.

I tested this by running the repro from the JIRA and impalad did not
crash but aborted the queries. I also ran the repro with
abort_on_error=1, and with the file handle cache disabled.

Text files are not affected by this problem, since the
text scanner doesn't try to recover from errors during ProcessRange()
but wraps it in RETURN_IF_ERROR instead. With this change queries abort
with the same error.

Parquet files are also not affected since they have the metadata at the
end. Truncated files immediately fail with this error:
WARNINGS: File 
'hdfs://localhost:20500/test-warehouse/tpch.partsupp_parquet/foo.0.parq'
has an invalid version number: 

Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
---
M be/src/common/status.h
M be/src/exec/base-sequence-scanner.cc
M be/src/exec/scanner-context.cc
M be/src/exec/scanner-context.h
M be/src/runtime/disk-io-mgr-scan-range.cc
M be/src/runtime/disk-io-mgr-test.cc
M be/src/runtime/disk-io-mgr.cc
M be/src/runtime/runtime-state.cc
M common/thrift/generate_error_codes.py
9 files changed, 106 insertions(+), 54 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I44dc95184c241fbcdbdbebad54339530680d3509
Gerrit-PatchSet: 10
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Joe McDonnell 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong