[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 4:

I changed the approach, please take another look.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4745: fix TestScratchLimit failure on S3

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4745: fix TestScratchLimit failure on S3
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I607b4c6ad10eba0e6c7bc8d6e640d42da26ee6c8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4745: fix TestScratchLimit failure on S3

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4745: fix TestScratchLimit failure on S3
..


IMPALA-4745: fix TestScratchLimit failure on S3

The commit "IMPALA-3202,IMPALA-2079: rework scratch file I/O"
improved efficiency of scratch file use in some scenarios.
TestScratchLimit::test_with_low_scratch_limit started failing
on S3, because it expects to use more than 50MB of scratch space.

Testing:
Ran the test in a loop locally for 50+ iterations - didn't see
any failures.

Change-Id: I607b4c6ad10eba0e6c7bc8d6e640d42da26ee6c8
Reviewed-on: http://gerrit.cloudera.org:8080/5654
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M tests/query_test/test_scratch_limit.py
1 file changed, 2 insertions(+), 2 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I607b4c6ad10eba0e6c7bc8d6e640d42da26ee6c8
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] Major update to Impala + Kudu page

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Major update to Impala + Kudu page
..


Patch Set 4:

(31 comments)

http://gerrit.cloudera.org:8080/#/c/5649/4/docs/topics/impala_alter_table.xml
File docs/topics/impala_alter_table.xml:

PS4, Line 887: Any dropped range must not contain
 :   any data values in that range.
I don't believe this is true -- dropping a range is an efficient way to 
bulk-delete a bunch of data for workloads like rolling-window time series


http://gerrit.cloudera.org:8080/#/c/5649/4/docs/topics/impala_compute_stats.xml
File docs/topics/impala_compute_stats.xml:

Line 55: 
nope, because afaik we don't have partition-level stats for kudu (they aren't 
partitions as far as HMS is concerned)


http://gerrit.cloudera.org:8080/#/c/5649/4/docs/topics/impala_create_table.xml
File docs/topics/impala_create_table.xml:

PS4, Line 107: expression
'expression' here makes it sound like we support computed defaults. Perhaps 
it's better to say 'constant' or 'value'?


Line 122:   RANGE
this should be RANGE (pk_col [, ...]) right?


PS4, Line 125: constant
perhaps say 'tuple' or something instead? for composite PKs you need something 
like: PARTITION ('foo', 1) < VALUES


http://gerrit.cloudera.org:8080/#/c/5649/4/docs/topics/impala_grant.xml
File docs/topics/impala_grant.xml:

Line 134:   Access to Kudu tables must be granted to roles as usual.
is it worth noting here that grant/revoke from Impala has no bearing on access 
via the direct Kudu APIs?


http://gerrit.cloudera.org:8080/#/c/5649/4/docs/topics/impala_kudu.xml
File docs/topics/impala_kudu.xml:

PS4, Line 39: use the Apache Kudu component
stored by Apache Kudu


PS4, Line 98: ultiple Kudu hosts.
should say "separated by commas"


PS4, Line 105: TBLPROPERTIES
should say which table property to set


PS4, Line 147: logically
I'd say these are physical


PS4, Line 155: Kudu tablet servers
no, just the number of replicas of a given table must be odd. You could have 4 
tablet servers, but replication count 3


PS4, Line 156:  When you set up a new cluster, the number of tablet servers 
might not
 :   exactly match the number of DataNodes. If you add or 
remove one host from an existing
 :   cluster, be careful not to change the number of tablet 
servers from 3 to 2, or from 99
 :   to 100, and so on.
I'd scrap this whole 


PS4, Line 200: cannot contain any duplicate values
worth noting that it's the _tuple_ that has to be unique. EG you can have 
PRIMARY KEY (a, b) and have values (1,1), (1,2), (2, 1), (2,2). I don't think 
that's clear here.


PS4, Line 460:   
 : Because primary key columns cannot contain any 
NULL values, you can
 : omit the NOT NULL clause for the 
primary key columns.
that's true, but I think it's good practice to specify NOT NULL regardless, in 
case we ever add the ability for a nullable PK.


Line 553: them with the default PLAIN encoding.
I'd disagree with this -- in many cases encodings have a net speedup - there's 
some small CPU cost but the compression ratio can be very good. Especially if 
you have something like a timestamp value or transaction ID in your PK, 
bitshuffle will compress it quite well and have a negligible effect on 
performance of a key lookup, since the cost of decoding one page is nothing 
compared to the cost of the random disk IO, etc.


PS4, Line 592: user_id values come from a specific set of strings
that's slightly unexpected. I think a better example for dictionary would be 
something like 'ship_status STRING" from tpch, or 'state' or 'country'


PS4, Line 597: subjects might start with the same first words
seems somewhat contrived. Prefix encoding isn't super useful for most cases so 
maybe not worth documenting. (we use it internally for storing the reified 
composite key index where it is quite effective)


PS4, Line 601: The original text
 : from the body column is the most 
frequently accessed, therefore it
 : uses the compression with the least CPU overhead to 
expand. The
 : spanish_translation is accessed 
less frequently, therefore it uses
 : a compression format that produces a smaller result 
but takes more CPU cycles to
hrm, I'm not sure I'd draw such a distinction between LZ4 and SNAPPY. In fact 
LZ4 has some extensions which we aren't yet using (but should be at some point) 
which will probably be able to get it to be more dense than snappy as well as 
being faster, at which point we'd stop encouraging snappy


PS4, Line 636: into units known as
 : cfiles. The cfiles for each column can 
have a specified block size
I think I'd not mention cfiles here, but say that there is an underlying unit 
of IO which is the "block size".

[Impala-ASF-CR] Updates to DML statements for Impala + Kudu

[Todd Lipcon (Code Review)]

Todd Lipcon has posted comments on this change.

Change subject: Updates to DML statements for Impala + Kudu
..


Patch Set 1:

(4 comments)

http://gerrit.cloudera.org:8080/#/c/5646/1/docs/topics/impala_delete.xml
File docs/topics/impala_delete.xml:

Line 49: DELETE [FROM] 
[database_name].table_name [ WHERE 
where_conditions ]
I think there is another form to describe:

DELETE  FROM  WHERE ..

eg:
DELETE t1 from todd_test t1 JOIN todd_test2 t2 ON t1.o_orderkey = t2.o_orderkey;


PS1, Line 65:  
:   Normally, a DELETE operation for a Kudu 
table fails if
:   some partition key columns are not found, due to their 
being deleted or changed
:   by a concurrent UPDATE or 
DELETE operation.
:   Specify DELETE IGNORE 
rest_of_statement to
:   make the DELETE continue in this case. The 
rows with the nonexistent
:   duplicate partition key column values are not removed.
: 
I don't think this is relevant anymore (the IGNORE keyword is gone)


Line 101: 
Yes it can, using syntax like:

