[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 8: Verified+1

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Internal Jenkins (Code Review)]

Internal Jenkins has submitted this change and it was merged.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

AnayticExpr.analyze() replaces the original FIRST/LAST_VALUE
function with a FIRST/LAST_VALUE_IGNORE_NULLS function if
the IGNORE NULLS clause is specified.

The bug was that several places in AnalyticExpr.analyze() assumed
and asserted that only the original FIRST/LAST_VALUE function
could be encountered during analysis. However, with subquery
rewriting the IGNORE NULLS version of the function may also be
seen because the whole statement is re-analyzed after rewriting.

The fix is to unset the IGNORE NULLS flag of the function params
after changing the analytic function name.

Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Reviewed-on: http://gerrit.cloudera.org:8080/4732
Reviewed-by: Alex Behm 
Tested-by: Internal Jenkins
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionParams.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 17 insertions(+), 9 deletions(-)

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



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

Gerrit-MessageType: merged
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 9
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 8: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 8
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 6:

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/322/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 6:

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/320/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Internal Jenkins (Code Review)]

Internal Jenkins has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 6: Verified-1

Build failed: 
http://sandbox.jenkins.cloudera.com/job/impala-external-gerrit-verify-merge-ASF/319/

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 6
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Internal Jenkins
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 5: Code-Review+2

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 4:

(2 comments)

http://gerrit.cloudera.org:8080/#/c/4732/4/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java:

Line 771: 
> An alternative to having to check *_IGNORE_NULLS here and in analyze() is t
Thanks for the suggestion! I like your version better. Done.


PS4, Line 772:   fnCall_.setIsAnalyticFnCall(true);
 :   fnCall_.setIsInternalFnCall(true);
 :   fnCall_.analyzeNoThrow(analyzer);
 :   analyticFnName = getFnCall().getFnName();
 :   Preconditions.checkState(type_.equals(fnCall_.getType()));
> not a real problem, but we'll re-execute this. that won't happen if we unse
Done


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Alex Behm (Code Review)]

Alex Behm has uploaded a new patch set (#5).

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..

IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

AnayticExpr.analyze() replaces the original FIRST/LAST_VALUE
function with a FIRST/LAST_VALUE_IGNORE_NULLS function if
the IGNORE NULLS clause is specified.

The bug was that several places in AnalyticExpr.analyze() assumed
and asserted that only the original FIRST/LAST_VALUE function
could be encountered during analysis. However, with subquery
rewriting the IGNORE NULLS version of the function may also be
seen because the whole statement is re-analyzed after rewriting.

The fix is to unset the IGNORE NULLS flag of the function params
after changing the analytic function name.

Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionParams.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
3 files changed, 17 insertions(+), 9 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 5
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Matthew Jacobs (Code Review)]

Matthew Jacobs has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 4:

(2 comments)

I think this makes sense. I mention an alternative inline, I'm OK either way.

http://gerrit.cloudera.org:8080/#/c/4732/4/fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
File fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java:

Line 771: 
An alternative to having to check *_IGNORE_NULLS here and in analyze() is to 
create the rewritten FunctionCallExpr with params that have IgnoreNulls set 
false. Then we can check isIgnoreNulls() and know it applies to the same 
original functions only.


PS4, Line 772:   fnCall_.setIsAnalyticFnCall(true);
 :   fnCall_.setIsInternalFnCall(true);
 :   fnCall_.analyzeNoThrow(analyzer);
 :   analyticFnName = getFnCall().getFnName();
 :   Preconditions.checkState(type_.equals(fnCall_.getType()));
not a real problem, but we'll re-execute this. that won't happen if we unset 
the ignore nulls flag in params.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-Reviewer: Matthew Jacobs 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Alex Behm (Code Review)]

Alex Behm has uploaded a new patch set (#4).

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..

IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

AnayticExpr.analyze() replaces the original FIRST/LAST_VALUE
function with a FIRST/LAST_VALUE_IGNORE_NULLS function if
the IGNORE NULLS clause is specified.

The bug was that several places in AnalyticExpr.analyze() assumed
and asserted that only the original FIRST/LAST_VALUE function
could be encountered during analysis. However, with subquery
rewriting the IGNORE NULLS version of the function may also be
seen because the whole statement is re-analyzed after rewriting.

The fix is to also accept the IGNORE NULLS version of the function.

Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 23 insertions(+), 10 deletions(-)


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

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 4
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.

[Alex Behm (Code Review)]

Alex Behm has posted comments on this change.

Change subject: IMPALA-4301: Fix IGNORE NULLS with subquery rewriting.
..


Patch Set 3:

(1 comment)

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

Line 1599
> why remove this?
Initially I though this was a duplicate test (see L862), but I guess we can 
leave it since it covers a non-analytic function. Restored.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
Gerrit-PatchSet: 3
Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-Owner: Alex Behm 
Gerrit-Reviewer: Alex Behm 
Gerrit-Reviewer: Marcel Kornacker 
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-4301: Fix IGNORE NULLS when subquery rewriting.

[Alex Behm (Code Review)]

Alex Behm has uploaded a new change for review.

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

Change subject: IMPALA-4301: Fix IGNORE NULLS when subquery rewriting.
..

IMPALA-4301: Fix IGNORE NULLS when subquery rewriting.

AnayticExpr.analyze() replaces the original FIRST/LAST_VALUE
function with a FIRST/LAST_VALUE_IGNORE_NULLS function if
the IGNORE NULLS clause is specified.

The bug was that several places in AnalyticExpr.analyze() assumed
and asserted that only the original FIRST/LAST_VALUE function
could be encountered during analysis. However, with subquery
rewriting the IGNORE NULLS version of the function may also be
seen because the whole statement is re-analyzed after rewriting.

The fix is to also accept the IGNORE NULLS version of the function.

Change-Id: I708de7925fe6aeef582fd7510da93d24c71229d9
---
M fe/src/main/java/org/apache/impala/analysis/AnalyticExpr.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
2 files changed, 22 insertions(+), 13 deletions(-)


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

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