[Impala-ASF-CR] IMPALA-3710: All-null columns give wrong estimates in planner Modified the planner to handle low-value NDVs by adjusting them upward by one to account for null values. Thus, even an al

[Paul Rogers (Code Review)]

Paul Rogers has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11528 )

Change subject: IMPALA-3710: All-null columns give wrong estimates in planner 
Modified the planner to handle low-value NDVs by adjusting them upward by one 
to account for null values. Thus, even an all-null column, which has an NDV of 
0 in stats, will have an NDV of 1 in
..

IMPALA-3710: All-null columns give wrong estimates in planner
Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Table column (not an internal column such as COUNT(*))
* Column is nullable
* Column has stats
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

In this narrow case, we add one to NDV to account for nulls.
(Any larger adjustment throws off the TPC-H tests which have
multiple columns, marked as non-null, with low NDV, but which
actually include no nulls.)

The change minimized impact on PlannerTest, but still some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
12 files changed, 448 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-3710: All-null columns give wrong estimates in planner Modified the planner to handle low-value NDVs by adjusting them upward by one to account for null values. Thus, even an al

[Paul Rogers (Code Review)]

Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-3710: All-null columns give wrong estimates in planner 
Modified the planner to handle low-value NDVs by adjusting them upward by one 
to account for null values. Thus, even an all-null column, which has an NDV of 
0 in stats, will have an NDV of 1 in
..

IMPALA-3710: All-null columns give wrong estimates in planner
Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Table column (not an internal column such as COUNT(*))
* Column is nullable
* Column has stats
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

In this narrow case, we add one to NDV to account for nulls.
(Any larger adjustment throws off the TPC-H tests which have
multiple columns, marked as non-null, with low NDV, but which
actually include no nulls.)

The change minimized impact on PlannerTest, but still some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
12 files changed, 448 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..

IMPALA-7310: All-null columns give wrong estimates in planner

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Table column (not an internal column such as COUNT(*))
* Column is nullable
* Column has stats
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

In this narrow case, we add one to NDV to account for nulls.
(Any larger adjustment throws off the TPC-H tests which have
multiple columns, marked as non-null, with low NDV, but which
actually include no nulls.)

The change minimized impact on PlannerTest, but still some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
12 files changed, 448 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..

IMPALA-7310: All-null columns give wrong estimates in planner

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Table column (not an internal column such as COUNT(*))
* Column is nullable
* Column has stats
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

In this narrow case, we add one to NDV to account for nulls.
(Any larger adjustment throws off the TPC-H tests which have
multiple columns, marked as non-null, with low NDV, but which
actually include no nulls.)

The change minimized impact on PlannerTest, but still some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
12 files changed, 449 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..

IMPALA-7310: All-null columns give wrong estimates in planner

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Table column (not an internal column such as COUNT(*))
* Column is nullable
* Column has stats
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

In this narrow case, we add one to NDV to account for nulls.
(Any larger adjustment throws off the TPC-H tests which have
multiple columns, marked as non-null, with low NDV, but which
actually include no nulls.)

The change minimized impact on PlannerTest, but still some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
12 files changed, 450 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 6
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..

IMPALA-7310: All-null columns give wrong estimates in planner

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Table column (not an internal column such as COUNT(*))
* Column is nullable
* Column has stats
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

In this narrow case, we add one to NDV to account for nulls.
(Any larger adjustment throws off the TPC-H tests which have
multiple columns, marked as non-null, with low NDV, but which
actually include no nulls.)

The change minimized impact on PlannerTest, but still some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
11 files changed, 443 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..

IMPALA-7310: All-null columns give wrong estimates in planner

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Table column (not an internal column such as COUNT(*))
* Column is nullable
* Column has stats
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

In this narrow case, we add one to NDV to account for nulls.
(Any larger adjustment throws off the TPC-H tests which have
multiple columns, marked as non-null, with low NDV, but which
actually include no nulls.)

The change minimized impact on PlannerTest, but still some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
11 files changed, 455 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 8
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11544


Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..

IMPALA-7310: All-null columns give wrong estimates in planner

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459

Fixes

Change-Id: I282951f20598839fad880995a032e016845083db
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
10 files changed, 443 insertions(+), 23 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I282951f20598839fad880995a032e016845083db
Gerrit-Change-Number: 11544
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11528 )

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..


Patch Set 8:

(32 comments)

Thanks for the reviews. Here are my responses thus far. Most are of the form 
"did it." Vuk, I had a couple of questions on your comments.

Thanks,

- Paul

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

http://gerrit.cloudera.org:8080/#/c/11528/2//COMMIT_MSG@7
PS2, Line 7: IMPALA-7310: All-null columns give wrong estimates in planner
> We tend to have a single line subject with a newline after it. That ends up
Other projects require the JIRA ticket number in the commit message. Do we? 
Such projects tend to favor the JIRA ticket title (or a cleaned up version) as 
the description. Do we? In these projects, the assumption is that the patch is 
to fix the listed ticket, so no need to include the word "Fix" in the patch 
title.

Happy to remove the ticket number and change the title to "Correct NDV 
estimates for all-null columns."

I pushed an update with the newline which shows up in my view of the patch.

Did that revision replace the one here on which you commented? Is there a 
better way to revise the commit message?


http://gerrit.cloudera.org:8080/#/c/11528/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11528/8//COMMIT_MSG@24
PS8, Line 24: * Table column (not an internal column such as COUNT(*))
> Base table?
Ack


http://gerrit.cloudera.org:8080/#/c/11528/8//COMMIT_MSG@27
PS8, Line 27: not provide a null count
> when does this happen when stats are computed?
See revised message. I saw multiple cases in the test DBs in which stats were 
computed, but null count was -1 (undefined), so I needed to handle that case in 
order to get things to work.


http://gerrit.cloudera.org:8080/#/c/11528/8//COMMIT_MSG@28
PS8, Line 28: NDV <= 1
> if its unknown whether there are nulls or not (from the prev. line), could 
Added to the commit message:

Research for this patch revealed that Impala treats NDVs in two distinct ways:  
   

* Stats (which use the NDV function) computes NDV as the number of distinct 
non-null values. (That is, the NDV of (0, null) is 1.
* The planner itself when working with constants, uses a definition of NDV that 
includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know that the NDV in 
stats excludes nulls (except for the BOOLEAN type), and we know that the column 
contains nulls, we can bump up the NDV to convert from the stats definition to 
the planner definition. But, to avoid regressions, we do so in a very narrow 
range of NDV values: only 0 and 1.


http://gerrit.cloudera.org:8080/#/c/11528/8//COMMIT_MSG@31
PS8, Line 31: Any larger adjustment
> confused... if we're adjusting for nulls, and we're treating a null as a si
Reworded:

Technically, the adjustment should apply to all NDV values. However, it turns 
out that if we do so, we end up with many failures in PlannerTest in those 
tests that work with TPC-H tables. The TPC-H tables have multiple columns 
marked as nullable but which actually have no nulls. Some of these columns also 
have a low NDV.  

By limiting the NDV adjustment to the narrow range, the TPC-H tests need not be 
updated.  Since end users could have a similar situation, the narrow range 
reduces the chance that this fix might impact such workloads.


http://gerrit.cloudera.org:8080/#/c/11528/2/.gitignore
File .gitignore:

http://gerrit.cloudera.org:8080/#/c/11528/2/.gitignore@48
PS2, Line 48:
> I'm not a stickler, but you could separate the .gitignore stuff into a sepa
Reverted all changes to this file.


http://gerrit.cloudera.org:8080/#/c/11528/8/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
File fe/src/main/java/org/apache/impala/analysis/SelectStmt.java:

http://gerrit.cloudera.org:8080/#/c/11528/8/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@111
PS8, Line 111:   public ArrayList getGroupList() { return groupingExprs_; 
}
> where is this used in this change?
Was used in a test, but that test ended up not being include as part of this 
patch, so removed the line.


http://gerrit.cloudera.org:8080/#/c/11528/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java
File fe/src/main/java/org/apache/impala/analysis/SlotRef.java:

http://gerrit.cloudera.org:8080/#/c/11528/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@102
PS2, Line 102:   private void computeNdv() {
> line has trailing whitespace
Ack


http://gerrit.cloudera.org:8080/#/c/11528/2/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@105
PS2, Line 105: // Potentially adjust NDV for nulls if the column is has 
stats,
> This used to execute even when hasStats() was false. What's numDistinctValu
Defaults to -1.

Put it back as it

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..

IMPALA-7310: All-null columns give wrong estimates in planner

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullTable/large_data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
10 files changed, 459 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 9
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: Use NDV=1 for a Column with all nulls
..

Use NDV=1 for a Column with all nulls

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Bug: IMPALA-7310: All-null columns give wrong estimates in planner
Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullRows/data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
10 files changed, 452 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 10
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11528 )

Change subject: Use NDV=1 for a Column with all nulls
..


Patch Set 10:

(3 comments)

Thanks for the continued reviews. Cleaned up a few more items. Answered some 
questions.

http://gerrit.cloudera.org:8080/#/c/11528/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11528/8//COMMIT_MSG@31
PS8, Line 31:
> what types of changes are these-- did they manage to change the structure o
Sorry. The note is trying to explain that 1) there is a planner change (that is 
the gist of the problem resolution), and 2) the change occurs only in the very 
narrow conditions spelled out in this note.

The line in question here explains why the adjustment is applied only to NDV of 
0-1 (and not, say, 0,-10). If we have NDV=10, and column is nullable, then we 
certainly might want to adjust NDV to 11. But, if we do, many tests fail due to 
changed cardinality, changed memory and so on. (The tests "fail" in the sense 
that the text plans have changed. The structure is the same, just the numbers 
differ.)

The original bug complained that, with NDV=0, the planner did misfire, did 
produce an incorrect plan. It did so because, in aggregation, we multiply 
cardinalities by 0, resulting in an overall cardinality of 0.

The new tests verify that, after the adjustments, cardinality estimates are no 
longer zeroed out in aggregate nodes, but instead bubble up. This will, 
presumably, fix the original problem. (The original bug did not give sufficient 
information to completely reproduce the particular query that failed.)


http://gerrit.cloudera.org:8080/#/c/11528/8/testdata/NullTable/large_data.csv
File testdata/NullTable/large_data.csv:

http://gerrit.cloudera.org:8080/#/c/11528/8/testdata/NullTable/large_data.csv@1
PS8, Line 1:
> Looks like theres several conventions under testdata... I see testdata/data
Created a new NullRows folder, moved this file there and renamed it to 
"data.csv".


http://gerrit.cloudera.org:8080/#/c/11528/8/testdata/datasets/functional/functional_schema_template.sql
File testdata/datasets/functional/functional_schema_template.sql:

http://gerrit.cloudera.org:8080/#/c/11528/8/testdata/datasets/functional/functional_schema_template.sql@1355
PS8, Line 1355: 
> I have no idea, but I counted BASE_TABLE_NAME and see it shows up 105 times
Removed the Kudu portion.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 10
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 02 Oct 2018 19:53:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7310: All-null columns give wrong estimates in planner

[Paul Rogers (Code Review)]

Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/11544 )

Change subject: IMPALA-7310: All-null columns give wrong estimates in planner
..


Abandoned

Duplicate of 11528
--
To view, visit http://gerrit.cloudera.org:8080/11544
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I282951f20598839fad880995a032e016845083db
Gerrit-Change-Number: 11544
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..

IMPALA-7310: Use NDV=1 for a Column with all nulls

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Bug: IMPALA-7310: All-null columns give wrong estimates in planner
Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullRows/data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
10 files changed, 452 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 11
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..

IMPALA-7310: Use NDV=1 for a Column with all nulls

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Bug: IMPALA-7310: All-null columns give wrong estimates in planner
Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullRows/data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
10 files changed, 457 insertions(+), 23 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 12
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11528 )

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..


Patch Set 12:

(3 comments)

> Uploaded patch set 12.

Addressed a few more review comments. Rebased on master.

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

http://gerrit.cloudera.org:8080/#/c/11528/10//COMMIT_MSG@71
PS10, Line 71: Bug: IMPALA-7310: All-null columns give wrong estimates in 
planner
> We almost always put "IMPALA-7310:" in the first line of the commit message
Fixed.


http://gerrit.cloudera.org:8080/#/c/11528/10/fe/src/main/java/org/apache/impala/analysis/SlotRef.java
File fe/src/main/java/org/apache/impala/analysis/SlotRef.java:

http://gerrit.cloudera.org:8080/#/c/11528/10/fe/src/main/java/org/apache/impala/analysis/SlotRef.java@110
PS10, Line 110: // not zero.
> Perhaps: "if we don't know the null count (in which case, getNumNulls() ret
Done


http://gerrit.cloudera.org:8080/#/c/11528/8/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
File testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test:

http://gerrit.cloudera.org:8080/#/c/11528/8/testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test@a986
PS8, Line 986:
> test
Is this comment itself a test, or is this a request to test something?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 12
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Fri, 12 Oct 2018 23:37:30 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7501: Slim down metastore Partition objects in LocalCatalog cache

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11688


Change subject: IMPALA-7501: Slim down metastore Partition objects in 
LocalCatalog cache
..

IMPALA-7501: Slim down metastore Partition objects in LocalCatalog cache

Used existing method to remove cached per-partition column information
as requested in the JIRA ticket. See IMPALA-7501 for details.

Tested by inspecting a heap dump before and after the change to ensure
that the column schema is no longer referenced from the Hive Partition
objects. (At present, we have no automated way to inspect a heap dump.)

Change-Id: I4c861452cf974970ab511406fe2e175ea3dc668d
---
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java
2 files changed, 7 insertions(+), 3 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4c861452cf974970ab511406fe2e175ea3dc668d
Gerrit-Change-Number: 11688
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7715: [DOCS] Better descriptions for conditional functions

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11704 )

Change subject: IMPALA-7715: [DOCS] Better descriptions for conditional 
functions
..


Patch Set 2:

(4 comments)

Thanks for making these changes so quickly! A few minor suggestions.

http://gerrit.cloudera.org:8080/#/c/11704/2/docs/topics/impala_conditional_functions.xml
File docs/topics/impala_conditional_functions.xml:

http://gerrit.cloudera.org:8080/#/c/11704/2/docs/topics/impala_conditional_functions.xml@341
PS2, Line 341:   FALSE if the expression is true or 
NULL.
Should the other refs to boolean constants be called out? "is false" --> "is 
FALSE"? Also for true?


http://gerrit.cloudera.org:8080/#/c/11704/2/docs/topics/impala_conditional_functions.xml@345
PS2, Line 345:   
After reading your other change, it appears we support IS FALSE. So, we might 
add: "Same as the IS FALSE operator."


http://gerrit.cloudera.org:8080/#/c/11704/2/docs/topics/impala_conditional_functions.xml@445
PS2, Line 445:   FALSE if the expression is false or 
NULL.
"an expression" --> "the expression". Code font for true and false.


http://gerrit.cloudera.org:8080/#/c/11704/2/docs/topics/impala_conditional_functions.xml@449
PS2, Line 449:   
Add comment "Is same as the IS TRUE operator."

