For NavigationExpander, currently I have not got a quite convincing explanation we should add it back. While for NavigationReplacer, I think it has its values. NavigationReplacer can convert expressions like A.price > PREV(B.price) to PREV(A.price, 0) > PREV(B.price, 1). This will make the implementation unified:
1) step 1: RexPatternFieldRef A.price or B.price
2) step 2: RexCall (PREV)

I have created a PR: https://github.com/apache/calcite/pull/516

Julian Hyde, Zhiqiang He, Please help to review.

Why is PREV("A"."net_weight", 0) better than "A"."net_weight"?

I get the sense that you have had design discussions in private. Nowhere have you explained what "navigation" is.

You're right. This is related to how to implement the above expression. I think PREV("A"."net_weight", 0) will make the implementation more unified.
Let's image how we implement PREV("A"."net_weight", 0). This may be separated into two steps:
1) Calculate "A"."net_weight" (suppose the result is result1)
2) Calculate PREV(result1, 0)
This means that the result of "A"."net_weight" is better not be an integer value (this depends on the implementation), otherwise it will be impossible to calculate PREV(result1, 0) as it makes no sense if the first argument of PREV is an integer value.

So for expression A.net_weight > PREV(B.net_weight, 1), the type of A.net_weight may not be an integer value (depends on the implementation) and the compare may make no sense.

In one word, if we don't transform A.net_weight to PREV(A.net_weight, 0), it will make it difficult to implement the following method in RexVisitor.

  R visitPatternFieldRef(RexPatternFieldRef fieldRef);

If the return result of this method is the specified field of the element, then the negation will be impossible. If not, the comparation of A.net_weight > PREV(B.net_weight, 1) will make non sense.

Makes sense. Please document that rationale in the code and I will accept the PR. Add a comment to this case when you have added the commit. No need to squash or rebase.

Julian Hyde, I have updated the PR, documented the rationale at the header of class NavigationReplacer and also updated the commit message.

Looks good. I will commit when tests pass. I changed NavigationReplacer.visit to call super.visit; it's simpler than what you had.

On reflection, your approach of modifying the SqlNode tree is an anti-pattern. Ask anyone who has tried to optimize queries by changing the AST. It quickly becomes a mess. The next time you need something like NavigationReplacer, it should operate on RexNode rather than SqlNode.

Fixed in http://git-wip-us.apache.org/repos/asf/calcite/commit/d3a7c0d7; thanks for the PR, Dian Fu!

Resolved in release 1.14.0 (2017-10-01)