[IMPALA-7752] Stmt.reset() doesn't (and can't)

• Type: Bug
• Status: Open
• Priority: Minor
• Resolution: Unresolved
• Affects Version/s: Impala 3.0
• Component/s: Frontend
• Epic Color: ghx-label-4
• Assignee: Unassigned
• Reporter: Paul Rogers
• Created: 25/Oct/18 03:12
• Updated: 07/Nov/18 19:47

Description

The test ExprRewriteTest has the following logic:

  public void RewritesOk(String stmt, int expectedNumChanges,
      int expectedNumExprTrees) throws ImpalaException {
    // Analyze without rewrites since that's what we want to test here.
    StatementBase parsedStmt = (StatementBase) ParsesOk(stmt);
    ...
    parsedStmt.rewriteExprs(exprToTrue_);
    ...

    // Make sure the stmt can be successfully re-analyzed.
    parsedStmt.reset();
    AnalyzesOkNoRewrite(parsedStmt);
  }

Basically, this replaces all expressions with a Boolean constant, then counts the number of replacements. A fine test. Then, the reset()) call is supposed to put things back the way they were.

The problem is, the rewrite rule replaces the one and only copy of the SELECT list expressions. The second time around, we get a failure because the ORDER BY clause (which was kept as an original copy) refers to the now-gone SELECT clause.

This error was not previously seen because a prior bug masked it.

This is an odd bug as reset() is called only from this one place.

The premise of test itself is invalid: we want to know that, after we rewrite the query from

select a.int_col a, 10 b, 20.2 c, ...
order by a.int_col, 4 limit 10

To

select FALSE a, FALSE b, FALSE c, ...
order by a.int_col, 4 limit 10

We assert that the query should again analyze correctly. This is an unrealistic expectation. Once the above bug is fixed, we verify that the new query is actually invalid, which, in fact, it is.

Two fixes are possible:

1. Create copies of all lists that are rewritten (SELECT, HAVING, etc.)
2. Remove the reset() test and (since this is the only use), the reset() code since it cannot actually do what it is advertised to do.

Since reset() is never used except in tests, and the premise is invalid, this ticket proposes to remove the reset() logic and remove the part of the test code that validates the reset.

Issue Links

• is part of: IMPALA-7831 Revisit expression rewriting integration with planner
• is part of: IMPALA-7747 Clean up the Expression Rewriter