Maybe also do this for "isnottrue() - IS NOT TRUE" and "isnotfalse() - IS NOT 
FALSE"



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5cc90d62645730d2674bcb3af614863aa92b92f6
Gerrit-Change-Number: 11704
Gerrit-PatchSet: 2
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Wed, 17 Oct 2018 01:28:18 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7715: [DOCS] Better descriptions for conditional functions

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11704 )

Change subject: IMPALA-7715: [DOCS] Better descriptions for conditional 
functions
..


Patch Set 3: Code-Review+1

Looks good. The changes wonderfully address the ambiguity in the original docs. 
Thanks!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5cc90d62645730d2674bcb3af614863aa92b92f6
Gerrit-Change-Number: 11704
Gerrit-PatchSet: 3
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Wed, 17 Oct 2018 18:41:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-5004: Switch to sorting node for large TopN queries

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11698 )

Change subject: IMPALA-5004: Switch to sorting node for large TopN queries
..


Patch Set 3:

(1 comment)

This will be a great change. See comment about using a memory-based decision 
rather than, say, a row-based decision or a decision informed by the memory 
available to the query.

http://gerrit.cloudera.org:8080/#/c/11698/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
File fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java:

http://gerrit.cloudera.org:8080/#/c/11698/3/fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java@313
PS3, Line 313:   estimatedTopNMaterializedSize < 
ctx_.getQueryOptions().topn_bytes_limit;
This code makes the sort/top-n decision based on memory. The number of bytes is 
a parameter. This seems awkward: the memory used must be part of the overall 
query budget. A query could, in odd cases, have multiple TopN operators, but 
the code here treats them one by one.

Further, a TopN will always use fewer bytes than a Sort: a TopN needs to keep 
only n rows in general, but sort must buffer all rows. (Though, of course, sort 
can spill to relieve memory pressure.)

I wonder, does it make sense to impose the limit as a row limit rather than a 
memory limit?

Or, does it make sense to set the limit as part of a query memory planning 
exercise rather than as another free parameter that the user must juggle when 
thinking about memory budgets?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I34c9db33c9302b55e9978f53f9c7061f2806c8a9
Gerrit-Change-Number: 11698
Gerrit-PatchSet: 3
Gerrit-Owner: Sahil Takiar 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Lars Volker 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Sahil Takiar 
Gerrit-Comment-Date: Thu, 18 Oct 2018 21:15:27 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 16:

(5 comments)

Good stuff. A few random comments.

http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc
File be/src/codegen/codegen-anyval.cc:

http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc@469
PS16, Line 469: void CodegenAnyVal::ConvertToCanonicalForm() {
This code handles the codegen case. Is there an interpreted code path change 
that is needed for when codegen is disabled?


http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc@728
PS16, Line 728: llvm::Value* CodegenAnyVal::EqToNativePtr(llvm::Value* 
native_ptr,
Maybe add a comment to define inclusive equality? The description in the commit 
message is good, maybe just add it here for future reference.


http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/codegen/codegen-anyval.cc@751
PS16, Line 751:   // Mirror logic in HashTableCtx::Equals - IMPALA-6661
Maybe explain why we're mirroring the logic? So that two NaN compare as equal? 
Or, so that they have the same hash code? Both?


http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/exec/hash-table.cc
File be/src/exec/hash-table.cc:

http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/exec/hash-table.cc@238
PS16, Line 238: bool HashTableCtx::Equals(const TupleRow* build_row, const 
uint8_t* expr_values,
Per earlier comment, maybe a comment here about what "INCLUSIVE" means? 
(Parallel handling of NULL and NaN.)


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

http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3074
PS16, Line 3074:  QUERY
A quick check of this file found no cases where we test base NaN functionality. 
According to Wikipedia(https://en.wikipedia.org/wiki/NaN), NaN has specific 
rules. Here we test IS DISTINCT, but should we add base tests for other 
operators? Having such tests will catch any regressions in the future if 
someone accidentally turns on the new mode at the wrong time.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 16
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 18 Oct 2018 22:11:14 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7659: Simplify expression to collect NULLs count

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11565 )

Change subject: IMPALA-7659: Simplify expression to collect NULLs count
..


Patch Set 5:

(2 comments)

A few comments.

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

http://gerrit.cloudera.org:8080/#/c/11565/5//COMMIT_MSG@10
PS5, Line 10: IMPALA-7655.
This fix would be helpful for IMPALA-7310: if NDV=0, we need to know if 
nullCount > 0 to know if we should adjust NDV upward by one. (The planner 
includes nulls in NDV, statistics don't.)


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

http://gerrit.cloudera.org:8080/#/c/11565/5/fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java@251
PS5, Line 251:   " IS NULL THEN 1 ELSE NULL END)");
If this expression is faster than an if() function (IMPALA-7655), then does it 
makes sense to optimize the if() function in the planner so it benefits all 
queries rather than just changing this one?



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ic68f8b4c3756eb1980ce299a602a7d56db1e507a
Gerrit-Change-Number: 11565
Gerrit-PatchSet: 5
Gerrit-Owner: Anonymous Coward 
Gerrit-Reviewer: Anonymous Coward 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 18 Oct 2018 22:25:52 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-6661 Make NaN values equal for grouping purposes.

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11535 )

Change subject: IMPALA-6661 Make NaN values equal for grouping purposes.
..


Patch Set 16:

(3 comments)

A few more comments as I learn this code.

http://gerrit.cloudera.org:8080/#/c/11535/16//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11535/16//COMMIT_MSG@17
PS16, Line 17:   additional logic to consider NaN values as equal.
The comment mentions equality. Is the rule that NaN = NaN is always false? But 
that Nan <=> NaN is true?

Looks like the IS DISTINCT (<=>) code is implemented in operators-ir.cc, but 
that file is not in this patch set. Does the implementation of IS DISTINCT need 
to change?


http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/runtime/raw-value.cc
File be/src/runtime/raw-value.cc:

http://gerrit.cloudera.org:8080/#/c/11535/16/be/src/runtime/raw-value.cc@33
PS16, Line 33: const float RawValue::CANONICAL_FLOAT_NAN = nanf("");
NaN is not a single bit pattern (which is why you've defined a canonical form), 
rather it is a family of bit patterns. Do the above functions guarantee to 
return the exact same bit pattern on all OSs, all versions, and all runs?

Or, should we use the LLVM trick to specify the exact bit pattern we want to 
use for NAN so that it is guaranteed to be the same on all hosts in the 
cluster, even if they use slightly different CPUs or library versions?


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

http://gerrit.cloudera.org:8080/#/c/11535/16/testdata/workloads/functional-query/queries/QueryTest/exprs.test@3074
PS16, Line 3074:  QUERY
> I've assumed that NaN tests have existed in other places.  Exhaustive valid
The thought is that we might want to ensure all paths work to handle the NaN 
case, not just the grouping case.

For example, we we have tests for all three (I believe) places that this can 
occur:

* SELECT clause: SELECT sqrt(-1) op floatCol ...
* WHERE clause: WHERE sqrt(-1) op floatCol ...
* JOIN: what is tested here.

Along with two operators

* = (NaN's are not equal)
* <=> (NaN's are equal)

We also have both the code gen and interpreted cases:

* SELECT: never code generated (when in the root fragment)
* WHERE, JOIN: code generated if enough rows, interpreted otherwise

The thought is that we might want to cover all these cases just to save the 
hassle of fixing any gaps later.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I996c4a2e1934fd887046ed0c55457b7285375086
Gerrit-Change-Number: 11535
Gerrit-PatchSet: 16
Gerrit-Owner: Michal Ostrowski 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Michael Ho 
Gerrit-Reviewer: Michal Ostrowski 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Thomas Marshall 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Mon, 22 Oct 2018 18:04:58 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7740: [DOCS] Correct description of NVL2 function

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11758 )

Change subject: IMPALA-7740: [DOCS] Correct description of NVL2 function
..


Patch Set 1: Code-Review+1

Thanks for the quick fix!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f6b9d56e85f7dffeb29218b244af1cc535dc03e
Gerrit-Change-Number: 11758
Gerrit-PatchSet: 1
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Tue, 23 Oct 2018 21:01:43 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the code-generated
CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The functions ifnull and nvl are aliases for isnull, so revised those to
directly rewrite them to CASE rather than first converting them to if().

Moved rewriting into our existing set of conditional rewrite rules
instead of in the parser. To make this work (and to solve the issue in
IMPALA-7741, added ifnull and nvl to the list of builtin Impala
functions.

Found a few additional simplifications for coalesce, ifnull and nvl. See
the Javadoc and unit tests for details.

Tests for conditional functions were in one huge function along with
other rewrite tests. Moved them into a new file, then broke up the tests
by function to allow much easier debugging of each function one-by-one.
This required moving the common test mechanims into a new common base
class.

Once tests verified that the rewrites worked, we no longer have need of
the specialized C++ implementations, so removed those.

Also cleaned up a few typos, unnecessary imports and JavaDocs found
during this work.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M be/src/exprs/scalar-expr.h
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java
13 files changed, 586 insertions(+), 485 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7739 IMPALA-7740: [DOCS] Correct descriptions of NVL2 and DECODE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11758 )

Change subject: IMPALA-7739 IMPALA-7740: [DOCS] Correct descriptions of NVL2 
and DECODE
..


Patch Set 4: Code-Review+1

Very nice. Great improvement!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7f6b9d56e85f7dffeb29218b244af1cc535dc03e
Gerrit-Change-Number: 11758
Gerrit-PatchSet: 4
Gerrit-Owner: Alex Rodoni 
Gerrit-Reviewer: Alex Rodoni 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Wed, 24 Oct 2018 00:30:14 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 2:

(6 comments)

http://gerrit.cloudera.org:8080/#/c/11760/2/be/src/exprs/scalar-expr.h
File be/src/exprs/scalar-expr.h:

http://gerrit.cloudera.org:8080/#/c/11760/2/be/src/exprs/scalar-expr.h@84
PS2, Line 84: /// necessary on the arguments to generate the result.These 
compute functions have
> Period after space; this change seems accidental.
Done


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

http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@89
PS2, Line 89:* IF(TRUE, then, else) → 
then
:* IF(FALSE|NULL, then, else) → 
else
> Do we need to do this? The general case of simplifyCaseExpr() should take c
Removing optimizations worked here.


http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@122
PS2, Line 122:* Rewrites IFNULL(a, x):
> Same comment: can we just always make this a case, and then other simplific
Tried removing the optimizations. Turns out that the CASE simplification did 
not discover these optimizations. See IMPALA-7750.


http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@175
PS2, Line 175:   // TODO: Clone child expr here?
> Do you know what the answer is here?
Done


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

http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java@1
PS2, Line 1: package org.apache.impala.analysis;
> Please copy over the regular license stuff here.
Done


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

http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java@35
PS2, Line 35: "CASE WHEN id = 0 THEN TRUE ELSE FALSE END");
> Should this be simplified further to "id = 0"? Or do we think it doesn't ma
Subtle issue. The expression, as written, is equivalent to istrue(id = 0). By 
itself, id = 0 can be true, false or null. So, we could not eliminate the CASE 
operator, though we could rewrite it to istrue(id = 0).

Of course, new could also rewrite isTrue(x) to be CASE x THEN true ELSE FALSE 
END ...



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 24 Oct 2018 01:17:20 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Tim Armstrong, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the code-generated
CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The functions ifnull and nvl are aliases for isnull, so revised those to
directly rewrite them to CASE rather than first converting them to if().

Moved rewriting into our existing set of conditional rewrite rules
into SimplifyConditionalRules instead of in the parser. To make this work
(and to solve the issue in IMPALA-7741, added ifnull and nvl to the list
of builtin Impala functions.

Found a few additional simplifications for coalesce, ifnull and nvl. See
the Javadoc and unit tests for details. (See IMPALA-7750 for why the
rules are needed; solve IMPALA-7750 and the rules can be removed as the
rewrite for CASE will handle them.)

Tests for conditional functions were in one huge function along with
other rewrite tests. Moved them into a new file, then broke up the tests
by function to allow much easier debugging of each function one-by-one.
This required moving the common test mechanims into a new common base
class.

Once tests verified that the rewrites worked, we no longer have need of
the specialized C++ implementations, so removed those.

Also cleaned up a few typos, unnecessary imports and JavaDocs found
during this work.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java
12 files changed, 600 insertions(+), 493 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Tim Armstrong, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the code-generated
CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The functions ifnull and nvl are aliases for isnull, so revised those to
directly rewrite them to CASE rather than first converting them to if().

Moved rewriting into our existing set of conditional rewrite rules
into SimplifyConditionalRules instead of in the parser. To make this work
(and to solve the issue in IMPALA-7741, added ifnull and nvl to the list
of builtin Impala functions.

Found a few additional simplifications for coalesce, ifnull and nvl. See
the Javadoc and unit tests for details. (See IMPALA-7750 for why the
rules are needed; solve IMPALA-7750 and the rules can be removed as the
rewrite for CASE will handle them.)

Tests for conditional functions were in one huge function along with
other rewrite tests. Moved them into a new file, then broke up the tests
by function to allow much easier debugging of each function one-by-one.
This required moving the common test mechanims into a new common base
class.

Once tests verified that the rewrites worked, we no longer have need of
the specialized C++ implementations, so removed those.

Also cleaned up a few typos, unnecessary imports and JavaDocs found
during this work.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M be/src/exprs/conditional-functions-ir.cc
M be/src/exprs/conditional-functions.cc
M be/src/exprs/conditional-functions.h
M be/src/exprs/scalar-expr.cc
M common/function-registry/impala_functions.py
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/SimplifyConditionalRulesTest.java
12 files changed, 617 insertions(+), 493 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@89
PS2, Line 89:* code gen for , avoiding the need for ad-hoc 
implementations
:* of each function.
> Removing optimizations worked here.
Further testing revealed complexities. First, the rewrite engine does not apply 
the same rule multiple times (rewrite function, then rewrite resulting case.)

If the code is changed to apply CASE rewrite to the result, additional problems 
result. One of which is that the CASE rewrites will revert to the original 
expression if all aggregates are removed. But, we can't do that here: we no 
longer have function implementations. So, we would need the aggregate fallback 
to fallback to the CASE rewritten from the function.

The resulting code gets pretty complex and looks hard to maintain. So, I'm 
thinking to revert to the code in the first patch, with the added subtlety of 
handling aggregates in the function rewrites.

Stay tuned for a revised version.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 24 Oct 2018 16:28:25 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 4:

(12 comments)

Thanks everyone for the comments.

PhilZ's note to use existing optimizations is a good one. A number of changes 
are required to make that happen. Researching that turned up quite a few bugs 
listed in IMPALA-7655. I plan to push changes for those bugs one by one to ease 
reviews. Once those fixes are in master, I'll push a new change set here 
rebased on the revised master.

http://gerrit.cloudera.org:8080/#/c/11760/4/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/11760/4/common/function-registry/impala_functions.py@531
PS4, Line 531:   # Rewritten away in the FE
> can you explain why this is needed in this change?
See IMPALA-7741. nvl2() currently does not appear in the list of built-in 
functions. However, since this improvement is orthogonal to the topic of this 
bug, I'll drop this change from here and do a separate change set for 
IMPALA-7741.


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

http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@a92
PS4, Line 92:
> rewrites can be disabled via a query option so cannot be assumed to run. it
Right. Turns out that I had to alter the rule application flow to always 
rewrite the conditional functions.

The other choice is to leave the current interpreted implementations and have 
two paths based on turning on optimizations or not. In the spirit of "simpler 
is better", seems to make sense to have only one implementation, but the 
two-implementation solution is also possible.

Though, note, in the original code, nvl2() was always rewritten, there never 
was a BE implementation, so that aspect is not really changing.


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

http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@89
PS2, Line 89:* code gen for , avoiding the need for ad-hoc 
implementations
:* of each function.
> The following code looks to me like it'll do re-writes until we're "stuck".
Turns out this is a difference between the simplified test framework (which 
applies selected rules once) and the production code (which replies rules 
repeatedly.) Easy enough to create a separate test that will apply rules in the 
production way so that we take multiple passes to verify all rewrites.

Making this change uncovered quite a few obscure bugs which are recorded in the 
JIRA and will be submitted as separate changes for review. Once those are in, 
I'll rebase this change onto that code, and implement the chained rewrite test 
and logic.


http://gerrit.cloudera.org:8080/#/c/11760/2/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@122
PS2, Line 122:   private Expr rewriteIfFn(FunctionCallExpr expr) {
> Is the issue that "null is null" is not getting constant optimization? In m
See earlier note.


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

http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@112
PS4, Line 112: 
> to be consistent with the style in the frontend Java code, pls remove these
Really? We create Javadoc comments, but don't use the resulting formatting? 
Eclipse, for example, will display the formatting in hovers. Without proper 
HTML formatting, the result is a bit of a mess.

Still, if our policy is to use Javadoc comments, but without formatting, I can 
revert this stuff.


http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@144
PS4, Line 144: instanceof NullLiteral)
> I would prefer .isNullLiteral() as other functions use that.
This is existing code that was moved. However, I agree with your point. I've 
gone though and cleaned up places that use this older style to use the newer 
predicates, including in cases where we test for true and false.


http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@183
PS4, Line 183: revised.size() - 1;
> Can you add a comment about not adding the last child here? It took me some
Done


http://gerrit.cloudera.org:8080/#/c/11760/4/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditio

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 4:

(1 comment)

http://gerrit.cloudera.org:8080/#/c/11760/4/common/function-registry/impala_functions.py
File common/function-registry/impala_functions.py:

http://gerrit.cloudera.org:8080/#/c/11760/4/common/function-registry/impala_functions.py@531
PS4, Line 531:   # Rewritten away in the FE
> See IMPALA-7741. nvl2() currently does not appear in the list of built-in f
Actually, this is a bit more subtle. Today, we rewrite nvl2() and nullif() at 
parse time. The goal is to rewrite them in the rewrite rules, along with other 
conditionals. Doing so allows us to take advantage of analyzed arguments to 
implement optimizations.

To do that, however, we need the function to be defined in this function table. 
We need it as a FE-only function (which also allows it to appear in the table 
of built-in functions) so it passes analysis, even though there is no BE 
implementation.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 25 Oct 2018 18:52:03 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] Update .gitignore

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11787 )

Change subject: Update .gitignore
..


Patch Set 3: Code-Review+1

Thanks! I've found a few more additions, many around the use of Eclipse. 
Probably easiest if I add those after you commit this set.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I281ab3b5c98ac32e5d60663562628ffda6606a6a
Gerrit-Change-Number: 11787
Gerrit-PatchSet: 3
Gerrit-Owner: Tim Armstrong 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 25 Oct 2018 18:56:48 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7501: Slim down metastore Partition objects in LocalCatalog cache

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11688 )

Change subject: IMPALA-7501: Slim down metastore Partition objects in 
LocalCatalog cache
..


Patch Set 1:

Hey Todd, you asked for this fix. Can you take a look at this change? Thanks!


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c861452cf974970ab511406fe2e175ea3dc668d
Gerrit-Change-Number: 11688
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Thu, 25 Oct 2018 23:25:46 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7501: Slim down metastore Partition objects in LocalCatalog

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Todd Lipcon,

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

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

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

Change subject: IMPALA-7501: Slim down metastore Partition objects in 
LocalCatalog
..

IMPALA-7501: Slim down metastore Partition objects in LocalCatalog

Used existing method to remove cached per-partition column information
as requested in the JIRA ticket. See IMPALA-7501 for details.

Then, revised catalogd to not include the unwanted partition schema
info when re-creating an HMS parition object.

Cleaned up a few unused imports in test files used during this effort.

Tested by inspecting a heap dump before and after the change to ensure
that the column schema is no longer referenced from the Hive Partition
objects. (At present, we have no automated way to inspect a heap dump.)

Change-Id: I4c861452cf974970ab511406fe2e175ea3dc668d
---
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java
M fe/src/test/java/org/apache/impala/catalog/CatalogTest.java
M fe/src/test/java/org/apache/impala/catalog/PartialCatalogInfoTest.java
5 files changed, 5 insertions(+), 15 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c861452cf974970ab511406fe2e175ea3dc668d
Gerrit-Change-Number: 11688
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] IMPALA-7501: Slim down metastore Partition objects in LocalCatalog

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11688 )

