Adding Calcite SQL Parser and make another entry point to query Pinot #4387

xiangfu0 · 2019-06-30T19:47:37Z

As part of winedepot#14, this PR implements a Calcite SQL parser to compile SQL to PinotQuery.

Something to be noticed as syntax backward incompatible with PQL.

'!=' is not supported, need to replace it with '<>';
Double quote is used for identifier;
Single quotes are used for string literal and it's part of the parsed string. It's up to us to keep it or remove it.

Currently I removed the single quote from StringLiteral to match PQL parsing behavior.
See:

org.apache.pinot.common.utils.request.RequestUtils

literal.setStringValue(node.toString().replaceAll("^\'|\'$", ""));

… use SQL

codecov-io · 2019-06-30T21:39:46Z

Codecov Report

Merging #4387 into master will increase coverage by 9.79%.
The diff coverage is 57.7%.

@@             Coverage Diff             @@
##             master   #4387      +/-   ##
===========================================
+ Coverage      55.7%   65.5%   +9.79%     
  Complexity       20      20              
===========================================
  Files          1066    1075       +9     
  Lines         55403   55835     +432     
  Branches       7894    7964      +70     
===========================================
+ Hits          30865   36573    +5708     
+ Misses        22150   16644    -5506     
- Partials       2388    2618     +230

Impacted Files	Coverage Δ	Complexity Δ
...org/apache/pinot/common/utils/CommonConstants.java	`48.97% <ø> (+18.54%)`	`0 <0> (ø)`	⬇️
...pql/parsers/PinotQuery2BrokerRequestConverter.java	`87.37% <100%> (+2.67%)`	`0 <0> (ø)`	⬇️
...che/pinot/pql/parsers/pql2/ast/OrderByAstNode.java	`83.33% <100%> (ø)`	`0 <0> (ø)`	⬇️
...g/apache/pinot/sql/parsers/CalciteSqlCompiler.java	`100% <100%> (ø)`	`0 <0> (?)`
...roker/requesthandler/BaseBrokerRequestHandler.java	`77.5% <100%> (+66.95%)`	`0 <0> (ø)`	⬇️
...pinot/pql/parsers/pql2/ast/InPredicateAstNode.java	`42.46% <100%> (ø)`	`0 <0> (ø)`	⬇️
.../pql/parsers/pql2/ast/BetweenPredicateAstNode.java	`33.96% <100%> (ø)`	`0 <0> (ø)`	⬇️
...not/pql/parsers/pql2/ast/PredicateListAstNode.java	`76.74% <100%> (ø)`	`0 <0> (ø)`	⬇️
...inot/pql/parsers/pql2/ast/OutputColumnAstNode.java	`95.12% <100%> (+19.51%)`	`0 <0> (ø)`	⬇️
...attenNestedPredicatesFilterQueryTreeOptimizer.java	`88.88% <100%> (+0.42%)`	`0 <0> (ø)`	⬇️
... and 364 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 33f583b...fd8ec34. Read the comment docs.

kishoreg

Can we not parse PQL using calcite after removing options.

kishoreg · 2019-07-01T20:55:57Z

pinot-api/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java

@@ -83,6 +88,22 @@ public BrokerResponse executeQuery(String brokerAddress, String query)
    }
  }

+  @Override
+  public BrokerResponse executeSqlQuery(String brokerAddress, String query)


better to add another overloaded parameter to executeQueryAsync to take queryFormat as a param (default can be PQL for now)

Make Query an object

pinot-broker/src/test/java/org/apache/pinot/broker/request/PqlAndCalciteSqlCompatibleTest.java

kishoreg · 2019-07-01T20:58:54Z

pinot-broker/src/test/java/org/apache/pinot/broker/request/PqlAndCalciteSqlCompatibleTest.java