DELETE c FROM my_second_table c, stock_symbols s WHERE c.name = s.symbol;

(but it only deletes from one of the tables)

Can also use JOIN syntax as shown above


http://gerrit.cloudera.org:8080/#/c/5646/1/docs/topics/impala_update.xml
File docs/topics/impala_update.xml:

Line 50:   [ WHERE where_conditions ]
similar to my comment on the DELETE, this can support UPDATE with JOIN


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I60512b7957fb53d86d3123a4f1d46fbb355f4665
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Jean-Daniel Cryans 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT statements

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4351,IMPALA-4353: [qgen] randomly generate INSERT 
statements
..


Patch Set 6: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5486/4/tests/comparison/model_translator.py
File tests/comparison/model_translator.py:

Line 799: return data_type_class.__name__.upper()
> It's basically the same case as with Oracle: the code exists and maybe work
I never had a need to run against MySQL instead of PostgreSQL. I would vote to 
remove this code.

In general I would vote to remove code that is not being run regularly. Maybe 
keep it on some branch until we are ready to start running it regularly again.


http://gerrit.cloudera.org:8080/#/c/5486/4/tests/comparison/query_profile.py
File tests/comparison/query_profile.py:

PS4, Line 647: partial'
> Old habit related to C, when you could mistake = for == in comparison. If y
Yeah, makes sense. It's fine, don't change it.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I842b41f0eed07ab30ec76d8fc3cdd5affb525af6
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Michael Brown 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Michael Brown 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1861: Simplify conditionals with constant conditions

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-1861: Simplify conditionals with constant conditions
..


Patch Set 6: Code-Review+2

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5585/6/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 157:   if (child instanceof NullLiteral) {
single line


Line 179: }
explain the implied 'else' case: that it's always false and therefore can be 
dropped


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4036: show create table outputs invalid SQL for partitioned table with comment

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4036: show create table outputs invalid SQL for 
partitioned table with comment
..


Patch Set 3: Code-Review+2

(1 comment)

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

Line 7: IMPALA-4036: show create table outputs invalid SQL for partitioned 
table with comment
long line


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Marcel Kornacker (Code Review)]

Hello Impala Public Jenkins, Dimitris Tsirogiannis, Tim Armstrong,

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

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

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

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..

IMPALA-4739: ExprRewriter fails on HAVING clauses

The bug was that expr rewrite rules such as ExtractCommonConjunctRule
analyzed their own output, which doesn't work for syntactic elements
that allow column aliases, such as the HAVING clause.
The fix was to remove the analysis step (the re-analysis happens anyway
in AnalysisCtx).

Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
6 files changed, 25 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Marcel Kornacker (Code Review)]

Hello Impala Public Jenkins, Dimitris Tsirogiannis, Tim Armstrong,

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

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

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

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..

IMPALA-4739: ExprRewriter fails on HAVING clauses

The bug was that expr rewrite rules such as ExtractCommonConjunctRule
analyzed their own output, which doesn't work for syntactic elements
that allow column aliases, such as the HAVING clause.
The fix was to remove the analysis step (the re-analysis happens anyway
in AnalysisCtx).

Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/ExtractCommonConjunctRule.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/constant-folding.test
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
7 files changed, 35 insertions(+), 20 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR](asf-site) Initial commit of the blog section of the Impala ASF website.

[David Knupp (Code Review)]

David Knupp has abandoned this change.

Change subject: Initial commit of the blog section of the Impala ASF website.
..


Abandoned

Abandoned in lieu of https://gerrit.cloudera.org/5667

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

Gerrit-MessageType: abandon
Gerrit-Change-Id: Iaa578a70237fcc97589c667c17a70d3d6dad5ae1
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: David Knupp 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR](asf-site) Initial commit of the blog section of the Impala ASF website.

[David Knupp (Code Review)]

David Knupp has uploaded a new change for review.

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

Change subject: Initial commit of the blog section of the Impala ASF website.
..

Initial commit of the blog section of the Impala ASF website.

Note: a sample version of the site is up and running at:

   http://dknupp.github.io/blog/

The blog site is generated using Nikola. Nikola is only being used
to manage the blog in this patch, but could easily be used to manage
the other pages as well. It may take a fair bit of tweaking to the
settings and Jinja templates to make everything work properly if Pelican
is to be used for non-blog pages -- a job for when more cycles are
generally available.

The README gives a pretty complete description of the workflow for
adding content to the blog. The formatted README file can be seen at:

https://github.com/dknupp/dknupp.github.io/tree/master/nikola_site_generator

Not a lot of effort was spent in setting up the typical bloggish bells
and whistles - e.g. RSS/ATOM feeds, comments section etc. Fancier bits
can come later.

Change-Id: If5810f04b56c587a5114953cae7e4f484a92be75
---
A .gitignore
A blog/2017/index.html
A blog/archive.html
A blog/assets/css/additional_styles.css
A blog/assets/css/bootstrap-responsive.min.css
A blog/assets/css/bootstrap.min.css
A blog/assets/css/code.css
A blog/assets/css/ipython.min.css
A blog/assets/css/ipython.min.css.map
A blog/assets/css/nikola_ipython.css
A blog/assets/css/rst.css
A blog/assets/css/theme.css
A blog/assets/images/incubator.png
A blog/assets/js/fancydates.js
A blog/assets/js/html5.js
A blog/assets/js/moment-with-locales.min.js
A blog/assets/xml/atom.xsl
A blog/assets/xml/rss.xsl
A blog/categories/index.html
A blog/index.html
A blog/posts/impala-blog-coming-soon/index.html
A blog/posts/impala-blog-coming-soon/index.md
A blog/robots.txt
A blog/sitemap.xml
A blog/sitemapindex.xml
A nikola_site_generator/README.md
A nikola_site_generator/conf.py
A nikola_site_generator/posts/blog-coming-soon.md
A nikola_site_generator/requirements.txt
A nikola_site_generator/themes/impala-theme/assets/css/additional_styles.css
A 
nikola_site_generator/themes/impala-theme/assets/css/bootstrap-responsive.min.css
A nikola_site_generator/themes/impala-theme/assets/css/bootstrap.min.css
A nikola_site_generator/themes/impala-theme/assets/images/incubator.png
A nikola_site_generator/themes/impala-theme/engine
A nikola_site_generator/themes/impala-theme/parent
A nikola_site_generator/themes/impala-theme/templates/base.tmpl
A nikola_site_generator/themes/impala-theme/templates/base_footer.tmpl
A nikola_site_generator/themes/impala-theme/templates/base_header.tmpl
A nikola_site_generator/themes/impala-theme/templates/base_helper.tmpl
A nikola_site_generator/themes/impala-theme/templates/index.tmpl
A nikola_site_generator/themes/impala-theme/templates/index_helper.tmpl
A nikola_site_generator/themes/impala-theme/templates/post.tmpl
A nikola_site_generator/themes/impala-theme/templates/post_header.tmpl
A nikola_site_generator/themes/impala-theme/templates/post_helper.tmpl
44 files changed, 4,061 insertions(+), 0 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: If5810f04b56c587a5114953cae7e4f484a92be75
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: asf-site
Gerrit-Owner: David Knupp 
Gerrit-Reviewer: Jim Apple 