Change subject: IMPALA-7501: Slim down metastore Partition objects in 
LocalCatalog
..


Patch Set 2:

(2 comments)

Updated with changes from code review comments.

http://gerrit.cloudera.org:8080/#/c/11688/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
File fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java:

http://gerrit.cloudera.org:8080/#/c/11688/1/fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java@1169
PS1, Line 1169:   // The per-partition schema information is not used by 
Impala.
> good idea, that would reduce network bandwidth noticeably considering we do
Good idea. In looking into this, the catalogd caches Impala-specific objects, 
then recreates an HMS Partition object, passing in the table schema when 
creating the StorageDescriptor(). The fix is to simply not add the schema.

When running in local catalog mode, we hit this line, with the original 
Partition object, so we still need this line.

There was discussion on some list or other about the idea of caching the Impala 
catalog objects rather than the HMS objects. The Impala objects appear to be 
stripped down. By caching HMS objects, we will find ourselves going through the 
same process that likely led to those light-weight Impala objects.


http://gerrit.cloudera.org:8080/#/c/11688/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java
File fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java:

http://gerrit.cloudera.org:8080/#/c/11688/1/fe/src/main/java/org/apache/impala/catalog/local/LocalFsPartition.java@71
PS1, Line 71:
> checkArgument
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c861452cf974970ab511406fe2e175ea3dc668d
Gerrit-Change-Number: 11688
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Todd Lipcon 
Gerrit-Comment-Date: Fri, 26 Oct 2018 22:33:23 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, 
Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The rewrite engine has many bugs that are beyond the scope of
this change to fix. This change codes around those bugs. The
result is that the conditional rewrite happens some of the time,
and sometimes produces less-than perfect optimizations.

Conditionals in the top-level ORDER BY clause are not rewritten
(IMPALA-7753), but those one or more levels down are. Some expressions
involving NULL are not simplified (IMPALA-7769). Several hacks were
used to work around the fact that the rewrite engine ignores unanalyzed
expressions, yet the rewrite engine does not, in general, re-analyze
the expressions it produces, causing simplifications to be skipped
(IMPALA-7754). And so on.

As a result, the BE retains the original interpreted forms that
are still used in two cases: 1) top-leel conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Further, code generation does not occur for CASE statements in the
SELECT clause when it is in the root fragment (the most common case
in simple tests.) This is another known bug (IMPALA-4356).

One possible performance regression is that the new form of the code
evaluates some expressions twice, where the original interpreted
code evaluated the argument once. E.g. coalesce(id, 10) is rewritten
to CASE WHEN id IS NULL THEN id ELSE 10 END. Here, id is evaluated
twice. If the "id" were replaced by a complex sub-expression, the
gain from compilation could be offset by doing work twice.
(IMPALA-7737)

Still, the fix provides most of what the JIRA ticket requested
within the limitations of the existing code.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Most functions use the simplest possible rewrite, relying on the
existing rewrite rules for further simplification.  The one exception
is coalesce(): the existing code relies on the semantics of the
function and so was retained and slightly improved. The code was
extended to produce a CASE statement directly, retaining existing
simplifications.

Tests for conditional functions were in one large function along with
other rewrite tests. Moved them into a new file, then broke up the
tests by function to allow much easier debugging of each function
one-by-one.  This required moving the common test mechanims into a
new common base class.

Existing tests focus on one or two rules at a time. The conditional
function rewrite, however, relies on the entire set of rules being
applied repeatedly. So, added a new FullRewriteTest case to verify this
behavior. This class contains several commented-out tests that cannot
pass due to existing rewrite bugs noted above.

Changing the rewrite cause the PlannerTest to produce different plans
than previously. Changed the expected results file to match the new
rewrite rules.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
16 files changed, 831 insertions(+), 309 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/11760/5
--
To view, visit http://gerrit.cloudera.org:8080/11760
To unsubscribe, visit http://gerrit.clou

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 5:

Thanks everyone for your patient reviews of this change. Per PhilZ, removed the 
per-function optimizations, relying on existing rewrite rules (bugs and all). 
Added tests to verify that the "downstream" rules work (when they do.)

Per Vuk, reduced the scope to only rewrite the requested functions, and to 
retain the BE implementation; living with the bugs that prevent the rewrites 
from being done some of the time.

Added more tests, including tests of the full rewrite process to ensure that 
the conditional functions are, in fact, first rewritten to use CASE, then CASE 
is further simplified. Tests include commented-out cases to record where we 
decided to live with existing bugs.

The result is the minimal change that accomplishes the request in the JIRA 
without breaking anything.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Mon, 29 Oct 2018 20:51:02 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, 
Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The rewrite engine has many bugs that are beyond the scope of
this change to fix. This change codes around those bugs. The
result is that the conditional rewrite happens some of the time,
and sometimes produces less-than perfect optimizations.

Conditionals in the top-level ORDER BY clause are not rewritten
(IMPALA-7753), but those one or more levels down are. Some expressions
involving NULL are not simplified (IMPALA-7769). Several hacks were
used to work around the fact that the rewrite engine ignores unanalyzed
expressions, yet the rewrite engine does not, in general, re-analyze
the expressions it produces, causing simplifications to be skipped
(IMPALA-7754). And so on.

As a result, the BE retains the original interpreted forms that
are still used in two cases: 1) top-leel conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Further, code generation does not occur for CASE statements in the
SELECT clause when it is in the root fragment (the most common case
in simple tests.) This is another known bug (IMPALA-4356).

One possible performance regression is that the new form of the code
evaluates some expressions twice, where the original interpreted
code evaluated the argument once. E.g. coalesce(id, 10) is rewritten
to CASE WHEN id IS NULL THEN id ELSE 10 END. Here, id is evaluated
twice. If the "id" were replaced by a complex sub-expression, the
gain from compilation could be offset by doing work twice.
(IMPALA-7737)

Still, the fix provides most of what the JIRA ticket requested
within the limitations of the existing code.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Most functions use the simplest possible rewrite, relying on the
existing rewrite rules for further simplification.  The one exception
is coalesce(): the existing code relies on the semantics of the
function and so was retained and slightly improved. The code was
extended to produce a CASE statement directly, retaining existing
simplifications.

Tests for conditional functions were in one large function along with
other rewrite tests. Moved them into a new file, then broke up the
tests by function to allow much easier debugging of each function
one-by-one.  This required moving the common test mechanims into a
new common base class.

Existing tests focus on one or two rules at a time. The conditional
function rewrite, however, relies on the entire set of rules being
applied repeatedly. So, added a new FullRewriteTest case to verify this
behavior. This class contains several commented-out tests that cannot
pass due to existing rewrite bugs noted above.

Changing the rewrite cause the PlannerTest to produce different plans
than previously. Changed the expected results file to match the new
rewrite rules.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
16 files changed, 836 insertions(+), 309 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/11760/6
--
To view, visit http://gerrit.cloudera.org:8080/11760
To unsubscribe, visit http://gerrit.clou

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, 
Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The rewrite engine has many bugs that are beyond the scope of
this change to fix. This change codes around those bugs. The
result is that the conditional rewrite happens some of the time,
and sometimes produces less-than perfect optimizations.

Conditionals in the top-level ORDER BY clause are not rewritten
(IMPALA-7753), but those one or more levels down are. Some expressions
involving NULL are not simplified (IMPALA-7769). Several hacks were
used to work around the fact that the rewrite engine ignores unanalyzed
expressions, yet the rewrite engine does not, in general, re-analyze
the expressions it produces, causing simplifications to be skipped
(IMPALA-7754). And so on.

As a result, the BE retains the original interpreted forms that
are still used in two cases: 1) top-leel conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Further, code generation does not occur for CASE statements in the
SELECT clause when it is in the root fragment (the most common case
in simple tests.) This is another known bug (IMPALA-4356).

One possible performance regression is that the new form of the code
evaluates some expressions twice, where the original interpreted
code evaluated the argument once. E.g. coalesce(id, 10) is rewritten
to CASE WHEN id IS NULL THEN id ELSE 10 END. Here, id is evaluated
twice. If the "id" were replaced by a complex sub-expression, the
gain from compilation could be offset by doing work twice.
(IMPALA-7737)

Still, the fix provides most of what the JIRA ticket requested
within the limitations of the existing code.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Most functions use the simplest possible rewrite, relying on the
existing rewrite rules for further simplification.  The one exception
is coalesce(): the existing code relies on the semantics of the
function and so was retained and slightly improved. The code was
extended to produce a CASE statement directly, retaining existing
simplifications.

Tests for conditional functions were in one large function along with
other rewrite tests. Moved them into a new file, then broke up the
tests by function to allow much easier debugging of each function
one-by-one.  This required moving the common test mechanims into a
new common base class.

Existing tests focus on one or two rules at a time. The conditional
function rewrite, however, relies on the entire set of rules being
applied repeatedly. So, added a new FullRewriteTest case to verify this
behavior. This class contains several commented-out tests that cannot
pass due to existing rewrite bugs noted above.

Changing the rewrite cause the PlannerTest to produce different plans
than previously. Changed the expected results file to match the new
rewrite rules.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
16 files changed, 837 insertions(+), 309 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/11760/7
--
To view, visit http://gerrit.cloudera.org:8080/11760
To unsubscribe, visit http://gerrit.clou

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11822


Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The rewrite engine has many bugs that are beyond the scope of
this change to fix. This change codes around those bugs. The
result is that the conditional rewrite happens some of the time,
and sometimes produces less-than perfect optimizations.

Conditionals in the top-level ORDER BY clause are not rewritten
(IMPALA-7753), but those one or more levels down are. Some expressions
involving NULL are not simplified (IMPALA-7769). Several hacks were
used to work around the fact that the rewrite engine ignores unanalyzed
expressions, yet the rewrite engine does not, in general, re-analyze
the expressions it produces, causing simplifications to be skipped
(IMPALA-7754). And so on.

As a result, the BE retains the original interpreted forms that
are still used in two cases: 1) top-level conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Further, code generation does not occur for CASE statements in the
SELECT clause when it is in the root fragment (the most common case
in simple tests.) This is another known bug (IMPALA-4356).

