Conversion to ANTLR 4 (work in progress)

[sharwell]

This is a work-in-progress showing the conversion of BQL.g (ANTLR 3.4) to BQL.g4 (ANTLR 4.2). Here is the current state of the conversion:

• Almost all custom actions have been removed from BQL.g4 and placed in a separate listener-based analyzer for the compiler, BQLCompilerAnalyzer.java.
• All tests in TestBQL.java pass.
• All tests in TestErrorHandling.java fail, since BQLCompiler.getErrorMessage has not been updated to work with the ANTLR 4 exceptions. All tests in TestErrorHandling.java now pass and have greatly improved error locality compared to the ANTLR 3 reporting; the pull request was updated to work with ANTLR 4 exceptions.

[sharwell]

Feel free to leave comments in the commits or here if you have questions

[yozhao]

Hi Sam,
In antlr3 we have many test cases like this, it reports line 4 has error.

@SuppressWarnings("unused")
@test
public void testInvalidInPredValues() throws Exception {
System.out.println("testInvalidInPredValues");
System.out.println("==================================================");

boolean caughtException = false;
try {
  JSONObject json = _compiler.compile("SELECT category \n" + "FROM cars \n"
      + "WHERE color in ('red', 2000, 'blue') \n" + "  AND price < 1750.00");
} catch (IllegalStateException err) {
  caughtException = true;
  System.out.println("errorMsg:" + _compiler.getErrorMessage(err));
  assertEquals(
    "[line:4, col:2] Value list for IN predicate of facet \"color\" contains incompatible value(s). (token=AND)",
    _compiler.getErrorMessage(err));
} finally {
  assertTrue(caughtException);
}

}

But it is obviously line 3 "WHERE color in ('red', 2000, 'blue')", while antlr reports correct line.

I got positions from ctx, like this. Do you think it is ok to change ut according the position like this:

public void exitIn_predicate(BQLParser.In_predicateContext ctx) {
    String col = getTextProperty(ctx.column_name());
    String[] facetInfo = _facetInfoMap.get(col);
    System.out.println("line:" + ctx.start.getLine());
    System.out.println("col:" + ctx.start.getCharPositionInLine());

[sharwell]

I added 3 commits showing how I would update the error reporting for the validation in several predicate methods. For your specific example, the error is located at the token 2000 inside the value of the WHERE clause, so that's where this code reports it.

[yozhao]

I have fixed the test case.

Error message are from 2 sources now.

1. IllegalStateException from BQLCompilerAnalyzer
2. ParseCancellationException from BQLParser

yozhao@aeeaf53

[sharwell]

I added a 4th commit to correct the tests in TestBQL now that some methods throw ParseCancellationException instead of IllegalStateException. Ideally, the entire BQLCompilerAnalyzer would be updated to throw a SemanticException for any case where context information is available (which is essentially the entire file). I wrapped the exception in a ParseCancellationException (which is a RuntimeException) to indicate that the analysis process is being cancelled.

[sharwell]

The problem with embedding the line number in the error message is it hinders the ability of an IDE to highlight errors spanning multiple tokens. By including the ParseTree itself (see SemanticException.getNode()), a tool could handle the exception by underlining an entire sequence of tokens responsible for a semantic error.

[yozhao]

So you don;t suggest embed the line number in the error message?

[yozhao]

I will try to use your latest code tomorrow.

[sharwell]

With the latest two commits, all unit tests in TestBQL and TestErrorHandling pass.

[yozhao]

Thanks Sam!
I will do a test, if no problem I will merge this change soon.

[yozhao]

Merged thanks Sam!

[otisg]

Huge thanks to @sharwell and big +1 to @yozhao and @bcui for getting this in so quickly!