[Impala-ASF-CR] IMPALA-1861: Simplify conditionals with constant conditions

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-1861: Simplify conditionals with constant conditions
..


Patch Set 5:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/5585/4/fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
File fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java:

Line 50: if (!(expr.getChild(0) instanceof BoolLiteral)
> why is isLiteral() not enough? (what other literal could it be?)
NullLiteral, so if we just called isLiteral() something like "NULL || TRUE" 
wouldn't get simplified to "TRUE", since it wouldn't be rewritten by this rule, 
and then SimplifyConstantConditionals wouldn't simplify it either since it 
doesn't have a BoolLiteral left child.


http://gerrit.cloudera.org:8080/#/c/5585/3/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 65:   /**
> then spell this out in a todo
Done


Line 132:* 'then' for the first one is returned. Otherwise, if any of the 
'when's have constant
> i don't think it'll have any measurable impact on runtime, and the code won
It turned out to be pretty simple, so I went for it.


http://gerrit.cloudera.org:8080/#/c/5585/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
File fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java:

Line 91:   /**
> spell out rule dependencies in the class comment (and also in analysisctx w
Done


Line 143: boolean canSimplify = false;
> 'simplified' -> 'removed' (even more specific)
Done


Line 148:   }
> why don't you do
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-1861: Simplify conditionals with constant conditions

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has uploaded a new patch set (#5).

Change subject: IMPALA-1861: Simplify conditionals with constant conditions
..

IMPALA-1861: Simplify conditionals with constant conditions

When there are conditionals with constant values of TRUE or
FALSE we can simplify them during analysis using the ExprRewriter.

This patch introduces the SimplifyConditionalsRule with covers IF,
OR, AND, CASE, and DECODE.

It also introduces NormalizeExprsRule which normalizes AND and OR
such that if either child is a BoolLiteral, then the left child is a
BoolLiteral.

Testing:
- Added unit tests to ExprRewriteRulesTest.
- Ran FE planner tests and BE expr-test.

Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/CaseWhenClause.java
A fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
A fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
5 files changed, 362 insertions(+), 12 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id70aaf9fd99f64bd98175b7e2dbba28f350e7d3b
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] IMPALA-4707: fix use-after-free in QueryExecMgr

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4707: fix use-after-free in QueryExecMgr
..


IMPALA-4707: fix use-after-free in QueryExecMgr

The bug is that the QueryState was referenced after the refcount is
decremented. The fix is to not do that.

Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
Reviewed-on: http://gerrit.cloudera.org:8080/5615
Reviewed-by: Dan Hecht 
Tested-by: Impala Public Jenkins
---
M be/src/runtime/query-exec-mgr.cc
1 file changed, 15 insertions(+), 17 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Dan Hecht: Looks good to me, approved



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4707: fix use-after-free in QueryExecMgr

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4707: fix use-after-free in QueryExecMgr
..


Patch Set 6: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4745: fix TestScratchLimit failure on S3

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4745: fix TestScratchLimit failure on S3
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/162/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I607b4c6ad10eba0e6c7bc8d6e640d42da26ee6c8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4745: fix TestScratchLimit failure on S3

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4745: fix TestScratchLimit failure on S3
..


Patch Set 2: Code-Review+2

rebase

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I607b4c6ad10eba0e6c7bc8d6e640d42da26ee6c8
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4549: consistently treat 9999 as upper bound for timestamp year

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4549: consistently treat  as upper bound for 
timestamp year
..


Patch Set 1:

(4 comments)

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

Line 9: Use the patched boost version and update tests accordingly.
> Maybe briefly describe the behavior of Impala before this patch?
Done


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

Line 1482:   TestIsNull("cast(cast('-12-31 23:59:59' as timestamp) + 
interval 1 year as bigint)",
> I think this test would be more interesting if we add 1 second instead. (th
I looked at this again and realised I misunderstood what it was doing.

I think this was a workaround to deal with the fact that we couldn't parse 
5-digit years 1. We have more extensive edge case tests below in the 
timestamp tests. I modified one of the below tests to test + 1 second.


Line 1484:   TestValue("cast(cast('-12-31 23:59:59' as timestamp) as 
bigint)",
> This seems like the base case, put it above the TestIsNull case. (maybe add
Done


Line 1486:   TestTimestampValue("cast(253402300799 as timestamp) - interval 1 
year",
> Not sure what this is testing. Maybe remove?
It's testing the int->timestamp reverse conversion. I restructured this to be a 
bit more logically organised.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4549: consistently treat 9999 as upper bound for timestamp year

[Tim Armstrong (Code Review)]

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

Change subject: IMPALA-4549: consistently treat  as upper bound for 
timestamp year
..

IMPALA-4549: consistently treat  as upper bound for timestamp year

Previously Impala was inconsistent about whether the year 1 was
supported, as a result of inconsistency in boost, which reported the
maximum year as  but sometimes allowed 1. This meant that
Impala sometimes accepted the year 1 and sometimes not.

Use the patched boost version and update tests accordingly.

Testing:
Ran an exhaustive build.

Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
---
M CMakeLists.txt
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-test.cc
M bin/impala-config.sh
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M tests/comparison/discrepancy_searcher.py
6 files changed, 35 insertions(+), 25 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 

[Impala-ASF-CR] IMPALA-4749: hit DCHECK in sorter with scratch limit

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4749: hit DCHECK in sorter with scratch limit
..


Patch Set 1: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id85b96826f13f74c7c5474b0b50e12229e9d8b4f
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-3671: Add SCRATCH LIMIT query option.

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3671: Add SCRATCH_LIMIT query option.
..


Patch Set 1: Code-Review+1

(1 comment)

The technical content looks accurate.

http://gerrit.cloudera.org:8080/#/c/5651/1/docs/topics/impala_scratch_limit.xml
File docs/topics/impala_scratch_limit.xml:

PS1, Line 79: DataNodes
It's really the number of Impala daemons that determines this, right? Even if 
it's usually the same number.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I662a59a59da8a170a2710d4a5245363ae1e3f754
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4163: Add doc for sortby() hint

[John Russell (Code Review)]

John Russell has uploaded a new patch set (#2).

Change subject: IMPALA-4163: Add doc for sortby() hint
..

IMPALA-4163: Add doc for sortby() hint

Add SORTBY() to list of hints in "Hints" topic.

Update hint syntax in INSERT topic.

Also modernize the hint syntax as shown under INSERT
to include the -- and /* */ formats also. List
the [] style last since it is the least-preferred
option.

Change-Id: Id3c1da9a87ace361b096fa73d8504b2f54e75bed
---
M docs/shared/impala_common.xml
M docs/topics/impala_hints.xml
M docs/topics/impala_insert.xml
3 files changed, 29 insertions(+), 7 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id3c1da9a87ace361b096fa73d8504b2f54e75bed
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 

[Impala-ASF-CR] IMPALA-4163: Add doc for sortby() hint

[John Russell (Code Review)]

John Russell has posted comments on this change.

Change subject: IMPALA-4163: Add doc for sortby() hint
..


Patch Set 1: Code-Review-1

(1 comment)

Needs another patch set for sure.

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

PS1, Line 7: Add doc for sortby() hint
In addition to this separate "Hints" topic, the hints relevant for INSERT 
statements are shown on the page for the INSERT statement. That page needs to 
be updated too. (That'll be patch set 2.)


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Id3c1da9a87ace361b096fa73d8504b2f54e75bed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4036: show create table outputs invalid SQL for partitioned table with comment

[Joe McDonnell (Code Review)]

Hello Thomas Tauber-Marshall,

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

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

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

Change subject: IMPALA-4036: show create table outputs invalid SQL for 
partitioned table with comment
..

IMPALA-4036: show create table outputs invalid SQL for partitioned table with 
comment

For a table that has both a table comment and a partition specified,
"show create table" incorrectly outputs the comment before the partition.
This is not the correct order, and it results in an invalid SQL.

This transaction fixes the ordering (partition comes before comment) and
adds tests for this case.

Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
---
M fe/src/main/java/org/apache/impala/analysis/ToSqlUtils.java
M fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
M testdata/workloads/functional-query/queries/QueryTest/show-create-table.test
3 files changed, 8 insertions(+), 3 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 

[Impala-ASF-CR] Updates to DML statements for Impala + Kudu

[John Russell (Code Review)]

John Russell has posted comments on this change.

Change subject: Updates to DML statements for Impala + Kudu
..


Patch Set 1:

(1 comment)

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

PS1, Line 7: Updates to DML statements for Impala + Kudu
FYI: I posted this PDF:

http://impala-pdf.s3.amazonaws.com/impala-kudu.pdf

with all the info from this code review + the separate code review for the main 
Kudu page + DDL changes.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I60512b7957fb53d86d3123a4f1d46fbb355f4665
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: John Russell 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4355: random query generator: modify statement execution flow to support DML

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4355: random query generator: modify statement execution 
flow to support DML
..


Patch Set 5: Code-Review+2

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

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

[Impala-ASF-CR] IMPALA-3524: Don't process spilled partitions with 0 probe rows

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-3524: Don't process spilled partitions with 0 probe rows
..


Patch Set 7:

(12 comments)

http://gerrit.cloudera.org:8080/#/c/5389/6/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

Line 388:   runtime_state_, 
Substitute(PREPARE_FOR_READ_FAILED_ERROR_MSG, id_));
> can't this cause us to fail a query that we used to be able to execute, sin
As discussed, this is okay because we would've needed at least as much memory 
to go through the repartitioning path anyways.

Also, I looked into moving PrepareForRead into OutputUnmatchedBuild to minimize 
the time we have the memory used, but I don't think this works, at least not 
without complicating the code, as OutputUnmatchedBuild may be called multiple 
times for the same partition if we reach out_batch->AtCapacity and calling 
PrepareForRead each time would be incorrect. And anyways, OutputUnmatchedBuild 
will be called almost immediately after this, so the difference in memory usage 
is minor.


PS6, Line 652: There were no probe rows so we skipped building the hash table. 
In this case, all
> this comment doesn't explains more "what" than "why", and what is fairly cl
Done


Line 677: if (output_unmatched_batch_iter_->AtEnd()) {
> Something I missed in the first pass. 'output_unmatched_batch_' may have ha
I  played around with this for awhile without success.

In particular, it seemed like for the queries that i ran, no resources were 
attached to output_unmatched_batch_. Is there a specific situation in which you 
expect this to happen?


Line 702: output_build_partitions_.pop_front();
> why is this the case?
See the comment at the top of this function.


http://gerrit.cloudera.org:8080/#/c/5389/6/be/src/exec/partitioned-hash-join-node.h
File be/src/exec/partitioned-hash-join-node.h:

PS6, Line 291:  will
 :   /// be the on
> seems redundant with "first"
Done


PS6, Line 293: titions_'.
 :   Status OutputAllBuild(RowBatch* out_batch);
> this seems to contradict the "first partition of 'output_build_partitions_'
Done


PS6, Line 306: 
> doesn't it increment the out_batch_iterator?
No, out_batch_iterator.Next() is called after OutputBuildRow, depending on the 
return of EvalConjuncts.

I could certainly move that logic into OutputBuildRow if you think its clearer, 
it would just give OutputBuildRow a somewhat long argument list.


Line 362:   /// rows. If there are no probe rows, we just prepare the build 
side to be read by
> this comment is now stale.
Done


Line 409:   RuntimeProfile::Counter* null_aware_eval_timer_;
> add a comment to explain it. e.g. what does it mean to be "skipped"?
Done


PS6, Line 477: putAllBuild() to iterat
> this also seems to contradict the earlier claim that there is only one part
Done


Line 479:   std::unique_ptr output_unmatched_batch_;
> this comment is tough to understand if you haven't already read the code in
Done


http://gerrit.cloudera.org:8080/#/c/5389/7/testdata/workloads/tpch/queries/tpch-outer-joins.test
File testdata/workloads/tpch/queries/tpch-outer-joins.test:

Line 32
To be clear, I verified that this test is still testing the situation from the 
referenced JIRA without these lines.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I175b32dd9031e51218b38c37693ac3e31dfab47b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3524: Don't process spilled partitions with 0 probe rows

[Thomas Tauber-Marshall (Code Review)]

Hello Tim Armstrong,

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

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

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

Change subject: IMPALA-3524: Don't process spilled partitions with 0 probe rows
..

IMPALA-3524: Don't process spilled partitions with 0 probe rows

In the partitioned hash join node, if a spilled partition has no probe
rows, building the hash table is unnecessary.

For some build types (right outer, right anti, and full outer), we still
need to process the build side to output unmatched rows (in this case, all
rows since there were no probe rows to match).

Testing: Added some cases to spilling.test. Manually tested these cases
for performance, and they all show around a 6% improvement.

Change-Id: I175b32dd9031e51218b38c37693ac3e31dfab47b
---
M be/src/exec/partitioned-hash-join-node.cc
M be/src/exec/partitioned-hash-join-node.h
M testdata/workloads/functional-query/queries/QueryTest/spilling.test
M testdata/workloads/tpch/queries/tpch-outer-joins.test
4 files changed, 232 insertions(+), 40 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I175b32dd9031e51218b38c37693ac3e31dfab47b
Gerrit-PatchSet: 7
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4549: consistently treat 9999 as upper bound for timestamp year

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4549: consistently treat  as upper bound for 
timestamp year
..


Patch Set 1:

(5 comments)

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

Line 9: Use the patched boost version and update tests accordingly.
Maybe briefly describe the behavior of Impala before this patch?


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

Line 1482:   TestIsNull("cast(cast('-12-31 23:59:59' as timestamp) + 
interval 1 year as bigint)",
I think this test would be more interesting if we add 1 second instead. (that 
should give us the smallest timestamp value that's not valid)


Line 1484:   TestValue("cast(cast('-12-31 23:59:59' as timestamp) as 
bigint)",
This seems like the base case, put it above the TestIsNull case. (maybe add a 
comment saying that we are verifying that the cast works on the largest allowed 
timestamp value (without nanoseconds)).


Line 1486:   TestTimestampValue("cast(253402300799 as timestamp) - interval 1 
year",
Not sure what this is testing. Maybe remove?


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

PS1, Line 2611: 1400..
Is this error message generated by boost?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-3973: optional 2nd and 3rd arguments for instr().

[John Russell (Code Review)]

John Russell has posted comments on this change.

Change subject: IMPALA-3973: optional 2nd and 3rd arguments for instr().
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5589/2/docs/topics/impala_string_functions.xml
File docs/topics/impala_string_functions.xml:

Line 406: select instr('hello world','o',-1);
> I do not see a patch set 3 here yet. Are you planning on sending that in a 
Sorry, I had patch set 3 worked up over the weekend but it got borked by a 
corrupted hard drive before I could push. I've got some other CRs to attend to 
before I come back to this one.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I17268bdb480230938f94559fe1eabe34ac2448b7
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: John Russell 
Gerrit-Reviewer: Zoltan Ivanfi 
Gerrit-Reviewer: zi+z...@cloudera.com
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-2615: warn if Status is ignored

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-2615: warn if Status is ignored
..


Patch Set 3:

this draft looks fine to me.

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

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

[Impala-ASF-CR] Add Kudu cmake utilities

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: Add Kudu cmake utilities
..


Patch Set 2:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5656/2/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:

PS2, Line 30: # There are two different kinds of exported libraries: internal 
and leaf.
: # Internal libraries are static archives while leaf libraries are 
shared
: # objects built from internal libraries.
> Why do we need this? Maybe I'm missing context- Is there a doc or readme ex
These are needed to so that we can import kudu's CMakeLists.txt without 
significant alteration. 

See 
https://github.com/henryr/Impala/blob/0ef5ce0a18eb734dc8caedfca2752dfd4f513ba2/be/src/kudu/util/CMakeLists.txt
 for the main case where they're used.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Major update to Impala + Kudu page

[John Russell (Code Review)]

John Russell has uploaded a new patch set (#4).

Change subject: Major update to Impala + Kudu page
..

Major update to Impala + Kudu page

Upgrade with details of latest syntax.

Fine-tune discussion of PK and other Kudu
notions.

The impala_kudu diff looks larger than actual changes
to the page, because subtopics got moved
around and promoted/demoted (which changes the
indentation). Best to review that page start-to-finish.

CREATE TABLE details for Impala + Kudu.

ALTER TABLE details for Impala + Kudu.

Unhide the Impala partitioning + Kudu topic.
Mainly a brief intro then a link to delegate
details to the main Kudu page, which already
has a partitioning subtopic.

Include changes to reserved words. Entirely
from Kudu integration work.

Add Kudu considerations for misc SQL statements.

Change-Id: I76dcb948dab08532fe41326b22ef78d73282db2c
---
M docs/shared/impala_common.xml
M docs/topics/impala_alter_table.xml
M docs/topics/impala_compute_stats.xml
M docs/topics/impala_create_table.xml
M docs/topics/impala_describe.xml
M docs/topics/impala_drop_table.xml
M docs/topics/impala_explain.xml
M docs/topics/impala_grant.xml
M docs/topics/impala_invalidate_metadata.xml
M docs/topics/impala_kudu.xml
M docs/topics/impala_literals.xml
M docs/topics/impala_partitioning.xml
M docs/topics/impala_refresh.xml
M docs/topics/impala_reserved_words.xml
M docs/topics/impala_revoke.xml
M docs/topics/impala_show.xml
M docs/topics/impala_tables.xml
M docs/topics/impala_truncate_table.xml
18 files changed, 1,750 insertions(+), 87 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I76dcb948dab08532fe41326b22ef78d73282db2c
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2:

> > Okay
 > 
 > Do you think this needs more explanation in the breakpad tests?

I guess you already say "running processes" in that comment you added, so I 
think that's enough.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2:

> Okay

Do you think this needs more explanation in the breakpad tests?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2:

Okay

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Release note updates for Impala 2.8

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: Release note updates for Impala 2.8
..


Patch Set 1:

(1 comment)

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

Line 28: Change-Id: I03144b423c4d698e87dd335914a8b7c0ff030496
I won't have time to do a thorough review here but this commit message needs 
exactly one Change-Id.

When I'm rebasing, I usually fixup, rather than squash, which removes the later 
message, including its change Id


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Anonymous Coward #250
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Release note updates for Impala 2.8

[Greg Rahn (Code Review)]

Greg Rahn has posted comments on this change.

Change subject: Release note updates for Impala 2.8
..


Patch Set 1:

(3 comments)

http://gerrit.cloudera.org:8080/#/c/5668/1/docs/topics/impala_incompatible_changes.xml
File docs/topics/impala_incompatible_changes.xml:

PS1, Line 111:  This is a change from early experimental versions of Impala + 
Kudu, where columns
 : had the NOT NULL attribute by 
default.
> I don't think this is correct. AFAIK the default behavior is the same and w
The GA behavior is columns are nullable by default, unless they are part of the 
PK.  There was a change that inverted this behavior for a time but now impala 
explicitly implements the expected RDBMS behavior.  Perhaps that is where the 
confusion comes from.


http://gerrit.cloudera.org:8080/#/c/5668/1/docs/topics/impala_new_features.xml
File docs/topics/impala_new_features.xml:

PS1, Line 66: The COMPUTE STATS statement can
: take advantage of multithreading.
Not sure if you want to be more specific here as the default for Parquet tables 
will now be MT_DOP=4.


PS1, Line 91: allows Impala INSERT operations
: that use dynamic partitioning to process a high 
number of
: partitions in a single statement.
SORTBY adds ordering for non-partition key columns to better support the 
effectiveness of min/max data elimination techniques (still a WIP).  
CLUSTERED adds ordering for the partition key columns such that there is only a 
single writer for any given partition thus reducing the overall memory needed 
when inserting into many partitions.  
Both of these are mentioned in the description of IMPALA-2522.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Anonymous Coward #250
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] Add Kudu cmake utilities

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Add Kudu cmake utilities
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5656/2/cmake_modules/kudu_cmake_fns.txt
File cmake_modules/kudu_cmake_fns.txt:

PS2, Line 26: # add_library() wrapper that adds a second variant of the 
library, suffixed with
: # "_exported" and which is compiled with special visibility flags 
to hide all symbols
: # except those that are part of the public ABI.
this doesn't seem to be true; the code differs from the kudu impl


PS2, Line 30: # There are two different kinds of exported libraries: internal 
and leaf.
: # Internal libraries are static archives while leaf libraries are 
shared
: # objects built from internal libraries.
Why do we need this? Maybe I'm missing context- Is there a doc or readme 
explaining how the kudu libraries in impala will be included/linked/work?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ibaae645d650ab1555452e4cc2574d6c84a90d941
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Henry Robinson 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2:

> Should we add this to tests/custom_cluster/test_breakpad.py where
 > we also look for logs?
I don't think we need this for the breakpad tests. The tests that actually kill 
the cluster trigger a flush of the log files before the process exits. Also, 
Breakpad forks to write a minidump and then prints its log messages to stdout 
and stderr before exiting, which are redirected into the log files. Therefore 
it seems that the messages show up in the logs, once the forks have written the 
minidumps and terminated (for which we wait in the test).

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4652: Add crcutil to build

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4652: Add crcutil to build
..


Patch Set 1: Code-Review+2

(1 comment)

Looks good, looking forward to the follow-up patches when we make use of all of 
this.

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

Line 7: IMPALA-4652: Add crcutil to build
Can you mention that the cmake module came from Kudu?


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

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

[Impala-ASF-CR] IMPALA-4651: Add LibEv to build

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4651: Add LibEv to build
..


Patch Set 1: Code-Review+2

(1 comment)

Looks good, looking forward to the follow-up patches when we make use of all of 
this.

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

Line 9: Add libev 4.20 to the Impala build. This is a dependency of KRPC.
Can you mention that the cmake module came from Kudu?


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

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

[Impala-ASF-CR] IMPALA-4036: show create table outputs invalid SQL for partitioned table with comment

[Marcel Kornacker (Code Review)]

Marcel Kornacker has posted comments on this change.

Change subject: IMPALA-4036: show create table outputs invalid SQL for 
partitioned table with comment
..


Patch Set 2:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5648/2/fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java
File fe/src/test/java/org/apache/impala/analysis/ToSqlTest.java:

Line 305:   public void TestCreateTableWCommentPartition() throws 
AnalysisException {
just roll this into an existing test, no need to have something separate (each 
statement takes a little bit of time to execute).


http://gerrit.cloudera.org:8080/#/c/5648/2/testdata/workloads/functional-query/queries/QueryTest/show-create-table.test
File 
testdata/workloads/functional-query/queries/QueryTest/show-create-table.test:

Line 119: # With a table comment and a partition
same here, roll this into an existing test case.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Joe McDonnell 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4707: fix use-after-free in QueryExecMgr

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4707: fix use-after-free in QueryExecMgr
..


Patch Set 6:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/160/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4707: fix use-after-free in QueryExecMgr

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4707: fix use-after-free in QueryExecMgr
..


Patch Set 6: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 2:

Marcel just informed me that he will continue working on this patch. (I am no 
longer working on this)

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2:

Should we add this to tests/custom_cluster/test_breakpad.py where we also look 
for logs?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] Release note updates for Impala 2.8

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: Release note updates for Impala 2.8
..


Patch Set 1:

(7 comments)

http://gerrit.cloudera.org:8080/#/c/5668/1/docs/topics/impala_incompatible_changes.xml
File docs/topics/impala_incompatible_changes.xml:

PS1, Line 91: not
typo


PS1, Line 111:  This is a change from early experimental versions of Impala + 
Kudu, where columns
 : had the NOT NULL attribute by 
default.
I don't think this is correct. AFAIK the default behavior is the same and we 
have nullable cols by default


http://gerrit.cloudera.org:8080/#/c/5668/1/docs/topics/impala_new_features.xml
File docs/topics/impala_new_features.xml:

PS1, Line 90: )
move left?


PS1, Line 103: rev="IMPALA-1788"
move to p?


PS1, Line 238: involving a small number of rows
needed?


PS1, Line 247: lineage metadata is not generated for
 : UPDATE and 
DELETE operations on Kudu tables.
does insert work for kudu tables? check with dimitris


PS1, Line 253: Currently, Kudu tables have limited support for Sentry:
dimitris should review

we may call out that access thru other engines to kudu doesnt respect this


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 
Gerrit-Reviewer: Ambreen Kazi 
Gerrit-Reviewer: Anonymous Coward #250
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Greg Rahn 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


IMPALA-4751: Remove blank line from raw_text template

The additional blank line can break tooling which uses the
/query_profile_encoded endpoint and has been erroneously introduced in
the fix for IMPALA-3918.

Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Reviewed-on: http://gerrit.cloudera.org:8080/5664
Reviewed-by: Jim Apple 
Reviewed-by: Tim Armstrong 
Tested-by: Impala Public Jenkins
---
M tests/webserver/test_web_pages.py
M www/raw_text.tmpl
2 files changed, 16 insertions(+), 2 deletions(-)

Approvals:
  Impala Public Jenkins: Verified
  Jim Apple: Looks good to me, approved
  Tim Armstrong: Looks good to me, but someone else must approve



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 2: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 2: Code-Review+2

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

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

[Impala-ASF-CR] IMPALA-4749: hit DCHECK in sorter with scratch limit

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4749: hit DCHECK in sorter with scratch limit
..


Patch Set 1: Code-Review+2

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

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

[Impala-ASF-CR] IMPALA-4745: fix TestScratchLimit failure on S3

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

Change subject: IMPALA-4745: fix TestScratchLimit failure on S3
..


Patch Set 1: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I607b4c6ad10eba0e6c7bc8d6e640d42da26ee6c8
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Lars Volker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 1:

(2 comments)

Thanks for the review, please see PS2.

http://gerrit.cloudera.org:8080/#/c/5669/1/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:

PS1, Line 142: \
> nit: the line continuation shouldn't be necessary inside parentheses. Might
Done


http://gerrit.cloudera.org:8080/#/c/5669/1/tests/custom_cluster/test_scratch_disk.py
File tests/custom_cluster/test_scratch_disk.py:

PS1, Line 86: -logbuflevel=-1
> We should document the importance of this somewhere. Maybe in the comment f
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
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-4722: Disable log caching in test scratch disk

[Lars Volker (Code Review)]

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

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..

IMPALA-4722: Disable log caching in test_scratch_disk

test_scratch_disk fails sporadically when trying to assert the presence
of log messages. This is probably caused by log caching, since after
such failures the log files do contains the lines in question.

I manually tested this by running the tests repeatedly for 2 days (10k
runs).

To make future diagnosis of similar problems easier, this change also
adds more output to assert_impalad_log_contains().

Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
---
M tests/common/custom_cluster_test_suite.py
M tests/custom_cluster/test_scratch_disk.py
2 files changed, 15 insertions(+), 9 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Taras Bobrovytsky (Code Review)]

Taras Bobrovytsky has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 2:

I think this requires some additional test fixes, I am looking into it.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Taras Bobrovytsky 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4745: fix TestScratchLimit failure on S3

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4745: fix TestScratchLimit failure on S3
..


Patch Set 1: Code-Review+1

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

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

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 2:

Is this ready for a new pre-commit Jenkins test?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5669/1/tests/common/custom_cluster_test_suite.py
File tests/common/custom_cluster_test_suite.py:

PS1, Line 142: \
nit: the line continuation shouldn't be necessary inside parentheses. Might as 
well fix it while we're here.


http://gerrit.cloudera.org:8080/#/c/5669/1/tests/custom_cluster/test_scratch_disk.py
File tests/custom_cluster/test_scratch_disk.py:

PS1, Line 86: -logbuflevel=-1
We should document the importance of this somewhere. Maybe in the comment for 
assert_impalad_log_contains()?


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

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

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 2: Code-Review+2

Test fixes look good to me

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4722: Disable log caching in test scratch disk

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

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

Change subject: IMPALA-4722: Disable log caching in test_scratch_disk
..

IMPALA-4722: Disable log caching in test_scratch_disk

test_scratch_disk fails sporadically when trying to assert the presence
of log messages. This is probably caused by log caching, since after
such failures the log files do contains the lines in question.

I manually tested this by running the tests repeatedly for 2 days (10k
runs).

To make future diagnosis of similar problems easier, this change also
adds more output to assert_impalad_log_contains().

Change-Id: I9f21284338ee7b4374aca249b6556282b0148389
---
M tests/common/custom_cluster_test_suite.py
M tests/custom_cluster/test_scratch_disk.py
2 files changed, 9 insertions(+), 6 deletions(-)


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

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

[Impala-ASF-CR] Release note updates for Impala 2.8

[John Russell (Code Review)]

John Russell has uploaded a new change for review.

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

Change subject: Release note updates for Impala 2.8
..

Release note updates for Impala 2.8

First cut at 'new features' topic.

Commented out one link to SCRATCH_LIMIT
query option - topic doesn't exist yet.

Filled in JIRA # and name for sortby() hint.

Made Incompatible Changes subtopic for Impala 2.8.

Also did some cleanup throughout the Incompatible
Changes page:

- Took out references to Cloudera release numbers
  from titles.
- Suppressed the display of ancient subtopics from
  the Impala beta days, which are intertwined with
  things like what version of Cloudera Manager was
  supported.

Change-Id: I03144b423c4d698e87dd335914a8b7c0ff030496

Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
---
M docs/topics/impala_incompatible_changes.xml
M docs/topics/impala_new_features.xml
2 files changed, 422 insertions(+), 32 deletions(-)


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

Gerrit-MessageType: newchange
Gerrit-Change-Id: I7c47f422e509cec6d3eb8aaa82294b584f393aed
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: John Russell 

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Marcel Kornacker (Code Review)]

Hello Impala Public Jenkins, Dimitris Tsirogiannis,

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

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

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

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..

IMPALA-4739: ExprRewriter fails on HAVING clauses

SelectStmt.rewriteExprs() rewrote the pre-analysis form of the HAVING
predicate; the fix was to rewrite the post-analysis version.

SelectStmt.rewriteExprs() is also not effective for the grouping exprs,
but I decided to leave that alone, since it won't result in an error.

The rewriter rule tests are too narrow right now, because they only apply
to select list exprs (which is why it missed the problem with the Having
clause). I added a functional test for this particular jira, but we should
also restructure the rewriter rule tests themselves to apply to all
syntactic elements that can see rewrites.

Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
4 files changed, 27 insertions(+), 16 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-3524: Don't process spilled partitions with 0 probe rows

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-3524: Don't process spilled partitions with 0 probe rows
..


Patch Set 6:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5389/6/be/src/exec/partitioned-hash-join-node.cc
File be/src/exec/partitioned-hash-join-node.cc:

Line 677:   output_unmatched_batch_->Reset();
Something I missed in the first pass. 'output_unmatched_batch_' may have had 
resources attached to it by BufferedTupleStream::GetNext(). We need to transfer 
resources from 'output_unmatched_batch_' to 'out_batch' with 
RowBatch::TransferResourceOwnership() before resetting the batch. 

The bug may be tricky to reproduce but it's worth trying to add a test for. 
We'd need a build partition with > 8mb of data (so that it uses multiple 
buffers) and then it would be a race to see if the data in the buffer that 
wasn't attached got overwritten.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I175b32dd9031e51218b38c37693ac3e31dfab47b
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Thomas Tauber-Marshall 
Gerrit-Reviewer: Dan Hecht 
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4707: fix use-after-free in QueryExecMgr

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4707: fix use-after-free in QueryExecMgr
..