+    }
+  }
+
+  public boolean validate(BrokerRequest br1, BrokerRequest br2) {


we should move this to some Validator so that we can invoke it from other places

pinot-common/src/main/java/org/apache/pinot/sql/parsers/CalciteSqlParser.java

pinot-common/src/test/resources/pql_queries.list

…ql parsing logic seamlessly

pinot-api/src/main/java/org/apache/pinot/client/JsonAsyncHttpPinotClientTransport.java

pinot-api/src/main/java/org/apache/pinot/client/Request.java

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/PinotQueryParserFactory.java

mayankshriv · 2019-07-05T01:01:55Z

pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java

@@ -338,6 +337,13 @@ public BrokerResponse handleRequest(JsonNode request, @Nullable RequesterIdentit
    return brokerResponse;
  }

+  private PinotQueryRequest getPinotQueryRequest(JsonNode request) {
+    if (request.has(SQL)) {


Would be good to assert that only one of SQL|PQL is specified in the request.

Since sql support is newly added, I feel it's ok to keep sql parsing as first priority comparing to PQL.
So existing PQL user won't get affected.

...ache/pinot/broker/requesthandler/MultipleOrEqualitiesToInClauseFilterQueryTreeOptimizer.java

...-broker/src/test/java/org/apache/pinot/broker/request/PqlAndCalciteSqlCompatibilityTest.java

mayankshriv · 2019-07-05T01:16:31Z

...-broker/src/test/java/org/apache/pinot/broker/request/PqlAndCalciteSqlCompatibilityTest.java

+  public void testPqlAndSqlCompatible()
+      throws Exception {
+    final BufferedReader brPql = new BufferedReader(new InputStreamReader(
+        PqlAndCalciteSqlCompatibilityTest.class.getClassLoader().getResourceAsStream("pql_queries.list")));


Are the queries.list comprehensive enough to test all the various PQL syntax features?

I don't think this will cover full syntax features. I've added some more queries into it. We should keep adding more query patterns.

Ok, we should ensure that we have full coverage before we advertise the SQL option. Let's open a issue to track that.

mayankshriv · 2019-07-05T01:17:30Z

pinot-common/pom.xml

@@ -96,6 +96,10 @@
      <groupId>org.antlr</groupId>
      <artifactId>antlr4-runtime</artifactId>
    </dependency>
+    <dependency>
+      <groupId>org.apache.calcite</groupId>
+      <artifactId>calcite-core</artifactId>


Does this have a lot more functionality than the complier that we won't need? If so, is there a different artifact to include that just has what we need?

this is the artifact we need in order to use SqlParser(https://github.com/apache/calcite/blob/master/core/src/main/java/org/apache/calcite/sql/parser/SqlParser.java).

So are we bringing in a lot more (in calcite-core) than we need? Probably we don't have an option, but in case we do, consider just getting the minimal artifact available that we need.

yes, we may exclude the unnecessary dependencies once conflict is found.

mayankshriv

A couple of minor comments need responding before pushing the change. LGTM, otherwise.

Adding Calcite SQL Parser and make another entry point to query Pinot…

45c8dea

… use SQL

xiangfu0 force-pushed the upstream_master branch from 54648fb to 45c8dea Compare June 30, 2019 21:00

kishoreg reviewed Jul 1, 2019

View reviewed changes

xiangfu0 added 2 commits July 1, 2019 23:57

Make CalciteSqlCompiler to implement AbstractCompiler to handle pql/s…

21599d2

…ql parsing logic seamlessly

Make validation logics into utils

c67c98d

xiangfu0 force-pushed the upstream_master branch from 073177a to c67c98d Compare July 2, 2019 09:51

mayankshriv reviewed Jul 5, 2019

View reviewed changes

Address comments

6d4da58

mayankshriv approved these changes Jul 9, 2019

View reviewed changes

update calcite-core exclusions

fd8ec34

xiangfu0 merged commit a7c419a into apache:master Jul 10, 2019

xiangfu0 deleted the upstream_master branch July 10, 2019 20:43

mayankshriv mentioned this pull request Jul 15, 2019

Remove the massive console print and log from CalciteSqlCompilerTest #4434

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding Calcite SQL Parser and make another entry point to query Pinot #4387

Adding Calcite SQL Parser and make another entry point to query Pinot #4387

xiangfu0 commented Jun 30, 2019 •

edited

codecov-io commented Jun 30, 2019 •

edited

kishoreg left a comment

kishoreg Jul 1, 2019

xiangfu0 Jul 2, 2019

kishoreg Jul 1, 2019

xiangfu0 Jul 2, 2019

mayankshriv Jul 5, 2019

xiangfu0 Jul 9, 2019

mayankshriv Jul 5, 2019

xiangfu0 Jul 9, 2019

mayankshriv Jul 9, 2019

xiangfu0 Jul 10, 2019

mayankshriv Jul 5, 2019

xiangfu0 Jul 9, 2019

mayankshriv Jul 9, 2019 •

edited

xiangfu0 Jul 10, 2019

mayankshriv left a comment

Adding Calcite SQL Parser and make another entry point to query Pinot #4387

Adding Calcite SQL Parser and make another entry point to query Pinot #4387

Conversation

xiangfu0 commented Jun 30, 2019 • edited

codecov-io commented Jun 30, 2019 • edited

Codecov Report

kishoreg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankshriv Jul 9, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mayankshriv left a comment

Choose a reason for hiding this comment

xiangfu0 commented Jun 30, 2019 •

edited

codecov-io commented Jun 30, 2019 •

edited

mayankshriv Jul 9, 2019 •

edited