Unifying in general sounds like a good idea. I haven’t thought through the specifics in this case.
Note that each time we generate a parser using JavaCC — which we do in several places in the code — it generates a new exception class. We don’t want to use those generated classes. We would like to convert those exceptions to non-generated exception classes, and I believe we do. It definitely be good if locations are converted to use SqlParserPos, i.e. (startCol, startLine, endCol, endLine) rather than (col, line) or a position embedded in a string. Julian > On Jun 26, 2019, at 3:54 AM, Danny Chan <yuzhao....@gmail.com> wrote: > > Now our parser has 3 kinds of throws behavior > > [1] Use JavaCC generateParseException > [2] Use SqlUtil.newContextException > [3] Use JavaCC ParseExecption directly > > For [1] and [2] there is a position info in the exception message, a throw > may like: >> From line 1, column 15 to line 1, column 26: > > But for 3, we only have the error message without pos info, which is not that > user friendly when > the sql text is huge (there are 10 occurance in our parser). > > So shall we unify them ? E.G. Use only 1 and 2 is enough for all the cases, > the 2 can totally replace 3. > > Do you think this is necessary ? > > > [1] > https://github.com/apache/calcite/blob/69c8053cd98ec65c55fa1c3b282b076536ab758f/core/src/main/codegen/templates/Parser.jj#L4494 > [2] > https://github.com/apache/calcite/blob/69c8053cd98ec65c55fa1c3b282b076536ab758f/core/src/main/codegen/templates/Parser.jj#L386 > [3] > https://github.com/apache/calcite/blob/69c8053cd98ec65c55fa1c3b282b076536ab758f/core/src/main/codegen/templates/Parser.jj#L601 > > Best, > Danny Chan
