[SPARK-12018][SQL] Refactor common subexpression elimination code

[viirya]

JIRA: https://issues.apache.org/jira/browse/SPARK-12018

The code of common subexpression elimination can be factored and simplified. Some unnecessary variables can be removed.

[viirya]

Actually, the above variables are not unnecessary. I will remove them later if the test is passed.

[SparkQA]

Test build #46783 has finished for PR 10009 at commit 4876ce0.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * case class SubExprEliminationState(isNull: String, value: String)\n

[rxin]

cc @nongli

[cloud-fan]

can we make this method return void? we can just assign values to isNull and value as they are both member variables.

[viirya]

ok. make sense.

[cloud-fan]

how about The collection of sub-exression result reset method that need to be called on each row.?

[cloud-fan]

LGTM overall, cc @nongli to take another look.

[SparkQA]

Test build #46795 has finished for PR 10009 at commit b3cf6a8.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * case class SubExprEliminationState(isNull: String, value: String)\n

[SparkQA]

Test build #46799 has finished for PR 10009 at commit 8e937d3.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * case class SubExprEliminationState(isNull: String, value: String)\n

[SparkQA]

Test build #46803 has finished for PR 10009 at commit b162ba5.

• This patch passes all tests.
• This patch merges cleanly.
• This patch adds the following public classes (experimental):\n * case class SubExprEliminationState(isNull: String, value: String)\n

[viirya]

ping @rxin @cloud-fan @nongli

[nongli]

Am I right in saying you've picked a different approach to do this? You removed isLoaded and just load it every time. I think this is scary because it does not allow shortcircuiting. This can be bad if it is expr1 AND expr2 and expr1 is selective or expr2 is expensive.

[viirya]

I don't understand your point. $fnName will be called only once for each row. This patch doesn't change that.

[nongli]

Nvm, what I said doesn't apply for this operator.

I was thinking for Filter you wouldn't want to call fnName once for each row. It can be called less than once.

LGTM

[marmbrus]

Thanks, merging to master and 1.6