One possible performance regression is that the new form of the code
evaluates some expressions twice, where the original interpreted
code evaluated the argument once. E.g. coalesce(id, 10) is rewritten
to CASE WHEN id IS NULL THEN id ELSE 10 END. Here, id is evaluated
twice. If the "id" were replaced by a complex sub-expression, the
gain from compilation could be offset by doing work twice.
(IMPALA-7737)

Still, the fix provides most of what the JIRA ticket requested
within the limitations of the existing code.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Most functions use the simplest possible rewrite, relying on the
existing rewrite rules for further simplification.  The one exception
is coalesce(): the existing code relies on the semantics of the
function and so was retained and slightly improved. The code was
extended to produce a CASE statement directly, retaining existing
simplifications.

Tests for conditional functions were in one large function along with
other rewrite tests. Moved them into a new file, then broke up the
tests by function to allow much easier debugging of each function
one-by-one.  This required moving the common test mechanims into a
new common base class.

Existing tests focus on one or two rules at a time. The conditional
function rewrite, however, relies on the entire set of rules being
applied repeatedly. So, added a new FullRewriteTest case to verify this
behavior. This class contains several commented-out tests that cannot
pass due to existing rewrite bugs noted above.

Changing the rewrite cause the PlannerTest to produce different plans
than previously. Changed the expected results file to match the new
rewrite rules.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163

Fixes

Change-Id: Idfe275032dc13aee64bfd60d448138b5b77ab96a
---
M be/src/exprs/conditional-functions.h
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
17 files changed, 870 insertions(+), 313 deletions(-)



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

Gerrit-Project: Impala-ASF
G

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 7:

(14 comments)

Thanks, Phil, for the code review. Addressed all but one comment.

http://gerrit.cloudera.org:8080/#/c/11760/7//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11760/7//COMMIT_MSG@29
PS7, Line 29: As a result, the BE retains the original interpreted forms that
> Let's mention this in the backend code in comments, so that folks know it's
Done


http://gerrit.cloudera.org:8080/#/c/11760/7//COMMIT_MSG@30
PS7, Line 30: are still used in two cases: 1) top-leel conditions in the
> nit: level
Done


http://gerrit.cloudera.org:8080/#/c/11760/7//COMMIT_MSG@45
PS7, Line 45: Still, the fix provides most of what the JIRA ticket requested
> Does the specific "compute stats" query that's underlying the JIRA get fast
Haven't gotten that far yet; still trying to make sure that things actually 
work. Will provide an update once I can do the required performance testing.


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
File fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java:

http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@54
PS7, Line 54:
> nit: it's weird that there are two spaces there.
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@66
PS7, Line 66: // then want to allow CASE to do any simplification 
(reverting to
> nit: The parenthetical here isn't closed.
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@70
PS7, Line 70:   Expr revised =  rewriteConditionalFn((FunctionCallExpr) 
expr);
> nit: should only have one space after =
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@72
PS7, Line 72:   // Workaround for IMALA-TBD
> Is TBD resolved at this point?
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@132
PS7, Line 132:* IFNULL(a, x) --> 
> If we're not using HTML, is the  here noise?
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@179
PS7, Line 179:   if (childExpr.isAggregate()) { sawAgg = true; }
> The javadoc for "expr.contains()" suggests that hasAgg has already checked
True.

This check adjusts the non-null literal case. Ignoring aggregates, we can stop 
adding arguments (ignoring all remaining arguments) if we hit a non-null 
literal. (This is the ! hasAgg case.)

Or, if we have aggregates, we can stop if we've already added at least one 
aggregate. Otherwise, we have to keep going to add at least one aggregate -- 
even though the aggregate will never actually be evaluated. (This is the sawAgg 
case.)

But, that I had to explain this means that using dual flags is the wrong 
approach. Rewrote to use a state machine. Please let me know if that is clearer.


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

http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java@50
PS7, Line 50:  * We can't eliminate aggregates as this may change the meaning 
of the
> This is the same as line 70-72. Is that intentional?
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
File fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java:

http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@42
PS7, Line 42: System.out.println(stmt);
> remove
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@97
PS7, Line 97:   @Test
:   public void testSqlRecovery() {
: ParseNode node = analyzeExpr("true and false");
: System.out.println(node.toSql());
: System.out.println(((SelectStmt) node).toSql(true));
: // SELECT TRUE AND FALSE FROM functional.alltypessmall
:   }
> There's no assertion here...
Ad-hoc test. Removed.


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java@117
PS7, Line 117: // TODO: IMPALA-7766
> nit: including the subject of the JIRA here may make it obvious why...
Done


http://gerrit.cloudera.org:8080/#/c/11760/7/fe/src/test/java/org/apache/impala/analysis/F

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/11822 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Abandoned

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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Idfe275032dc13aee64bfd60d448138b5b77ab96a
Gerrit-Change-Number: 11822
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, 
Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The rewrite engine has many bugs that are beyond the scope of
this change to fix. This change codes around those bugs. The
result is that the conditional rewrite happens some of the time,
and sometimes produces less-than perfect optimizations.

Conditionals in the top-level ORDER BY clause are not rewritten
(IMPALA-7753), but those one or more levels down are. Some expressions
involving NULL are not simplified (IMPALA-7769). Several hacks were
used to work around the fact that the rewrite engine ignores unanalyzed
expressions, yet the rewrite engine does not, in general, re-analyze
the expressions it produces, causing simplifications to be skipped
(IMPALA-7754). And so on.

As a result, the BE retains the original interpreted forms that
are still used in two cases: 1) top-level conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Further, code generation does not occur for CASE statements in the
SELECT clause when it is in the root fragment (the most common case
in simple tests.) This is another known bug (IMPALA-4356).

One possible performance regression is that the new form of the code
evaluates some expressions twice, where the original interpreted
code evaluated the argument once. E.g. coalesce(id, 10) is rewritten
to CASE WHEN id IS NULL THEN id ELSE 10 END. Here, id is evaluated
twice. If the "id" were replaced by a complex sub-expression, the
gain from compilation could be offset by doing work twice.
(IMPALA-7737)

Still, the fix provides most of what the JIRA ticket requested
within the limitations of the existing code.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Most functions use the simplest possible rewrite, relying on the
existing rewrite rules for further simplification.  The one exception
is coalesce(): the existing code relies on the semantics of the
function and so was retained and slightly improved. The code was
extended to produce a CASE statement directly, retaining existing
simplifications.

Tests for conditional functions were in one large function along with
other rewrite tests. Moved them into a new file, then broke up the
tests by function to allow much easier debugging of each function
one-by-one.  This required moving the common test mechanims into a
new common base class.

Existing tests focus on one or two rules at a time. The conditional
function rewrite, however, relies on the entire set of rules being
applied repeatedly. So, added a new FullRewriteTest case to verify this
behavior. This class contains several commented-out tests that cannot
pass due to existing rewrite bugs noted above.

Changing the rewrite cause the PlannerTest to produce different plans
than previously. Changed the expected results file to match the new
rewrite rules.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M be/src/exprs/conditional-functions.h
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
17 files changed, 870 insertions(+), 313 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/11760/8
--
To view, visit http://gerrit.cloudera.org:8080/11760

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, 
Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The rewrite engine has many bugs that are beyond the scope of
this change to fix. This change codes around those bugs. The
result is that the conditional rewrite happens some of the time,
and sometimes produces less-than perfect optimizations.

Conditionals in the top-level ORDER BY clause are not rewritten
(IMPALA-7753), but those one or more levels down are. Some expressions
involving NULL are not simplified (IMPALA-7769). Several hacks were
used to work around the fact that the rewrite engine ignores unanalyzed
expressions, yet the rewrite engine does not, in general, re-analyze
the expressions it produces, causing simplifications to be skipped
(IMPALA-7754). And so on.

As a result, the BE retains the original interpreted forms that
are still used in two cases: 1) top-level conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Further, code generation does not occur for CASE statements in the
SELECT clause when it is in the root fragment (the most common case
in simple tests.) This is another known bug (IMPALA-4356).

One possible performance regression is that the new form of the code
evaluates some expressions twice, where the original interpreted
code evaluated the argument once. E.g. coalesce(id, 10) is rewritten
to CASE WHEN id IS NULL THEN id ELSE 10 END. Here, id is evaluated
twice. If the "id" were replaced by a complex sub-expression, the
gain from compilation could be offset by doing work twice.
(IMPALA-7737)

Still, the fix provides most of what the JIRA ticket requested
within the limitations of the existing code.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Most functions use the simplest possible rewrite, relying on the
existing rewrite rules for further simplification.  The one exception
is coalesce(): the existing code relies on the semantics of the
function and so was retained and slightly improved. The code was
extended to produce a CASE statement directly, retaining existing
simplifications.

Tests for conditional functions were in one large function along with
other rewrite tests. Moved them into a new file, then broke up the
tests by function to allow much easier debugging of each function
one-by-one.  This required moving the common test mechanims into a
new common base class.

Existing tests focus on one or two rules at a time. The conditional
function rewrite, however, relies on the entire set of rules being
applied repeatedly. So, added a new FullRewriteTest case to verify this
behavior. This class contains several commented-out tests that cannot
pass due to existing rewrite bugs noted above.

Changing the rewrite cause the PlannerTest to produce different plans
than previously. Changed the expected results file to match the new
rewrite rules.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M be/src/exprs/conditional-functions.h
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
18 files changed, 946 insertions(+), 313 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60/11

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, 
Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..

IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the above functions into the equivalent CASE
structure.

The rewrite engine has many bugs that are beyond the scope of
this change to fix. This change codes around those bugs. The
result is that the conditional rewrite happens some of the time,
and sometimes produces less-than perfect optimizations.

Conditionals in the top-level ORDER BY clause are not rewritten
(IMPALA-7753), but those one or more levels down are. Some expressions
involving NULL are not simplified (IMPALA-7769). Several hacks were
used to work around the fact that the rewrite engine ignores unanalyzed
expressions, yet the rewrite engine does not, in general, re-analyze
the expressions it produces, causing simplifications to be skipped
(IMPALA-7754). And so on.

As a result, the BE retains the original interpreted forms that
are still used in two cases: 1) top-level conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Further, code generation does not occur for CASE statements in the
SELECT clause when it is in the root fragment (the most common case
in simple tests.) This is another known bug (IMPALA-4356).

One possible performance regression is that the new form of the code
evaluates some expressions twice, where the original interpreted
code evaluated the argument once. E.g. coalesce(id, 10) is rewritten
to CASE WHEN id IS NULL THEN id ELSE 10 END. Here, id is evaluated
twice. If the "id" were replaced by a complex sub-expression, the
gain from compilation could be offset by doing work twice.
(IMPALA-7737)

Still, the fix provides most of what the JIRA ticket requested
within the limitations of the existing code.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Most functions use the simplest possible rewrite, relying on the
existing rewrite rules for further simplification.  The one exception
is coalesce(): the existing code relies on the semantics of the
function and so was retained and slightly improved. The code was
extended to produce a CASE statement directly, retaining existing
simplifications.

Tests for conditional functions were in one large function along with
other rewrite tests. Moved them into a new file, then broke up the
tests by function to allow much easier debugging of each function
one-by-one.  This required moving the common test mechanims into a
new common base class.

Existing tests focus on one or two rules at a time. The conditional
function rewrite, however, relies on the entire set of rules being
applied repeatedly. So, added a new FullRewriteTest case to verify this
behavior. This class contains several commented-out tests that cannot
pass due to existing rewrite bugs noted above.

Changing the rewrite cause the PlannerTest to produce different plans
than previously. Changed the expected results file to match the new
rewrite rules.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M be/src/exprs/conditional-functions.h
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/aggregation.test
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M testdata/workloads/functional-planner/queries/PlannerTest/implicit-joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/nested-collections.test
M testdata/workloads/functional-planner/queries/PlannerTest/outer-joins.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
18 files changed, 1,014 insertions(+), 282 deletions(-)


  git pull ssh://gerrit.cloudera.org:29418/Impala-ASF refs/changes/60

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 7:

Turns out that there is a BE bug that means CASE is not equivalent to 
coalesce(). Disabled all coalesce() rewrites and tests, restoring original 
behavior from master. Coalesce() can be revisited when IMPALA-7793 is fixed.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 7
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 31 Oct 2018 02:24:15 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull, coalesce to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull, coalesce to use CASE
..


Patch Set 10:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG
Commit Message:

http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG@43
PS8, Line 43: (IMPALA-7737)
> are there examples of these fns in our benchmarks to quantify the regressio
Done


http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG@45
PS8, Line 45: Still, the fix provides most of what the JIRA ticket requested
> I'd skip these next three paragraphs.
Done


http://gerrit.cloudera.org:8080/#/c/11760/8//COMMIT_MSG@57
PS8, Line 57:
> pls make a section for this called "Testing" so its easier to jump to. also
Done


http://gerrit.cloudera.org:8080/#/c/11760/8/be/src/exprs/conditional-functions.h
File be/src/exprs/conditional-functions.h:

http://gerrit.cloudera.org:8080/#/c/11760/8/be/src/exprs/conditional-functions.h@76
PS8, Line 76: since
: /// various bugs mean that this implementation is still sometimes 
used. But
: /// the goal is to remove these classes at some point.
> simpler: "until their use is eliminated by the frontend".
Done


http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
File fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java:

http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@37
PS8, Line 37: vanish
> is this accurate given the comments in the commit message about order by?
Yes, they vanish after the rewrite. If the rewrite does not occur (due to bugs 
or options), then the functions do not vanish.


http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@48
PS8, Line 48: planner runs the rule to simplify CASE
:  * after this rule. Where that other rule can perform 
simplifications,
:  * those simplifications are omitted here
> simplify and use the specific rule name for concreteness.
Rewrote. Since many rules apply, did not seem to add value to try to enumerate 
them.


http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@106
PS8, Line 106:return rewriteCoalesceFn(e
> clarify whether you think this happens after the rewrite or before. If its
Done


http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@130
PS8, Line 130:Lists.ne
> isn't this all that's done here (the most general case) and we'll depend on
Rewrote comment


http://gerrit.cloudera.org:8080/#/c/11760/8/fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java@172
PS8, Line 172: ESCE(a, literal) when literal
> The simplest rewrite here would be to not look at the child exprs for the v
I found that I could strip this function down as you suggest, but the results 
were sub-optimal due to weaknesses in other parts of the code.

coalesce(id) --> CASE ELSE id END (expected: id)
coalesce(id, null) --> CASE WHEN id IS NOT NULL THEN ID ELSE NULL END 
(expected: id)
coalesce(null, id) --> same as above
coalesce(null is null, id) --> 1 (expected: TRUE)
coalesce(10 + null, id) --> CASE WHEN NULL IS NOT NULL THEN NULL ELSE id END 
(expected: id)
coalesce(42, min(distinct tinyint_col)) --> CASE WHEN FALSE THEN NULL WHEN TRUE 
THEN 42 ELSE min(distinct tinyint_col, 42)) END (Expected: CASE WHEN 
min(distinct tinyint_col) IS NOT NULL THEN min(distinct tinyint_col) ELSE 42 
END)

And so on.

One solution is to restore the original coalesce() function rewrite which 
produces somewhat better results. Even that function has holes which this 
version fixed. So, we'd lose those optimizations.