Patch Set 6:

I had to make a non-trivial change to fix a bug, would be good if reviewers 
could take another quick look.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4707: fix use-after-free in QueryExecMgr

[Tim Armstrong (Code Review)]

Hello Marcel Kornacker, Impala Public Jenkins, Jim Apple, Henry Robinson,

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

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

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

Change subject: IMPALA-4707: fix use-after-free in QueryExecMgr
..

IMPALA-4707: fix use-after-free in QueryExecMgr

The bug is that the QueryState was referenced after the refcount is
decremented. The fix is to not do that.

Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
---
M be/src/runtime/query-exec-mgr.cc
1 file changed, 15 insertions(+), 17 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I329ff758a0de148a3bdfedc245e56c3a63255535
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Tim Armstrong 

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

[Dan Hecht (Code Review)]

Dan Hecht has posted comments on this change.

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


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/exprs/bit-byte-functions-ir.cc
File be/src/exprs/bit-byte-functions-ir.cc:

PS3, Line 152: rotate_left
> Yes, there are some places where UBSAN catches UB on 128-bit ints.
I think we should assume the implementation defined behavior is the behavior 
that clang currently implements. I assume it's the same.


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

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

[Impala-ASF-CR] IMPALA-4036: show create table outputs invalid SQL for partitioned table with comment

