From 1101de57f36bab2dc35bbc963452f9b08ac027a8 Mon Sep 17 00:00:00 2001 From: Nikola Grcevski Date: Mon, 16 Dec 2019 16:57:28 -0500 Subject: [PATCH] Avoid re-parsing sql expressions. --- .../pinot/pql/parsers/Pql2Compiler.java | 45 +++++++++++++++++-- .../pinot/pql/parsers/Pql2CompilerTest.java | 32 +++++++++++++ 2 files changed, 74 insertions(+), 3 deletions(-) diff --git a/pinot-common/src/main/java/org/apache/pinot/pql/parsers/Pql2Compiler.java b/pinot-common/src/main/java/org/apache/pinot/pql/parsers/Pql2Compiler.java index 9785ee00e60b..194d327f2218 100644 --- a/pinot-common/src/main/java/org/apache/pinot/pql/parsers/Pql2Compiler.java +++ b/pinot-common/src/main/java/org/apache/pinot/pql/parsers/Pql2Compiler.java @@ -24,6 +24,9 @@ import javax.annotation.Nonnull; import javax.annotation.Nullable; import javax.annotation.concurrent.ThreadSafe; + +import com.github.benmanes.caffeine.cache.Cache; +import com.github.benmanes.caffeine.cache.Caffeine; import org.antlr.v4.runtime.ANTLRInputStream; import org.antlr.v4.runtime.BailErrorStrategy; import org.antlr.v4.runtime.BaseErrorListener; @@ -73,6 +76,13 @@ public class Pql2Compiler implements AbstractCompiler { public static String ENABLE_DISTINCT_KEY = "pinot.distinct.enabled"; public static boolean ENABLE_DISTINCT = Boolean.valueOf(System.getProperty(ENABLE_DISTINCT_KEY, "true")); + private static final int REQUEST_CACHE_SIZE = 1_000; + private static final int TRANSFORM_CACHE_SIZE = 1_000; + + private static final Cache _brokerRequestCache = Caffeine.newBuilder().maximumSize(REQUEST_CACHE_SIZE).build(); + private static final Cache _brokerRequestCacheWithPinotQuery = Caffeine.newBuilder().maximumSize(REQUEST_CACHE_SIZE).build(); + private static final Cache _expressionTreeCache = Caffeine.newBuilder().maximumSize(TRANSFORM_CACHE_SIZE).build(); + private static class ErrorListener extends BaseErrorListener { @Override @@ -84,6 +94,14 @@ public void syntaxError(@Nonnull Recognizer recognizer, @Nullable Object o private static final ErrorListener ERROR_LISTENER = new ErrorListener(); + private static Cache getBrokerRequestCache() { + if (ENABLE_PINOT_QUERY) { + return _brokerRequestCacheWithPinotQuery; + } + + return _brokerRequestCache; + } + /** * Compile the given expression into {@link BrokerRequest}. * @@ -94,7 +112,13 @@ public void syntaxError(@Nonnull Recognizer recognizer, @Nullable Object o public BrokerRequest compileToBrokerRequest(String expression) throws Pql2CompilationException { try { - // + // Try to load previously cached result + BrokerRequest cached = getBrokerRequestCache().getIfPresent(expression); + if (cached != null) { + return cached.deepCopy(); + } + + // Initialize CharStream charStream = new ANTLRInputStream(expression); PQL2Lexer lexer = new PQL2Lexer(charStream); lexer.setTokenFactory(new CommonTokenFactory(true)); @@ -144,7 +168,11 @@ public BrokerRequest compileToBrokerRequest(String expression) } } } - return brokerRequest; + + // Cache parsed broker request for reuse + getBrokerRequestCache().put(expression, brokerRequest); + + return brokerRequest.deepCopy(); } catch (Pql2CompilationException e) { throw e; } catch (Exception e) { @@ -153,6 +181,13 @@ public BrokerRequest compileToBrokerRequest(String expression) } public TransformExpressionTree compileToExpressionTree(String expression) { + // Check for cached expression tree + TransformExpressionTree transformExpressionTree = _expressionTreeCache.getIfPresent(expression); + if (transformExpressionTree != null) { + return transformExpressionTree; + } + + // Initialize CharStream charStream = new ANTLRInputStream(expression); PQL2Lexer lexer = new PQL2Lexer(charStream); lexer.setTokenFactory(new CommonTokenFactory(true)); @@ -167,7 +202,11 @@ public TransformExpressionTree compileToExpressionTree(String expression) { Pql2AstListener listener = new Pql2AstListener(expression); walker.walk(listener, parseTree); - return new TransformExpressionTree(listener.getRootNode()); + // Cache result for future reuse + transformExpressionTree = new TransformExpressionTree(listener.getRootNode()); + _expressionTreeCache.put(expression, transformExpressionTree); + + return transformExpressionTree; } private void validateHavingClause(AstNode rootNode) { diff --git a/pinot-common/src/test/java/org/apache/pinot/pql/parsers/Pql2CompilerTest.java b/pinot-common/src/test/java/org/apache/pinot/pql/parsers/Pql2CompilerTest.java index e633d0f669ac..e56e067a84d3 100644 --- a/pinot-common/src/test/java/org/apache/pinot/pql/parsers/Pql2CompilerTest.java +++ b/pinot-common/src/test/java/org/apache/pinot/pql/parsers/Pql2CompilerTest.java @@ -39,6 +39,8 @@ import org.testng.Assert; import org.testng.annotations.Test; +import static junit.framework.Assert.*; + /** * Some tests for the PQL 2 compiler. @@ -476,4 +478,34 @@ private void testOrderBy(String pql, List orderBys, List isAscs Assert.assertEquals(orderBy.isIsAsc(), isAscs.get(i).booleanValue()); } } + + /** + * Test that ensures we are properly caching requests for performance reasons + */ + @Test + public void testQueryCaching() { + final String PQL = "select * from table order by d2 asc, d3 desc"; + final String PQL1 = "select * from table order by d2, d3 desc"; + final String EXP = "`a.b.c`"; + final String EXP1 = "`a.b.d`"; + + BrokerRequest brokerRequest = COMPILER.compileToBrokerRequest(PQL); + BrokerRequest secondBrokerRequest = COMPILER.compileToBrokerRequest(PQL1); + BrokerRequest redoBrokerRequest = COMPILER.compileToBrokerRequest(PQL); + + assertEquals(brokerRequest, redoBrokerRequest); + // This is completely different Broker Request + assertFalse(brokerRequest == secondBrokerRequest); + // Identity equality won't work because we do deepCopy since broker request options are mutable + assertFalse(brokerRequest == redoBrokerRequest); + + TransformExpressionTree expTree = COMPILER.compileToExpressionTree(EXP); + TransformExpressionTree secondExpTree = COMPILER.compileToExpressionTree(EXP1); + TransformExpressionTree redoExpTree = COMPILER.compileToExpressionTree(EXP); + + // This is completely different Broker Request + assertFalse(expTree == secondExpTree); + // Identity equality should work because we have cached the previous request + assertTrue(expTree == redoExpTree); + } }