As it turns out, all this is moot because the BE CASE implementation has a bug 
so we can't even use CASE to replace coalesce()...

For now, I did rework the function to the simple form requested, then disabled 
it. We can tackle any other concerns if/when it is a priority.


http://gerrit.cloudera.org:8080/#/c/11760/8/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
File 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test:

http://gerrit.cloudera.org:8080/#/c/11760/8/testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test@1137
PS8, Line 1137: |  other predicates: functional.alltypestiny.tinyint_col + 
functional.alltypestiny.smallint_col + functional.alltypestiny.int_col > 10, 
CASE WHEN functional.alltypestiny.tinyint_col + 
functional.alltypestiny.bigint_col IS NULL THEN 1 ELSE 
functional.alltypestiny.tinyint_col + functional.alltypestiny.bigint_col END = 1
> so this is the example of the performance regression (same w

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Philip Zeyliger, Tim Armstrong, Csaba Ringhofer, 
Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7655: Rewrite if, isnull to use CASE
..

IMPALA-7655: Rewrite if, isnull to use CASE

See IMPALA-7655 for backgound. Tim found that the current interpreted
forms of if, isnull and coalesce are slow compared to the
code-generated CASE statement.

This patch rewrites the if and isnull functions into the equivalent
CASE structure. Coalesce is omitted due to limitations within the
code.

Caveats:

* IMPALA-7753: Conditionals in the top-level ORDER BY clause are not
rewritten, but those one or more levels down are. The result is that
PlannerTest files still show uses of if and isnull despite this fix.
* IMPALA-7785: Similar, but says that the GROUP BY clause is not
analyzed prior to rewrites, so the code again forces analysis.
There are some odd "analyze()" calls in the code to work around this
limitation snd the next two.
* IMPALA-7769: Some expressions involving NULL are not simplified.
As a result, some of the rewrites don't result in the most optimal
result.
* IMPALA-7754: The rewrite engine ignores unanalyzed expressions, yet
the rewrite engine does not, in general, re-analyze the expressions it
produces, causing simplifications to be skipped.
* IMPALA-4356: Even after this change, code gen won't occur for SELECT
expressions executed in teh root fragment (the most common case
in simple tests.)

Because of the above, some if and isnull calls are not rewritten, and
others are not rewritten to the simplest forms. The tests contain
commented-out tests, and comments with bug numbers, to record these
known issues.  Still, the current fix is useful enough by itself to
proceed despite the above.

While the desire was to replace the BE implementation of if and isnull,
the caveats prevent doing so at present, so the BE retains the original
interpreted forms. They are used when: 1) top-level conditions in the
ORDER BY clause, and 2) if the user disables rewrites.

Conditional function rewrites are moved into a new class,
RewriteConditionalsRule in order to keep things simple.

Coalesce() is implemented, but disabled. The old rewrites are retained
until blocking issues are fixed.

Testing:

* Tests for conditional functions are pulled out into a new test class.
* A new base class holds code common to the new class and the existing
ExprRewriterTest.
* A new FullRewriteTest class tests rewrites that require multiple
passes thorugh teh rewrite engine.
* Planner Test expected results were updated based on the rewritten
expressions.

Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
---
M be/src/exprs/conditional-functions.h
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
A fe/src/main/java/org/apache/impala/rewrite/RewriteConditionalFnsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeExprsTest.java
A fe/src/test/java/org/apache/impala/analysis/BaseRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/FullRewriteTest.java
A fe/src/test/java/org/apache/impala/analysis/RewriteConditionalFnsRuleTest.java
M testdata/workloads/functional-planner/queries/PlannerTest/analytic-fns.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
M 
testdata/workloads/functional-planner/queries/PlannerTest/runtime-filter-propagation.test
13 files changed, 939 insertions(+), 256 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 11
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7501: Slim down metastore Partition objects in LocalCatalog

[Paul Rogers (Code Review)]

Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/11688 )

Change subject: IMPALA-7501: Slim down metastore Partition objects in 
LocalCatalog
..


Abandoned

Change has become too involved for a simple ramp-up task; really requires the 
deep understanding of the metadata team. Please incorporate the specific 
changes if/where they make sense.
--
To view, visit http://gerrit.cloudera.org:8080/11688
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I4c861452cf974970ab511406fe2e175ea3dc668d
Gerrit-Change-Number: 11688
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Todd Lipcon 

[Impala-ASF-CR] IMPALA-7805: Emit zero as "0" in toSql()

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11878


Change subject: IMPALA-7805: Emit zero as "0" in toSql()
..

IMPALA-7805: Emit zero as "0" in toSql()

It turns out that Impala has a somewhat Baroque way to represent the
value of a numeric 0.  NumericLiteral.toSql() uses the Java
BigDecimal class to convert a numeric value to a string for use in
explained plans and in verifying expression rewrites.

The default Java behavior is to consider scale when rendering numbers,
including 0. Thus, depending on precision and scale, you may get:

0
0.0
0.00
0.000
...
0E-38

However, mathematically, zero is zero. Plans attach no special meaning
to the extra decimal points or trailing zeros.

To make testing easier, changed the behavior to always emit "0" when the
value is zero, regardless of precision or scale.

Testing: Reran the planner tests and modified captured plans that had
the 0.0, 0.00 variations of zero.

Since this change affects only EXPLAIN output, it cannot affect the
operation of queries. If may impact other tests that compare EXPLAIN
output to a "golden" copy.

Change-Id: I0b2f2f34fe5e6003de407301310ccf433841b9f1
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
6 files changed, 28 insertions(+), 21 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I0b2f2f34fe5e6003de407301310ccf433841b9f1
Gerrit-Change-Number: 11878
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7807: Analysis test fixture

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11881


Change subject: IMPALA-7807: Analysis test fixture
..

IMPALA-7807: Analysis test fixture

Refactors the existing ExprRewriteRulesTest to pull out the framework
functionality into a "test fixture" class which can be used for a
variety of tests. The fixture allows more variation, and access to
more intermediate state, than does the existing function-based
framework. For example, the fixture allows setting per-query
options, allows doing full or partial (no rewrite) analysis, provides
access to things like the Analyzer, and more.

The fixture pulls in code from FrontEndBase to handle the low-level
tasks of running a query, and from ExprRewriteRulesTest for the
rewrite-specific aspects. For now, the FrontEndBase was left unchanged.
ExprRewriteRulesTest is refactored to use the new fixture in order to
illustrate how it can be used.

The key value of this work is to allow greater detail in testing in
future change requests. No new test cases were added here in order to
keep the refactoring as simple and clean as possible.

Testing: since this is a test fixture, the refactored ExprRewriteRulesTest
implicitly tests the fixture code. No "production" (non-test) code was
changed in this patch.

Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
---
A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/testutil/TestUtils.java
3 files changed, 478 insertions(+), 89 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11883


Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
..

IMPALA-7808: Refactor Analyzer for easier debugging

Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.

A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.

To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.

This CR introduces the inner algorithm class, but leaves the original
code at the original indent level to make reviews easier. You should see
the same code, now broken into smaller functions, with a new "driver"
section that calls these new functions from where the code itself
previously resided.

Once a review is done of the code in this form, I can revise this CR
with the only change being to indent the methods one level, which can be
reviewed as a second step.

One block of code had to be moved below the inner class since it is not
part of the analysis process.

Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Testing: Reran all tests, which passed.
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 227 insertions(+), 149 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging

[Paul Rogers (Code Review)]

Hello Csaba Ringhofer, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
..

IMPALA-7808: Refactor Analyzer for easier debugging

Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.

A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.

To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.

This CR introduces the inner algorithm class, but leaves the original
code at the original indent level to make reviews easier. You should see
the same code, now broken into smaller functions, with a new "driver"
section that calls these new functions from where the code itself
previously resided.

Once a review is done of the code in this form, I can revise this CR
with the only change being to indent the methods one level, which can be
reviewed as a second step.

One block of code had to be moved below the inner class since it is not
part of the analysis process.

Testing: No functional change, changes are purely structure.
Reran all tests, which passed.

Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 625 insertions(+), 558 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11883 )

Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
..


Patch Set 2:

(12 comments)

Thanks for the reviews. Addressed the comments.

If you are ready, I'll go ahead and shift the indent of the methods in the 
inner class, which will mark a zillion likes as changed. In its current form, 
you can more easily see the actual changes.

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

http://gerrit.cloudera.org:8080/#/c/11883/1//COMMIT_MSG@37
PS1, Line 37: Reran a
> pls move this testing section above the Change-Id
Done


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

http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461
PS1, Line 461: r
> nit: extra space
Done


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java@461
PS1, Line 461: eturn;
> We generally do not use braces in one line "if (cond) stmt;".
Thanks. Every project has its quirks; I 'm still learning Impala's preferences.
Fixed.


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

http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@176
PS1, Line 176: private final Analyzer analyzer_;
> these members should be suffixed with "_"
Done


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@186
PS1, Line 186: Note tem
> note, you can always include such review-level comments as comments in gerr
Agreed. The comment is here to rather force the need to come back and shift 
indentation before the final +2. At that point, the comment will be removed.


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@219
PS1, Line 219: if (multiAggInfo_ != null && 
multiAggInfo_.hasAggregateExprs()) {
> here and at many other functions: in Impala we generally do not add an empt
Done


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@389
PS1, Line 389: if (t
> did some other class need this protected?
Tried to minimize extra changes. It works to make this private, so went ahead 
and did so.


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@526
PS1, Line 526: c
> nit: extra space
Done


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@617
PS1, Line 617: su
> why public?
Done


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@659
PS1, Line 659:
> why public?
Done


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@757
PS1, Line 757: }
> why public?
Done


http://gerrit.cloudera.org:8080/#/c/11883/1/fe/src/main/java/org/apache/impala/analysis/SelectStmt.java@894
PS1, Line 894:
> just to remind myelf... end of inner class.
Yes. When you are satisfied that nothing changed other than the inner class, 
I'll go ahead and indent the whole mess.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 06 Nov 2018 20:44:33 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11890


Change subject: IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging
..

IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging

Impala provides a single test class ExprRewriteRulesTest for all rewrite
rules. Each rule class has an associated test function. Th
SimplifyConditionalRule class is complex, and the result test function
is quite large.

When doing work to modify a particular conditional rewrite, it became
clear that debugging would be much easier if each detailed rewrite had
its own test rather than using one huge test function. This ticket asks
to break up the big function. (JUnit does not care about small vs. large
functions.)

This patch splits the function separate from later work that will modify
the tests themselves.

Also unified test function naming, using the standard "javaStyle" rather
than the unusual "CppStyle" that was used.

Added a few new tests, including some that do not pass (commented out)
to motivate the need for the refactoring.

Testing: this is a test, ran the test to ensure it still passes.

Change-Id: I99a28b170bc0132424041e51f61ebe5c848c4083
---
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
1 file changed, 112 insertions(+), 24 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I99a28b170bc0132424041e51f61ebe5c848c4083
Gerrit-Change-Number: 11890
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging

[Paul Rogers (Code Review)]

Hello Csaba Ringhofer, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
..

IMPALA-7808: Refactor Analyzer for easier debugging

Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.

A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.

To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.

For the most part, the existign code was simply split into functions
and indented.  One block of code had to be moved below the inner class
since it is not part of the analysis process.

Testing: No functional change, changes are purely structure.
Reran all tests, which passed.

Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 751 insertions(+), 688 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging

[Paul Rogers (Code Review)]

Hello Csaba Ringhofer, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
..

IMPALA-7808: Refactor Analyzer for easier debugging

Changes two blocks of code to make debugging easier. No functional
changes occur; changes are pure refactoring. A trivial change in
AnalyzerContext removes a nested conditional clause.

A larger change in SelectStmt takes the large analysis function and
breaks it into a series of smaller functions. The functions were large
because they shared state: variables created near the top are used much
later near the bottom.

To solve this, moved the code into an "algorithm" class whose only job
is to hold onto the temporary state so that the big function can be
broken into smaller pieces, with the temporary class fields used in
place of the former local variables.

For the most part, the existign code was simply split into functions
and indented.  One block of code had to be moved below the inner class
since it is not part of the analysis process.

Testing: No functional change, changes are purely structure.
Reran all tests, which passed.

Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
---
M fe/src/main/java/org/apache/impala/analysis/AnalysisContext.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
2 files changed, 767 insertions(+), 688 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7808: Refactor Analyzer for easier debugging

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11883 )

Change subject: IMPALA-7808: Refactor Analyzer for easier debugging
..


Patch Set 5:

Added indentation, fixed lines that became too long.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I576c80c4c7a974df226fc91d8903db275069ed52
Gerrit-Change-Number: 11883
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Tue, 06 Nov 2018 23:10:56 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11893


Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..

IMPALA-7823: Clean up Java warnings, fix minor issues

Roll-up of trivial Java fixes: for warnings, formatting, toString()
methods. Done as a separate patch to keep others small and focused.

Testing: no functional changes. Ran full tests to check for
regressions.

Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
M fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java
M fe/src/main/java/org/apache/impala/analysis/ParquetHelper.java
M fe/src/main/java/org/apache/impala/analysis/PlanHint.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationException.java
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M 
fe/src/main/java/org/apache/impala/catalog/HdfsPartitionLocationCompressor.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DataPartition.java
M fe/src/main/java/org/apache/impala/planner/JoinNode.java
M fe/src/main/java/org/apache/impala/planner/PlanFragment.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/ValueRange.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
61 files changed, 57 insertions(+), 138 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4b174d64

[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11887


Change subject: IMPALA-7818: Standardize use of Expr predicates
..

IMPALA-7818: Standardize use of Expr predicates

The Expr node provids two kinds of queries about node categories:
predicates and isMuble() functions. This change standardizes two
existing methods to use predicates instead.

First, the existing isLiteral() method is replaced by a new
IS_LITERAL predicate.

The key purpose is to clean up the confusing use of the existing
isNullLiteral() method, which actually is a check for either a null
literal or a cast of a null. To make this clearer, replaced this
method with a new IS_NULL_VALUE() predicate.

Added a new IS_NULL_LITERAL predicate for the case when a node must be
exactly the NULL literal, not a cast to NULL. Replaced instances of
foo instanceof NullLiteral with a use of the new IS_NULL_LITERAL
predicate. (This revealed bugs which will be addressed separately.)

Added an IS_NON_NULL_LITERAL to replace the two-method ideom used in
several places.

Tests: No functional change. Reran all tests to ensure nothing changed.

Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
M fe/src/main/java/org/apache/impala/analysis/PartitionKeyValue.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
29 files changed, 135 insertions(+), 93 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
Gerrit-Change-Number: 11887
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11887 )

Change subject: IMPALA-7818: Standardize use of Expr predicates
..


Patch Set 3:

Dry-run passed: https://jenkins.impala.io/job/pre-review-test/221/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
Gerrit-Change-Number: 11887
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Wed, 07 Nov 2018 17:35:47 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11893 )

Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..


Patch Set 1:

Dry-run tests passed: https://jenkins.impala.io/job/pre-review-test/220/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e
Gerrit-Change-Number: 11893
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Wed, 07 Nov 2018 17:36:59 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates

[Paul Rogers (Code Review)]