[Thomas Tauber-Marshall (Code Review)]

Thomas Tauber-Marshall has posted comments on this change.

Change subject: IMPALA-4036: show create table outputs invalid SQL for 
partitioned table with comment
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I29a33cfd142b473997fdc3acfe3f0966bc7ed784
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: joemcdonn...@cloudera.com
Gerrit-Reviewer: Thomas Tauber-Marshall 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4549: consistently treat 9999 as upper bound for timestamp year

[Tim Armstrong (Code Review)]

Tim Armstrong has uploaded a new change for review.

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

Change subject: IMPALA-4549: consistently treat  as upper bound for 
timestamp year
..

IMPALA-4549: consistently treat  as upper bound for timestamp year

Use the patched boost version and update tests accordingly.

Testing:
Ran an exhaustive build.

Change-Id: Iaf23b40833017789d879e5da7bb10384129e2d10
---
M CMakeLists.txt
M be/src/exprs/expr-test.cc
M be/src/runtime/timestamp-test.cc
M bin/impala-config.sh
M testdata/workloads/functional-query/queries/QueryTest/exprs.test
M tests/comparison/discrepancy_searcher.py
6 files changed, 20 insertions(+), 23 deletions(-)


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

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

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

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

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


Patch Set 3:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/5082/3/be/src/exprs/bit-byte-functions-ir.cc
File be/src/exprs/bit-byte-functions-ir.cc:

PS3, Line 152: rotate_left
> why do we have to handle int128? Our biggest int type is 64-bits.  Is there
Yes, there are some places where UBSAN catches UB on 128-bit ints.

This might not be one of them, I'll check.

Great point about GCC! I looked around and it loks like clang makes no 
promises, though:

https://llvm.org/bugs/show_bug.cgi?id=11272

Thoughts?


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

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

Re: [Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Henry Robinson]

Let's get it in. We can discuss the other issues later.


On Tue, Jan 10, 2017 at 9:33 AM Tim Armstrong (Code Review) <
ger...@cloudera.org> wrote:

> Tim Armstrong has posted comments on this change.
>
>
>
> Change subject: IMPALA-4751: Remove blank line from raw_text template
>
> ..
>
>
>
>
>
> Patch Set 2: Code-Review+1
>
>
>
> --
>
> To view, visit http://gerrit.cloudera.org:8080/5664
>
> To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
>
>
>
> Gerrit-MessageType: comment
>
> Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
>
> Gerrit-PatchSet: 2
>
> Gerrit-Project: Impala-ASF
>
> Gerrit-Branch: master
>
> Gerrit-Owner: Lars Volker 
>
> Gerrit-Reviewer: David Knupp 
>
> Gerrit-Reviewer: Henry Robinson 
>
> Gerrit-Reviewer: Impala Public Jenkins
>
> Gerrit-Reviewer: Jim Apple 
>
> Gerrit-Reviewer: Lars Volker 
>
> Gerrit-Reviewer: Tim Armstrong 
>
> Gerrit-HasComments: No
>
>

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Tim Armstrong (Code Review)]

