[SPARK-2509][SQL] Add optimization for Substring.

[ueshin]

Substring including null literal cases could be added to NullPropagation.

[SparkQA]

QA tests have started for PR 1428. This patch merges cleanly.
View progress: https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16705/consoleFull

[rxin]

Is this not doable with constant folding?

[rxin]

Now I think about it more, this is probably ok to leave it here since in constant folding we don't really special case for null literals. However, I looked into other rules in NullPropagation and a lot of them don't really belong there... (has nothing to do with your PR though)

[ueshin]

@rxin, thank you for your comment.
The rule ConstantFolding can be applied only if the expression is foldable, i.e. ALL of the child expressions are foldable, and the NullPropagation covers the case there exists at least one null literal regardless of foldabilities of the others.
So I think the cases are needed.

BTW, Substring can be foldable, right?

[SparkQA]

QA results for PR 1428:
- This patch PASSES unit tests.
- This patch merges cleanly
- This patch adds no public classes

For more information see test ouptut:
https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/16705/consoleFull

[rxin]

Thanks. Merging this in master & branch-1.0.

[ueshin]

@rxin Ah, wait for a moment.
What do you think about foldability of the Substring I mentioned above?

[ueshin]

I'll open another issue about foldability of Substring.

[rxin]

I've already merged this. I think we should re-evaluate in the future whether we would want to push some stuff into expression foldability. I think that might also lead to higher planner performance (although that's probably less important). More importantly, it does feel more natural to me that this stuff goes into each expression, rather than having a rule for each NullPropagation.

cc @marmbrus

[ueshin]

@rxin I agree that we need the better way for NullPropagation instead of too much pattern matching.

[marmbrus]

You are right that this rule does more than null propagation now. I'm not sure what a better name would be. DegenerateExpressionSimplification?

Regarding moving null propagation into the expressions, you could do it... but what would it look like? You specify which of the children make the entire expression null if they are null? Seems like a lot of refactoring for little benefit...

[rxin]

It doesn't bring much benefit right now, but what we are doing here is creating patterns in NullPropagation to specify the semantics of each individual expression ... not very scalable in maintaining this code base.

[marmbrus]

I'm not sure I agree with that. This is a pretty niche optimization not something fundamental about the expressions that is required for correct evaluation (and the first version of this code had a bunch of mistakes). Having all of these rules in one place made it easier for me to realize that when doing the review.

[rxin]

Well with that argument we shouldn't have any of the constant folding logic in expressions themselves either since they don't affect correctness.

This is a situation that it is not obvious where to put the stuff. Anyway I think it's ok for them to be here for now, since it is not too long yet. As the list gets longer, we can revisit.

[marmbrus]

That is exactly the argument I made when the folding logic was added. :) I suggested that we add deterministic instead and then have a rule that folds things that are deterministic and have no references. deterministic I would argue is something fundamental about the expression itself, unlike these optimizations we are making.

[marmbrus]

I would be supportive of changing it to match my original proposal...