Hello Tim Armstrong, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7818: Standardize use of Expr predicates
..

IMPALA-7818: Standardize use of Expr predicates

The Expr node provids two kinds of queries about node categories:
predicates and isMuble() functions. This change standardizes two
existing methods to use predicates instead.

First, the existing isLiteral() method is replaced by a new
IS_LITERAL predicate.

The key purpose is to clean up the confusing use of the existing
isNullLiteral() method, which actually is a check for either a null
literal or a cast of a null. To make this clearer, replaced this
method with a new IS_NULL_VALUE() predicate.

Added a new IS_NULL_LITERAL predicate for the case when a node must be
exactly the NULL literal, not a cast to NULL. Replaced instances of
foo instanceof NullLiteral with a use of the new IS_NULL_LITERAL
predicate. (This revealed bugs which will be addressed separately.)

Added an IS_NON_NULL_LITERAL to replace the two-method ideom used in
several places.

Tests: No functional change. Reran all tests to ensure nothing changed.

Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
M fe/src/main/java/org/apache/impala/analysis/PartitionKeyValue.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
29 files changed, 137 insertions(+), 93 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
Gerrit-Change-Number: 11887
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Tim Armstrong 

[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11887 )

Change subject: IMPALA-7818: Standardize use of Expr predicates
..


Patch Set 4:

(2 comments)

Fixed check style warnings.

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

http://gerrit.cloudera.org:8080/#/c/11887/3/fe/src/main/java/org/apache/impala/analysis/Expr.java@700
PS3, Line 700:   Preconditions.checkState(Expr.IS_NULL_LITERAL.apply(this) 
||
> line too long (92 > 90)
Done


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

http://gerrit.cloudera.org:8080/#/c/11887/3/fe/src/main/java/org/apache/impala/analysis/PartitionKeyValue.java@87
PS3, Line 87: if (Expr.IS_NULL_LITERAL.apply(literalValue) ||
> line too long (94 > 90)
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
Gerrit-Change-Number: 11887
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Wed, 07 Nov 2018 19:02:22 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11893 )

Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..


Patch Set 1:

(3 comments)

Thanks Bharath for the review. Addressed comments.

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

http://gerrit.cloudera.org:8080/#/c/11893/1/fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java@86
PS1, Line 86: new LinkedHashMap();
> (multiple places) We can also use Maps.newLinkedHashMap() for brevity or pr
Not sure why this didn't work the first time. Now, Eclipse does allow the 
diamond form, so used that.

The Guava methods were added prior to the diamond syntax; Google suggests using 
the more modern syntax instead.


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

http://gerrit.cloudera.org:8080/#/c/11893/1/fe/src/main/java/org/apache/impala/catalog/AuthorizationException.java@24
PS1, Line 24: @SuppressWarnings("serial")
> Apply this in the CatalogException class too and get rid of  serialVersionU
Done


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

http://gerrit.cloudera.org:8080/#/c/11893/1/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@96
PS1, Line 96:   @SuppressWarnings("unused")
> Why can't we remove them? I'm not familiar enough with RuntimeFilter code.
I'm not familiar with the code either or I would have removed them. I did 
remove a few others that clearly were not doing any good.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e
Gerrit-Change-Number: 11893
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Wed, 07 Nov 2018 21:02:50 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..

IMPALA-7823: Clean up Java warnings, fix minor issues

Roll-up of trivial Java fixes: for warnings, formatting, toString()
methods. Done as a separate patch to keep others small and focused.

Testing: no functional changes. Ran full tests to check for
regressions.

Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
M fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java
M fe/src/main/java/org/apache/impala/analysis/ParquetHelper.java
M fe/src/main/java/org/apache/impala/analysis/PlanHint.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationException.java
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogException.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M 
fe/src/main/java/org/apache/impala/catalog/HdfsPartitionLocationCompressor.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DataPartition.java
M fe/src/main/java/org/apache/impala/planner/JoinNode.java
M fe/src/main/java/org/apache/impala/planner/PlanFragment.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/ValueRange.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
62 files changed, 58 insertions(+), 140 deletions(-)


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

[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates

[Paul Rogers (Code Review)]

Hello Tim Armstrong, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7818: Standardize use of Expr predicates
..

IMPALA-7818: Standardize use of Expr predicates

The Expr node provide two kinds of queries about node categories:
predicates and isMumble() functions. This change standardizes two
existing methods to use predicates instead.

First, the existing isLiteral() method is replaced by a new
IS_LITERAL predicate.

The key purpose is to clean up the confusing use of the existing
isNullLiteral() method, which actually is a check for either a null
literal or a cast of a null. To make this clearer, replaced this
method with a new IS_NULL_VALUE() predicate.

Added a new IS_NULL_LITERAL predicate for the case when a node must be
exactly the NULL literal, not a cast to NULL. Replaced instances of
foo instanceof NullLiteral with a use of the new IS_NULL_LITERAL
predicate. (This revealed bugs which will be addressed separately.)

Added an IS_NON_NULL_LITERAL to replace the two-method idiom used in
several places.

Tests: No functional change. Reran all tests to ensure nothing changed.

Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnDef.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java
M fe/src/main/java/org/apache/impala/analysis/LikePredicate.java
M fe/src/main/java/org/apache/impala/analysis/LiteralExpr.java
M fe/src/main/java/org/apache/impala/analysis/PartitionKeyValue.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/TupleIsNullPredicate.java
M fe/src/main/java/org/apache/impala/catalog/FeCatalogUtils.java
M fe/src/main/java/org/apache/impala/catalog/FeFsTable.java
M fe/src/main/java/org/apache/impala/catalog/HdfsTable.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalFsTable.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeCountStarRule.java
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M fe/src/test/java/org/apache/impala/catalog/local/LocalCatalogTest.java
29 files changed, 138 insertions(+), 94 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
Gerrit-Change-Number: 11887
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7818: Standardize use of Expr predicates

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11887 )

Change subject: IMPALA-7818: Standardize use of Expr predicates
..


Patch Set 5:

(5 comments)

Thanks Vuk for the review. Addressed your comments.

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

http://gerrit.cloudera.org:8080/#/c/11887/4//COMMIT_MSG@10
PS4, Line 10: isMumble(
> sp?
Done


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

http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/Expr.java@1169
PS4, Line 1169: IS_NU
> drop here, and other places where its not needed.
Thanks for catching this.


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

http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/FunctionCallExpr.java@432
PS4, Line 432: IS_NU
> can drop
Done


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

http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/analysis/LikePredicate.java@132
PS4, Line 132: IS_NON_NULL_LITERAL
> double-checking if this translation preserves the previous expression? isNu
I agree, the original is confusing with "isNullLiteral()" meaning "null literal 
or cast." Let's work this through.

The first term in the original matches only literal nodes. The second term 
matched Null literal or a cast of a null. But, since the first term excluded 
the cast, we only care about the null literal check.

The new predicate allows all literals except the null literal. So, unless I'm 
missing something, they seem identical.


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

http://gerrit.cloudera.org:8080/#/c/11887/4/fe/src/main/java/org/apache/impala/rewrite/FoldConstantsRule.java@52
PS4, Line 52: IS_LITERAL.apply
> IS_LITERAL?
Yes. The change shown here is needed, but is better presented in a separate 
patch that includes new test cases.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I09a089c0f2484246e5c6444b78990fa9260c036a
Gerrit-Change-Number: 11887
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Wed, 07 Nov 2018 21:17:04 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11893 )

Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..


Patch Set 3:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/11893/1/fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java@96
PS1, Line 96:   // can be applied at the corresponding scan nodes.
> I had a chat with Bikram offline (his patch added these). He too thinks the
Thanks! Went ahead and removed them.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e
Gerrit-Change-Number: 11893
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Wed, 07 Nov 2018 21:20:05 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7528: Fix division by zero when computing cardinalities

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11901 )

Change subject: IMPALA-7528: Fix division by zero when computing cardinalities
..


Patch Set 4:

As it turns out, I tackled this at the root cause (NDV=0) in #11528, 
IMPALA-7310: Use NDV=1 for a Column with all nulls (Waiting for final review.)

Perhaps it is good to tackle the issue in both places: on the metadata side and 
by being safe when doing calculations.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieedd51d3ad6131a4ea2a5883f05104e6a0f2cd14
Gerrit-Change-Number: 11901
Gerrit-PatchSet: 4
Gerrit-Owner: Bikramjeet Vig 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Comment-Date: Thu, 08 Nov 2018 02:44:52 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11528 )

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..


Patch Set 12:

Bikramjeet, please take a look at this fix to see how we can coordinate our 
efforts.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 12
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 
Gerrit-Comment-Date: Thu, 08 Nov 2018 02:47:04 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..

IMPALA-7823: Clean up Java warnings, fix minor issues

Roll-up of trivial Java fixes: for warnings, formatting, toString()
methods. Done as a separate patch to keep others small and focused.

Testing: no functional changes. Ran full tests to check for
regressions.

Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e
---
M .gitignore
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/BetweenPredicate.java
M fe/src/main/java/org/apache/impala/analysis/BinaryPredicate.java
M fe/src/main/java/org/apache/impala/analysis/CaseExpr.java
M fe/src/main/java/org/apache/impala/analysis/ColumnLineageGraph.java
M fe/src/main/java/org/apache/impala/analysis/CompoundPredicate.java
M fe/src/main/java/org/apache/impala/analysis/ComputeStatsStmt.java
M fe/src/main/java/org/apache/impala/analysis/CreateFunctionStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateOrAlterViewStmtBase.java
M fe/src/main/java/org/apache/impala/analysis/CreateTableLikeFileStmt.java
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/ExistsPredicate.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/GrantRevokePrivStmt.java
M fe/src/main/java/org/apache/impala/analysis/KuduPartitionExpr.java
M fe/src/main/java/org/apache/impala/analysis/MultiAggregateInfo.java
M fe/src/main/java/org/apache/impala/analysis/ParquetHelper.java
M fe/src/main/java/org/apache/impala/analysis/PlanHint.java
M fe/src/main/java/org/apache/impala/analysis/PrivilegeSpec.java
M fe/src/main/java/org/apache/impala/analysis/RangePartition.java
M fe/src/main/java/org/apache/impala/analysis/ShowGrantPrincipalStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/catalog/AuthorizationException.java
M fe/src/main/java/org/apache/impala/catalog/CatalogDeltaLog.java
M fe/src/main/java/org/apache/impala/catalog/CatalogException.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectCache.java
M fe/src/main/java/org/apache/impala/catalog/CatalogObjectImpl.java
M fe/src/main/java/org/apache/impala/catalog/HdfsPartition.java
M 
fe/src/main/java/org/apache/impala/catalog/HdfsPartitionLocationCompressor.java
M fe/src/main/java/org/apache/impala/catalog/ImpaladCatalog.java
M fe/src/main/java/org/apache/impala/catalog/PartitionStatsUtil.java
M fe/src/main/java/org/apache/impala/catalog/local/CatalogdMetaProvider.java
M fe/src/main/java/org/apache/impala/catalog/local/LocalDb.java
M fe/src/main/java/org/apache/impala/planner/AggregationNode.java
M fe/src/main/java/org/apache/impala/planner/DataPartition.java
M fe/src/main/java/org/apache/impala/planner/JoinNode.java
M fe/src/main/java/org/apache/impala/planner/PlanFragment.java
M fe/src/main/java/org/apache/impala/planner/RuntimeFilterGenerator.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/ValueRange.java
M fe/src/main/java/org/apache/impala/rewrite/BetweenToCompoundRule.java
M fe/src/main/java/org/apache/impala/rewrite/EqualityDisjunctsToInRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeBinaryPredicatesRule.java
M fe/src/main/java/org/apache/impala/rewrite/NormalizeExprsRule.java
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyConditionalsRule.java
M fe/src/main/java/org/apache/impala/rewrite/SimplifyDistinctFromRule.java
M fe/src/main/java/org/apache/impala/service/CatalogOpExecutor.java
M fe/src/main/java/org/apache/impala/service/FeSupport.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/main/java/org/apache/impala/service/JniCatalog.java
M fe/src/main/java/org/apache/impala/service/JniFrontend.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeDDLTest.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationStmtTest.java
M fe/src/test/java/org/apache/impala/analysis/AuthorizationTest.java
M fe/src/test/java/org/apache/impala/analysis/ParserTest.java
M fe/src/test/java/org/apache/impala/analysis/StmtMetadataLoaderTest.java
M fe/src/test/java/org/apache/impala/planner/PlannerTest.java
62 files changed, 56 insertions(+), 144 deletions(-)


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

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Bikramjeet Vig, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..

IMPALA-7310: Use NDV=1 for a Column with all nulls

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Bug: IMPALA-7310: All-null columns give wrong estimates in planner
Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullRows/data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
9 files changed, 455 insertions(+), 21 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 13
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7805: Emit zero as "0" in toSql()

[Paul Rogers (Code Review)]