Tim Armstrong has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 2: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 2:

> Why can't this be fixed on the consumer side? Stripping out leading
 > and trailing white space seems a pretty reasonable requirement for
 > parsing this file.

I suppose it can be done on the consumer side, too. Since this used to work and 
Impala changed its behavior, it looked like the cleanest approach to me to get 
the file back into the expected format.

Do we have documentation on these endpoints where we can explain how to parse 
them and what a consumer should expect? Do you think we should hold of on this 
change and keep the current behavior instead?

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Henry Robinson (Code Review)]

Henry Robinson has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 2:

Why can't this be fixed on the consumer side? Stripping out leading and 
trailing white space seems a pretty reasonable requirement for parsing this 
file.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Henry Robinson 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 2:

Build started: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/158/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 2:

> DO you want me to start the pre-commit testing Jenkins job?

Thanks for the review. Yes, it would be great if you could run the Jenkins job 
and submit the change for me.

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Lars Volker (Code Review)]

Lars Volker has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 1:

(3 comments)

Thanks for the reviews, please see PS2.

http://gerrit.cloudera.org:8080/#/c/5664/1/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

Line 34: """Test that /query_profile_encoded error message does not have 
any leading newlines.
> Not just no newlines, but the expected line in the case of a missing query.
Done


http://gerrit.cloudera.org:8080/#/c/5664/1/www/raw_text.tmpl
File www/raw_text.tmpl:

Line 17: under the License.
> We should also add a comment saying that the whitespace is critical so that
Done


Line 18: }}{{contents}}
> Any other templates like this that should be fixed?
I manually went through www/*.tmpl and this one was the only one that seemed to 
have the issue.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Lars Volker (Code Review)]

Hello David Knupp,

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

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

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

Change subject: IMPALA-4751: Remove blank line from raw_text template
..

IMPALA-4751: Remove blank line from raw_text template

The additional blank line can break tooling which uses the
/query_profile_encoded endpoint and has been erroneously introduced in
the fix for IMPALA-3918.

Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
---
M tests/webserver/test_web_pages.py
M www/raw_text.tmpl
2 files changed, 16 insertions(+), 2 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 2
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Jim Apple (Code Review)]

Jim Apple has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 1:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/5664/1/tests/webserver/test_web_pages.py
File tests/webserver/test_web_pages.py:

Line 34: """Test that /query_profile_encoded error message does not have 
any leading newlines.
Not just no newlines, but the expected line in the case of a missing query.


