[GitHub] [accumulo-access] keith-turner commented on pull request #7: reimplements java parser and evaluator

["keith-turner (via GitHub)" <gi...@apache.org>]

keith-turner commented on PR #7:
URL: https://github.com/apache/accumulo-access/pull/7#issuecomment-1724537334

   > It's not clear what the goal of this reimplementation was, or what the substantive differences are.
   
   I should have explained that when I opened the PR.  These changes started off with me trying to implement the ABNF grammar in Java code.  Going down that path I ended with a much simpler and easier to understand parser than currently exist in the code.  So I continued with re-implementing  the parse tree code in addition to the parser, and I think the new parse tree code is large improvement over what exists.   
   
   Compare the [new parsing code](  https://github.com/apache/accumulo-access/blob/6ff14a5b1fecb120b6c40a0b411d45e0662f3b2f/src/main/java/org/apache/accumulo/access/Parser.java) vs [old parsing code](https://github.com/apache/accumulo-access/blob/295a5cd30229799f32aa58a1342be85a6a656f22/src/main/java/org/apache/accumulo/access/AccessExpressionImpl.java#L308-L474).
   
   The new parse tree (and all of the operation on the parse tree) is found in [AeNode.java](https://github.com/apache/accumulo-access/blob/6ff14a5b1fecb120b6c40a0b411d45e0662f3b2f/src/main/java/org/apache/accumulo/access/AeNode.java).  For the old parse tree, the data structure and the operations on the data structure are in multiple places in multiple files, so not easy to post a a link for comparison.
   
   So in my opinion the parser and the parse tree are much easier to understand in this new version of the code.  I think it would be much easier for someone looking to implement Accumulo Access in another language to follow this code.
   
   Also I was able to get 100% test coverage for the new code.  For the old code there were odd cases in the code that were not covered by the test and I am not sure if they were reachable, but the older code is harder to analyze.
   
   > So, my main concern would be how well does this perform compared to the previous implementation
   
   I used #10 to run some test with the current implementation and the new implementation in this PR.  For those test this new impl was slightly faster.
   
   Below is the output of running #10 on the current impl.
   
   ```
   Benchmark                                               Mode  Cnt   Score   Error   Units
   AccessExpressionBenchmark.measureBytesParsing          thrpt   12  11.964 ± 0.362  ops/us
   AccessExpressionBenchmark.measureEvaluation            thrpt   12  13.474 ± 0.643  ops/us
   AccessExpressionBenchmark.measureEvaluationAndParsing  thrpt   12   5.779 ± 0.121  ops/us
   AccessExpressionBenchmark.measureStringParsing         thrpt   12  10.111 ± 0.207  ops/us
   ```
   
   Below is the output of running #10 on this impl in the PR.
   
   ```
   Benchmark                                               Mode  Cnt   Score   Error   Units
   AccessExpressionBenchmark.measureBytesParsing          thrpt   12  11.799 ± 0.297  ops/us
   AccessExpressionBenchmark.measureEvaluation            thrpt   12  15.295 ± 0.199  ops/us
   AccessExpressionBenchmark.measureEvaluationAndParsing  thrpt   12   6.415 ± 0.046  ops/us
   AccessExpressionBenchmark.measureStringParsing         thrpt   12  10.037 ± 0.957  ops/us
   ```
   
   Seems the old impl is slightly faster for parsing and the new impl is slightly faster for evaluating.  For parsing+evaluating this new impl is faster at 6.415 operations/microsecond vs 5.779 operations/microsecond.  
   
   I will include a lot of this info in the commit message if merged.  Also I am going to update the class level javadocs with a short description of each classes purpose.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org