Paul Rogers has uploaded a new patch set (#2). ( 
http://gerrit.cloudera.org:8080/11878 )

Change subject: IMPALA-7805: Emit zero as "0" in toSql()
..

IMPALA-7805: Emit zero as "0" in toSql()

It turns out that Impala has a somewhat Baroque way to represent the
value of a numeric 0.  NumericLiteral.toSql() uses the Java
BigDecimal class to convert a numeric value to a string for use in
explained plans and in verifying expression rewrites.

The default Java behavior is to consider scale when rendering numbers,
including 0. Thus, depending on precision and scale, you may get:

0
0.0
0.00
0.000
...
0E-38

However, mathematically, zero is zero. Plans attach no special meaning
to the extra decimal points or trailing zeros.

To make testing easier, changed the behavior to always emit "0" when the
value is zero, regardless of precision or scale.

Testing: Reran the planner tests and modified captured plans that had
the 0.0, 0.00 variations of zero.

Since this change affects only EXPLAIN output, it cannot affect the
operation of queries. If may impact other tests that compare EXPLAIN
output to a "golden" copy.

Change-Id: I0b2f2f34fe5e6003de407301310ccf433841b9f1
---
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/kudu-selectivity.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-all.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-kudu.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-nested.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpch-views.test
6 files changed, 28 insertions(+), 21 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I0b2f2f34fe5e6003de407301310ccf433841b9f1
Gerrit-Change-Number: 11878
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-7807: Analysis test fixture

[Paul Rogers (Code Review)]

Paul Rogers has uploaded a new patch set (#3). ( 
http://gerrit.cloudera.org:8080/11881 )

Change subject: IMPALA-7807: Analysis test fixture
..

IMPALA-7807: Analysis test fixture

Refactors the existing ExprRewriteRulesTest to pull out the framework
functionality into a "test fixture" class which can be used for a
variety of tests. The fixture allows more variation, and access to
more intermediate state, than does the existing function-based
framework. For example, the fixture allows setting per-query
options, allows doing full or partial (no rewrite) analysis, provides
access to things like the Analyzer, and more.

The fixture pulls in code from FrontEndBase to handle the low-level
tasks of running a query, and from ExprRewriteRulesTest for the
rewrite-specific aspects. For now, the FrontEndBase was left unchanged.
ExprRewriteRulesTest is refactored to use the new fixture in order to
illustrate how it can be used.

The key value of this work is to allow greater detail in testing in
future change requests. No new test cases were added here in order to
keep the refactoring as simple and clean as possible.

Testing: since this is a test fixture, the refactored ExprRewriteRulesTest
implicitly tests the fixture code. No "production" (non-test) code was
changed in this patch.

Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
---
A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/testutil/TestUtils.java
3 files changed, 480 insertions(+), 89 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 3
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Bikramjeet Vig, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..

IMPALA-7310: Use NDV=1 for a Column with all nulls

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

Testing required a new table, similar to the existing nulltable,
but which has multiple rows and has stats calculated.

The change was limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Bug: IMPALA-7310: All-null columns give wrong estimates in planner
Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullRows/data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/joins.test
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
11 files changed, 460 insertions(+), 26 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 14
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Bikramjeet Vig, Impala Public Jenkins, Vuk Ercegovac,

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

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

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

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..

IMPALA-7310: Use NDV=1 for a Column with all nulls

Modified the planner to handle low-value NDVs by adjusting them
upward by one to account for null values. Thus, even an all-null
column, which has an NDV of 0 in stats, will have an NDV of 1 in
the planner. (The planner already expects NDV to include nulls.)

Modified the front end to allow capturing the full plan for use in
a unit test. Added unit tests that verify estimated cardinality
for a plan as a way to verify that the fix solved the scenario
in IMPALA-7310.

The change is limited to a very narrow range of cases:

* Base table column (not an internal column such as COUNT(*))
* Type is not BOOLEAN (turns out metadata does the needed
  NDV correction for BOOLEAN only.)
* Column has stats
* Column is nullable
* Column does not provide a null count, or null count > 0
* Reported NDV <= 1

Testing showed that, at least for the functional test tables,
we do have cases in which stats are computed, but the null
count is -1 (undefined), which is why null count had to
be considered. If we know the null count, and the null count
is zero, then no adjustment is needed, But, if we don't know
the null count, or it is positive, then adjustment may be
needed.

Research for this patch revealed that Impala treats NDVs in
two distinct ways:

* Stats (which use the NDV function) computes NDV as the number
  of distinct non-null values. (That is, the NDV of (0, null) is
  1.)
* The planner itself when working with constants, uses a definition
  of NDV that includes nulls. That is, the NDV of (0, null) is 2.

This fix attempts to bridge the two definitions, Since we know
that the NDV in stats excludes nulls (except for the BOOLEAN
type), and we know that the column contains nulls, we can bump
up the NDV to convert from the stats definition to the planner
definition. But, to avoid regressions, we do so in a very narrow
range of NDV values: only 0 and 1.

Technically, the adjustment should apply to all NDV values. However,
it turns out that if we do so, we end up with many failures in
PlannerTest in those tests that work with TPC-H tables.
The TPC-H tables have multiple columns marked as nullable but which
actually have no nulls. Some of these columns also have a low NDV.

By limiting the NDV adjustment to the narrow range, the TPC-H tests
need not be updated.  Since end users could have a similar situation,
the narrow range reduces the chance that this fix might impact such
workloads.

Testing: Reran and adjusted PlannerTest. Added a new CardinalityTest
based on an adjustment to the planning API.

Added a new table, similar to the existing nulltable,
but which has multiple rows with stats calculated.

Although the change minimized impact on PlannerTest, some
memory numbers needed adjusting for a test in which one
column hit the criteria listed above and had its NDV adjusted.

Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
---
M fe/src/main/java/org/apache/impala/analysis/SlotDescriptor.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
M fe/src/test/java/org/apache/impala/analysis/ExprNdvTest.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
A testdata/NullRows/data.csv
M testdata/bin/compute-table-stats.sh
M testdata/datasets/functional/functional_schema_template.sql
M testdata/datasets/functional/schema_constraints.csv
M testdata/workloads/functional-planner/queries/PlannerTest/tpcds-all.test
10 files changed, 458 insertions(+), 24 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 15
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7841: Refactor QueryStmt, etc. for easier debugging

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11915


Change subject: IMPALA-7841: Refactor QueryStmt, etc. for easier debugging
..

IMPALA-7841: Refactor QueryStmt, etc. for easier debugging

Builds on IMPALA-7808 with several additional refactorings:

* IMPALA-7808 minimized code changes. This change cleans up the
  new functions, removing if's and merging the "aggregation"
  function with the body of the new analyze() function.
* Creates a new "QueryAnalyzer" class in the QueryStmt class.
* Changed the SelectStmt's SelectAnalyzer to extend the new
  QueryAnalyzer. This allows removing some "analyzer"
  parameters.
* Played the same trick for other classes that extend
  QueryStmt.

This is all refactoring: there is no functional change.

Testing: Reran existing tests to ensure that functionality
remained unchanged.

Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CollectionTableRef.java
M fe/src/main/java/org/apache/impala/analysis/InlineViewRef.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/Subquery.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
8 files changed, 218 insertions(+), 202 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1
Gerrit-Change-Number: 11915
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11920


Change subject: IMPALA-7842: Make query fragments available for unit testing
..

IMPALA-7842: Make query fragments available for unit testing

Today, the FrontEnd class uses a functional model to generate a plan:
pass in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate plan
fragments produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and can pass the plan fragments back out.
There is no functional change, just a refactoring of the existing code.

The changes also introduces a new test case that uses this feature to
verify plan cardinality.

Tests: The new test case demonstrates the functionality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
2 files changed, 207 insertions(+), 15 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
..


Patch Set 1:

Dry-run tests passed: https://jenkins.impala.io/job/pre-review-test/226/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Sun, 11 Nov 2018 21:16:18 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

[Paul Rogers (Code Review)]

Hello Philip Zeyliger, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7842: Make query fragments available for unit testing
..

IMPALA-7842: Make query fragments available for unit testing

Today, the FrontEnd class uses a functional model to generate a plan:
pass in SQL text to the createExecRequest() method and get back a Thrift
plan ready for serialization.

For unit testing, however, we need access to the intermediate plan
fragments produced by the planner. Inspecting only the Thrift version
results in loss of important details.

This fix introduces a new planner context (PlanCtx) class that passes
information into the planner, and can pass the plan fragments back out.
There is no functional change, just a refactoring of the existing code.

The changes also introduces a new test case that uses this feature to
verify plan cardinality.

Tests: The new test case demonstrates the functionality.

Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
---
M fe/src/main/java/org/apache/impala/service/Frontend.java
A fe/src/test/java/org/apache/impala/planner/CardinalityTest.java
2 files changed, 219 insertions(+), 16 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 

[Impala-ASF-CR] IMPALA-7842: Make query fragments available for unit testing

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11920 )

Change subject: IMPALA-7842: Make query fragments available for unit testing
..


Patch Set 2:

(2 comments)

Thanks, Phil, for the review. Addressed your comments.

http://gerrit.cloudera.org:8080/#/c/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java
File fe/src/main/java/org/apache/impala/service/Frontend.java:

http://gerrit.cloudera.org:8080/#/c/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java@188
PS1, Line 188: protected final TQueryCtx queryCtx_;
 : protected final StringBuilder explainBuf_;
 : protected boolean capturePlan_;
 : protected List pla
> Are these intentionally public?
Made these protected, some final.


http://gerrit.cloudera.org:8080/#/c/11920/1/fe/src/main/java/org/apache/impala/service/Frontend.java@1227
PS1, Line 1227:   StringBuilder explainString)
> This should be on line 1220?
There are actually three public entry points. Retained the older two to avoid 
changing more code than necessary. Can clean that up later.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4c615dbc1d14685a6966c5ca7538777cdc80b74d
Gerrit-Change-Number: 11920
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Comment-Date: Tue, 13 Nov 2018 21:41:28 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7841: Refactor QueryStmt, etc. for easier debugging

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11915 )

Change subject: IMPALA-7841: Refactor QueryStmt, etc. for easier debugging
..


Patch Set 1:

Dry-run tests passed: https://jenkins.impala.io/job/pre-review-test/224/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4f4fe3d3a1ab3170e294714dab066d40d169eff1
Gerrit-Change-Number: 11915
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Tue, 13 Nov 2018 21:44:50 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7807: Analysis test fixture

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7807: Analysis test fixture
..

IMPALA-7807: Analysis test fixture

Refactors the existing ExprRewriteRulesTest to pull out the framework
functionality into a "test fixture" class which can be used for a
variety of tests. The fixture allows more variation, and access to
more intermediate state, than does the existing function-based
framework. For example, the fixture allows setting per-query
options, allows doing full or partial (no rewrite) analysis, provides
access to things like the Analyzer, and more.

The fixture pulls in code from FrontEndBase to handle the low-level
tasks of running a query, and from ExprRewriteRulesTest for the
rewrite-specific aspects. For now, the FrontEndBase base class was
left unchanged.  ExprRewriteRulesTest is refactored to use the new
fixture in order to illustrate usage of the test fixture.

The key value of this work is to allow greater detail when testing
future change requests.  In order to keep the refactoring as simple
and clean, no new test cases were added here

Testing: since this is a test fixture, the refactored ExprRewriteRulesTest
implicitly tests the fixture code. No "production" (non-test) code was
changed in this patch.

Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
---
A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/QueryFixture.java
A fe/src/test/java/org/apache/impala/analysis/RewriteFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/testutil/TestUtils.java
6 files changed, 500 insertions(+), 89 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7807: Analysis test fixture

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Fredy Wijaya, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7807: Analysis test fixture
..

IMPALA-7807: Analysis test fixture

Refactors the existing ExprRewriteRulesTest to pull out the framework
functionality into a "test fixture" class which can be used for a
variety of tests. The fixture allows more variation, and access to
more intermediate state, than does the existing function-based
framework. For example, the fixture allows setting per-query
options, allows doing full or partial (no rewrite) analysis, provides
access to things like the Analyzer, and more.

The fixture pulls in code from FrontEndBase to handle the low-level
tasks of running a query, and from ExprRewriteRulesTest for the
rewrite-specific aspects. For now, the FrontEndBase base class was
left unchanged.  ExprRewriteRulesTest is refactored to use the new
fixture in order to illustrate usage of the test fixture.

The key value of this work is to allow greater detail when testing
future change requests.  In order to keep the refactoring as simple
and clean, no new test cases were added here

Testing: since this is a test fixture, the refactored ExprRewriteRulesTest
implicitly tests the fixture code. No "production" (non-test) code was
changed in this patch.

Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
---
A fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
A fe/src/test/java/org/apache/impala/analysis/QueryFixture.java
A fe/src/test/java/org/apache/impala/analysis/RewriteFixture.java
M fe/src/test/java/org/apache/impala/common/FrontendTestBase.java
M fe/src/test/java/org/apache/impala/testutil/TestUtils.java
6 files changed, 503 insertions(+), 89 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-Change-Number: 11881
Gerrit-PatchSet: 5
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7807: Analysis test fixture

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11881 )

Change subject: IMPALA-7807: Analysis test fixture
..


Patch Set 4:

(14 comments)

Thanks for the review! Suggestions applied. Also added a few explanations of 
future intent.

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

http://gerrit.cloudera.org:8080/#/c/11881/3//COMMIT_MSG@19
PS3, Line 19: the FrontEndBase base class was
> Can you please add a TODO there? There is a lot of common code and this mig
Good suggestions. Added the TODO.

My thought is to refine this fixture in another commit, then create an example 
test that shows folks exactly how to use the fixture, which seems more useful 
than a general suggestion. Is this OK?


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

http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@55
PS3, Line 55: this.db_ = db;
> Add an example usage here? (something like in ExprRewriterTest)
Added note here. Will add a more complete example as this class evolves to 
absorb more of the existing test framework functionality.


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@56
PS3, Line 56:
:
:   public void setOptions(TQueryOptions queryOptions) {
: this.queryOptions_ = queryOptions;
:   }
:
:   public TQueryOptions getOptions() {
: if (queryOptions_ == null) {
:   que
> Move to their respective class definitions?
Done


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@80
PS3, Line 80:   public TQueryCtx queryContext() {
> We generally use "_" suffix for class member variables. For ex: queryCtx_.
Done


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@86
PS3, Line 86: * using the options defined
> Do we need to track this separately?  can be inferred from the analysisResu
Done


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@103
PS3, Line 103: return new RewriteFixture(this);
> use log4j logger
In a test? This is an unexpected test failure. Do we normally want to paw 
though logs to find such failures?


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@109
PS3, Line 109: }
> We probably also need helper functions like parsesOk() etc? (to port Parser
Agreed. I'll add those as I modify code so that I can use those changed tests 
to test the implementation.


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@114
PS3, Line 114:
> move this to ctor?
At present this class does analysis in two ways: with or without rewrites. As I 
learn this stuff better, I suspect it would be better to split this into two 
classes. I'll save that for the next commit, if you agree.


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@135
PS3, Line 135:
> I think this also skips authz. Mention that?
Done


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@204
PS3, Line 204:
> Move it inside RewriteFixture? Doesn't seem like to be used else where.
Done


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@205
PS3, Line 205:
> Use "_" suffix.
Done


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@224
PS3, Line 224:
> Move it to its own class?  IIUC AnalysisFixture is a more generic thing tha
Done


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@256
PS3, Line 256:
> Does it make sense for RewriteFixture to extend AnalysisFixture to avoid th
Rewriting has some awkwardness that requires that things be in a different 
order here than in the base class. So, reuse by composition works a bit better 
here than reuse via inheritance.


http://gerrit.cloudera.org:8080/#/c/11881/3/fe/src/test/java/org/apache/impala/analysis/AnalysisFixture.java@284
PS3, Line 284:
> assert that it is analyzed before calling this?
Good idea in general. Here, the structure exists after parse, analysis only 
"decorates" this structure.



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Id69c99b284960f16394f61072d38dd00269bc10c
Gerrit-

[Impala-ASF-CR] IMPALA-7823: Clean up Java warnings, fix minor issues

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11893 )

Change subject: IMPALA-7823: Clean up Java warnings, fix minor issues
..


Patch Set 4:

(1 comment)

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

http://gerrit.cloudera.org:8080/#/c/11893/4/fe/src/main/java/org/apache/impala/analysis/CollectionStructType.java@28
PS4, Line 28: import com.google.common.base.Preconditions;
This turns out to be an excellent change. Thanks!



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I4b174d6448371c55e98487c6d4212c74c6b0c79e
Gerrit-Change-Number: 11893
Gerrit-PatchSet: 4
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Anonymous Coward (168)
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Thu, 15 Nov 2018 23:27:37 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging

[Paul Rogers (Code Review)]

Hello Bharath Vissapragada, Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging
..

IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging

Impala provides a single test class ExprRewriteRulesTest for all rewrite
rules. Each rule class has an associated test function. The
SimplifyConditionalRule class is complex, and the result test function
is quite large.

When doing work to modify a particular conditional rewrite, it became
clear that debugging would be much easier if each detailed rewrite had
its own test rather than using one huge test function. This ticket asks
to break up the big function. (JUnit does not care about small vs. large
functions.)

This patch splits the function separate from later work that will modify
the tests themselves.

Also unified test function naming, using the standard "javaStyle" rather
than the unusual "CppStyle" that was used.

Testing: this is a test, ran the test to ensure it still passes.

Change-Id: I99a28b170bc0132424041e51f61ebe5c848c4083
---
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
1 file changed, 76 insertions(+), 27 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I99a28b170bc0132424041e51f61ebe5c848c4083
Gerrit-Change-Number: 11890
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11890 )

Change subject: IMPALA-7821: Refactor ExprRewriteRulesTest for easier debugging
..


Patch Set 2:

(4 comments)

Thanks for the reviews. Addressed the comments, including removing the new (but 
disabled) tests. Will include those later along with the fixes that they 
motivated.

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

http://gerrit.cloudera.org:8080/#/c/11890/1//COMMIT_MSG@10
PS1, Line 10: Th
> ?
Done


http://gerrit.cloudera.org:8080/#/c/11890/1//COMMIT_MSG@26
PS1, Line 26: Testing: this is a test, ran the test to ensure it still passes.
:
> Should we add these tests separately along with the jira fixes? Not sure wh
Sure.


http://gerrit.cloudera.org:8080/#/c/11890/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
File fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java:

http://gerrit.cloudera.org:8080/#/c/11890/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@438
PS1, Line 438: l FALSE, return implicit NULL ELSE.
 : RewritesOk("decode(1, 1 + 1, id, 1 + 2, 3)", rules, "NULL");
 : // Multiple TRUE, first one becomes ELSE.
> nit: format to fewer lines
Done


http://gerrit.cloudera.org:8080/#/c/11890/1/fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java@484
PS1, Line 484: ("coalesce(1,
> nit: more meaningful name?
Done



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I99a28b170bc0132424041e51f61ebe5c848c4083
Gerrit-Change-Number: 11890
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Thu, 15 Nov 2018 23:41:08 +
Gerrit-HasComments: Yes

[Impala-ASF-CR] IMPALA-7801: Remove toSql() from ParseNode interface.

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11942 )

Change subject: IMPALA-7801: Remove toSql() from ParseNode interface.
..


Patch Set 2:

(6 comments)

Thanks much for the improvement. Some general comments sprinkled among the 
files.

http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/Expr.java
File fe/src/main/java/org/apache/impala/analysis/Expr.java:

http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@395
PS2, Line 395:   String sql = toSql(DEFAULT);
Let's think about the meaning of the options for expressions. Expr nodes won't 
have the state to provide un-rewritten SQL: a node is what it is, and does not 
know if it should be considered rewritten or not. The rewritten state is 
something that the outer non-expr node must provide.

Expr nodes can decide whether to include or exclude implicit casts.

This, then, raises and interesting issue. The ToSqlOptions enum is an enum: we 
get to pick one value. But, some of the options overlap: I may want to include 
implicit casts as one choice, and may want source or rewritten SQL on the other 
hand.

I wonder, can we make this simpler somehow?


http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/Expr.java@1300
PS2, Line 1300:   name, (printExpr) ? " '" + toSql(DEFAULT) + "'" : "", 
type_.toString()));
This comment applies in a zillion other places. Our general rule for error 
messages should be that we show the user's original SQL without rewrites or 
implicit casts. We have hundreds of such usages and we have to check that new 
messages use the correct form. Can we standardize these somehow?