http://gerrit.cloudera.org:8080/#/c/5664/1/www/raw_text.tmpl
File www/raw_text.tmpl:

Line 18: }}{{contents}}
Any other templates like this that should be fixed?


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-Reviewer: Jim Apple 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[David Knupp (Code Review)]

David Knupp has posted comments on this change.

Change subject: IMPALA-4751: Remove blank line from raw_text template
..


Patch Set 1: Code-Review+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Lars Volker 
Gerrit-Reviewer: David Knupp 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4751: Remove blank line from raw text template

[Lars Volker (Code Review)]

Lars Volker has uploaded a new change for review.

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

Change subject: IMPALA-4751: Remove blank line from raw_text template
..

IMPALA-4751: Remove blank line from raw_text template

The additional blank line can break tooling which uses the
/query_profile_encoded endpoint and has been erroneously introduced in
the fix for IMPALA-3918.

Change-Id: I9b688aa9e2423b0271c8891a983e5b22707d8dbc
---
M tests/webserver/test_web_pages.py
M www/raw_text.tmpl
2 files changed, 12 insertions(+), 2 deletions(-)


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

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

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 1:

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/156/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4739: ExprRewriter fails on HAVING clauses

[Impala Public Jenkins (Code Review)]

Impala Public Jenkins has posted comments on this change.

Change subject: IMPALA-4739: ExprRewriter fails on HAVING clauses
..


Patch Set 1: Verified-1

Build failed: http://jenkins.impala.io:8080/job/gerrit-verify-dryrun/157/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: Ife74c61f549f620c42f74928f6474e8a5a7b7f00
Gerrit-PatchSet: 1
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Marcel Kornacker 
Gerrit-Reviewer: Dimitris Tsirogiannis 
Gerrit-Reviewer: Impala Public Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: No