One thought is to rename the option from DEFAULT to something like ORIGINAL or 
SOURCE. But, this means we still have to sprinkle this option across many error 
messages. This is, really, exposing implementation in too many places.

As it turns out, ParseNode is an interface, with many direct decedents. Does it 
make sense to introduce a new base class, AbstractParseNode, that has a 
userSql() (or sourceSql()) method that we use in error messages?

Recognizing that expressions are different than statements, maybe:

ParseNode
|- Expr (root of all expressions)
|- AbstractStmt (root of all statements)

The statement base would provide the source/rewritten options. Exprs only 
provide the implicit cast or not options.


http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/ParseNode.java
File fe/src/main/java/org/apache/impala/analysis/ParseNode.java:

http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/ParseNode.java@33
PS2, Line 33:   String toSql(ToSqlOptions options);
A common practice in Java is to include the enum definition in the interface 
file if the enum is used only by the interface (and its implementations).


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

http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/analysis/QueryStmt.java@217
PS2, Line 217:   "OFFSET requires an ORDER BY clause: " + 
limitElement_.toSql(DEFAULT).trim());
I wonder if we can solve a mess here with exceptions also. Rather than building 
message (very inconsistently) in each message, maybe have a builder:

throw new AnalysisException
  .builder("OFFSET requires an ORDER BY clause")
  .expr(limitElement_)
  .build();

The builder would have methods for expressions (which will call the proper 
toSql version), for statements, for additional context, and so on.

This would avoid the issue of exposing the magic toSql(DEFAULT) incantation in 
zillions of places.


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

http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/planner/SortNode.java@44
PS2, Line 44: import static org.apache.impala.analysis.ToSqlOptions.DEFAULT;
Two suggestions.

1. Rename this as ToSqlOption (or ToSqlFormat) since we can pass only one 
option.
2. Import the enum, not the static value. Reference it with the enum name: 
ToSqlFormat.DEFAULT (or, based on earlier suggestions, ToSqlFormat.SOURCE).


http://gerrit.cloudera.org:8080/#/c/11942/2/fe/src/main/java/org/apache/impala/planner/SortNode.java@214
PS2, Line 214: output.append(info_.getSortExprs().get(i).toSql(DEFAULT) 
+ " ");
This is an example of the point made elsewhere. It SEEMS we are asking the 
expression to give the DEFAULT (source) SQL. But, an expression can't make that 
decision. if we want the source SQL, we'd have to have cached it.

I'm working on a project to do just that in a more standardized form. (

[Impala-ASF-CR] IMPALA-7866: Predicates, helpers for implicit casts, slot refs

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11953


Change subject: IMPALA-7866: Predicates, helpers for implicit casts, slot refs
..

IMPALA-7866: Predicates, helpers for implicit casts, slot refs

* Refactors implicit cast functions into a predicate and a static helper
  rather than the methods on the Expr base class.
* Adds an IS_SLOT_REF predicate and replaces explcit instanceof checks
  with uses of the predicate.
* Other minor clean-up.

Tests: No functional changes, just refactoring. Reran existing tests.

Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613
---
M fe/src/main/java/org/apache/impala/analysis/Analyzer.java
M fe/src/main/java/org/apache/impala/analysis/CastExpr.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/PartitionSet.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectListItem.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/SortInfo.java
M fe/src/main/java/org/apache/impala/analysis/StmtRewriter.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/catalog/AggregateFunction.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/DataSourceScanNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsPartitionPruner.java
M fe/src/main/java/org/apache/impala/planner/KuduScanNode.java
M fe/src/main/java/org/apache/impala/planner/SingleNodePlanner.java
M fe/src/main/java/org/apache/impala/planner/SortNode.java
M fe/src/main/java/org/apache/impala/rewrite/RemoveRedundantStringCast.java
M fe/src/test/java/org/apache/impala/analysis/AnalyzeStmtsTest.java
20 files changed, 100 insertions(+), 95 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613
Gerrit-Change-Number: 11953
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7866: Predicates, helpers for implicit casts, slot refs

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11953 )

Change subject: IMPALA-7866: Predicates, helpers for implicit casts, slot refs
..


Patch Set 1:

Pre-commit tests passed: https://jenkins.impala.io/job/pre-review-test/228/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Ieaa0aee1b9015e0aed521f2038bf44513d7f8613
Gerrit-Change-Number: 11953
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Mon, 19 Nov 2018 19:33:01 +
Gerrit-HasComments: No

[Impala-ASF-CR] IMPALA-7865: Repeated type widening of arithmetic expressions

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11952


Change subject: IMPALA-7865: Repeated type widening of arithmetic expressions
..

IMPALA-7865: Repeated type widening of arithmetic expressions

In its present form, the analyser will analyze some expressions multiple
times: during analysis, after substututing references, again after
performing rewrites.

The analyzer implements a set of type propagation rules that generates a
resulting type for arithmetic expressions that is wider than the types
of the arguments. For example for a + 1, a and 1 are TINYINT, but the
result of the expression is promoted to SMALLINT.  The planner then sets
the type of the constant (1 here) to SMALLINT.

Now, repeat the process on the next cycle. a is TINYINT, 1 is a
SMALLINT. Now the result of the expression is INT and 1 is retyped to be
an INT. Repeat again and the expression (and constant) are promoted to
BIGINT.

Meanwhile, analysis has taken a clone of the expression with the old
types. As a result, the types of columns in the result list for a SELECT
statement can differ from the same columns recorded in the SELECT list.

This patch applies a number of partial fixes:

* When analyzing a numeric expression, the type of any constant argument
  is reset back to the default. (TINYINT in the example above.)
* When performing substitution, expressions are re-analyzed only if that
  expression itself changed, or one of its children changed.

A full fix is to enhance the analyzer to do a bottom-up, one pass
analysis, which is beyond the scope of this change.

Tests:

* Reran all FE tests, adjusting one Planner test.
* Added a new test case that demonstrates that types are widened only
  statically (for a + 1 + 1, say), but not dynamically (due to repeated
  re-analysis.)

Change-Id: I3a8f6d1c508a289083b5e026f33581bc44117ca2
---
M fe/src/main/java/org/apache/impala/analysis/ArithmeticExpr.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/NumericLiteral.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriteRulesTest.java
M fe/src/test/java/org/apache/impala/analysis/ExprRewriterTest.java
M 
testdata/workloads/functional-planner/queries/PlannerTest/predicate-propagation.test
6 files changed, 112 insertions(+), 32 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I3a8f6d1c508a289083b5e026f33581bc44117ca2
Gerrit-Change-Number: 11952
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7865: Repeated type widening of arithmetic expressions

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11952 )

Change subject: IMPALA-7865: Repeated type widening of arithmetic expressions
..


Patch Set 2:

Pre-commit tests passed: https://jenkins.impala.io/job/pre-review-test/229/


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I3a8f6d1c508a289083b5e026f33581bc44117ca2
Gerrit-Change-Number: 11952
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Mon, 19 Nov 2018 19:37:42 +
Gerrit-HasComments: No

[Impala-ASF-CR] MPALA-7867, part 1: Expose List in TreeNode, parser

[Paul Rogers (Code Review)]

Paul Rogers has uploaded this change for review. ( 
http://gerrit.cloudera.org:8080/11954


Change subject: MPALA-7867, part 1: Expose List in TreeNode, parser
..

MPALA-7867, part 1: Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
15 files changed, 99 insertions(+), 89 deletions(-)



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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 1
Gerrit-Owner: Paul Rogers 

[Impala-ASF-CR] IMPALA-7655: Rewrite if, isnull to use CASE

[Paul Rogers (Code Review)]

Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/11760 )

Change subject: IMPALA-7655: Rewrite if, isnull to use CASE
..


Abandoned

Will revisit after cleaning up blocking issues.
--
To view, visit http://gerrit.cloudera.org:8080/11760
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: I526654d8546e17b2545c42cc59dab66d9fe1b163
Gerrit-Change-Number: 11760
Gerrit-PatchSet: 11
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bharath Vissapragada 
Gerrit-Reviewer: Csaba Ringhofer 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7310: Use NDV=1 for a Column with all nulls

[Paul Rogers (Code Review)]

Paul Rogers has abandoned this change. ( http://gerrit.cloudera.org:8080/11528 )

Change subject: IMPALA-7310: Use NDV=1 for a Column with all nulls
..


Abandoned

Split test code into a separate patch. Will reopen this with just NDV changes 
after the test code is committed.
--
To view, visit http://gerrit.cloudera.org:8080/11528
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: abandon
Gerrit-Change-Id: Ife657a43c9cafc451bd12ddf857dcb7169e97459
Gerrit-Change-Number: 11528
Gerrit-PatchSet: 15
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Bikramjeet Vig 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Reviewer: Philip Zeyliger 
Gerrit-Reviewer: Tim Armstrong 
Gerrit-Reviewer: Vuk Ercegovac 

[Impala-ASF-CR] IMPALA-7867, part 1: Expose List in TreeNode, parser

[Paul Rogers (Code Review)]

Hello Impala Public Jenkins,

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

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

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

Change subject: IMPALA-7867, part 1: Expose List in TreeNode, parser
..

IMPALA-7867, part 1: Expose List in TreeNode, parser

When using Java collections, a common Java best practice is to expose
the collection interface, but hide the implementation choice. This
pattern allows us to start with a generic implementation (an ArrayList,
say), but evolve to a more specific implementation to achieve certain
goals (a LinkedList or ImmutableList, say.)

For whatever reason, the Impala FE code exposes ArrayList, HashMap and
other implementation choices as variable types and in method signatures.

Also, since Java 7, the preferred way to create an array is

new ArrayList<>()

Replaced older forms:

new ArrayList() // Pre-Java 7
Lists.newArrayList() // Guava form, pre-Java 7

This ticket cleans up two files, and their dependencies:

* TreeNode (the root of all parser nodes)
* sql-parser.cup (the code which creates the parser nodes)

Many other uses exist, and will be submitted as separate patches to keep
patches small.

Tests: This is purely a refactoring, no functionality changed. Ran the
FE unit tests to verify no regressions.

Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
---
M fe/src/main/cup/sql-parser.cup
M fe/src/main/java/org/apache/impala/analysis/DescribeTableStmt.java
M fe/src/main/java/org/apache/impala/analysis/Expr.java
M fe/src/main/java/org/apache/impala/analysis/FunctionName.java
M fe/src/main/java/org/apache/impala/analysis/QueryStmt.java
M fe/src/main/java/org/apache/impala/analysis/SelectStmt.java
M fe/src/main/java/org/apache/impala/analysis/SlotRef.java
M fe/src/main/java/org/apache/impala/analysis/UnionStmt.java
M fe/src/main/java/org/apache/impala/analysis/ValuesStmt.java
M fe/src/main/java/org/apache/impala/analysis/WithClause.java
M fe/src/main/java/org/apache/impala/catalog/StructType.java
M fe/src/main/java/org/apache/impala/common/TreeNode.java
M fe/src/main/java/org/apache/impala/planner/HdfsScanNode.java
M fe/src/main/java/org/apache/impala/planner/Planner.java
M fe/src/main/java/org/apache/impala/service/Frontend.java
15 files changed, 99 insertions(+), 89 deletions(-)


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Impala Public Jenkins 

[Impala-ASF-CR] IMPALA-7867, part 1: Expose List in TreeNode, parser

[Paul Rogers (Code Review)]

Paul Rogers has posted comments on this change. ( 
http://gerrit.cloudera.org:8080/11954 )

Change subject: IMPALA-7867, part 1: Expose List in TreeNode, parser
..


Patch Set 2:

This is a simple clean-up patch. No urgency.


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

Gerrit-Project: Impala-ASF
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: Iebab7dccdb4b2fa0b5ca812beab0e8bdba39f539
Gerrit-Change-Number: 11954
Gerrit-PatchSet: 2
Gerrit-Owner: Paul Rogers 
Gerrit-Reviewer: Fredy Wijaya 
Gerrit-Reviewer: Impala Public Jenkins 
Gerrit-Reviewer: Paul Rogers 
Gerrit-Comment-Date: Tue, 20 Nov 2018 18:41:14 +
Gerrit-HasComments: No