Add customizable parser module by walterddr · Pull Request #8484 · apache/pinot

walterddr · 2022-04-07T17:28:56Z

This PR added a customizable parser syntax extension.

Details:

created fmpp-maven-plugin that supports multi scope config template.
- supported overwrite config.fmpp over Calcite's default.
supported customizable syntax via extending parserImpls.ftl
incorporated BABEL syntax Pinot used
- need to reduce the amount of nonReservedKeywords but we kept the full list for backward compatibility
introduced a new SqlInsertFromTable node.

walterddr · 2022-04-07T17:29:24Z

pinot-common/src/main/codegen/includes/parserImpls.ftl

+/**
+ * INSERT INTO [db_name.]table_name
+ *   FROM [ FILE | ARCHIVE ] 'file_uri' [, [ FILE | ARCHIVE ] 'file_uri' ]
+ */


siddharthteotia · 2022-04-07T20:12:42Z

Don't we need to write new handlers (java code) for the new grammar ?

xiangfu0 · 2022-04-07T22:49:33Z

Don't we need to write new handlers (java code) for the new grammar ?

Yes, I'm on the java part.

We plan to add the query syntax to insert raw data files into pinot using the query console. See the examples here: #8465 (comment)

@walterddr is extending the Calcite dialect to support new syntax then we can hook this with parser logic to this new ad-hoc minion task endpoint to execute tasks.

walterddr · 2022-04-08T20:42:42Z

this is still in POC phase but we were able to incorporate a new syntax and a new SqlCall node during parsing phase.

caveat. no longer using BABEL parser factory. let's see how many tests failed b/c of this change

walterddr

updated PR:

Included a full list of BABEL overwrite so the parserImpls.ftl is quite large.
If we want to separately release the fmpp plugin I created: https://github.com/walterddr/fmpp-maven-plugin

codecov-commenter · 2022-04-09T16:29:19Z

Codecov Report

Merging #8484 (0c1ba30) into master (52e5a2c) will decrease coverage by 3.77%.
The diff coverage is 63.83%.

❗ Current head 0c1ba30 differs from pull request most recent head ae4d2ec. Consider uploading reports for the commit ae4d2ec to get more accurate results

@@             Coverage Diff              @@
##             master    #8484      +/-   ##
============================================
- Coverage     70.77%   67.00%   -3.78%     
+ Complexity     4281     4202      -79     
============================================
  Files          1664     1274     -390     
  Lines         87356    64076   -23280     
  Branches      13229    10053    -3176     
============================================
- Hits          61826    42931   -18895     
+ Misses        21234    18062    -3172     
+ Partials       4296     3083    -1213

Flag	Coverage Δ
integration1	`?`
integration2	`?`
unittests1	`67.00% <63.83%> (-0.04%)`	⬇️
unittests2	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...pache/pinot/common/config/provider/TableCache.java	`0.00% <0.00%> (-76.20%)`	⬇️
...inot/common/function/scalar/DateTimeFunctions.java	`95.23% <ø> (ø)`
.../apache/pinot/sql/parsers/parser/UnparseUtils.java	`0.00% <0.00%> (ø)`
.../org/apache/pinot/core/common/MinionConstants.java	`0.00% <ø> (ø)`
...in/java/org/apache/pinot/core/common/Operator.java	`0.00% <ø> (ø)`
...inot/core/data/manager/offline/DimensionTable.java	`90.90% <0.00%> (-9.10%)`	⬇️
...ata/manager/offline/DimensionTableDataManager.java	`84.61% <0.00%> (-1.66%)`	⬇️
...operator/AcquireReleaseColumnsSegmentOperator.java	`0.00% <ø> (ø)`
...he/pinot/core/operator/BitmapDocIdSetOperator.java	`71.42% <ø> (+3.24%)`	⬆️
...g/apache/pinot/core/operator/DocIdSetOperator.java	`96.29% <ø> (+3.43%)`	⬆️
... and 716 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 52e5a2c...ae4d2ec. Read the comment docs.

xiangfu0 · 2022-04-09T21:16:52Z

pinot-common/pom.xml

          </java>
        </configuration>
      </plugin>
+      <plugin>


need to sync the pom with pinot-common-jdk8 module

xiangfu0 · 2022-04-09T21:17:45Z

updated PR:

Included a full list of BABEL overwrite so the parserImpls.ftl is quite large.

If we want to separately release the fmpp plugin I created: https://github.com/walterddr/fmpp-maven-plugin

we can move fmpp to org: https://github.com/pinot-contrib ?

Jackie-Jiang

High level question: Do we introduce this custom parser to accelerate the parsing but keep the same preserved words as BABEL? Can we get the same performance if switching to DEFAULT? I feel it might not worth the maintenance overhead if it is just for some extra preserved words.
If it is a must have in order to support more custom query syntax support, then go for it

Jackie-Jiang · 2022-04-11T23:57:04Z

pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java

-            + "    ELSE 'The quantity is under 30'\n"
-            + "END AS QuantityText\n"
-            + "FROM OrderDetails");
+        "SELECT OrderID, Quantity,\n" + "CASE\n" + "    WHEN Quantity > 30 THEN 'The quantity is greater than 30'\n"


(format) Please configure the formatter to honor formatter comments (Code Style -> Formatter -> check Turn formatter on/off with markers in code comments)

xiangfu0 · 2022-04-12T00:30:38Z

High level question: Do we introduce this custom parser to accelerate the parsing but keep the same preserved words as BABEL? Can we get the same performance if switching to DEFAULT? I feel it might not worth the maintenance overhead if it is just for some extra preserved words. If it is a must have in order to support more custom query syntax support, then go for it

The main goal is to introduce new SQL syntax for INSERT INTO ...

walterddr · 2022-04-12T01:06:04Z

High level question: Do we introduce this custom parser to accelerate the parsing but keep the same preserved words as BABEL?

the goal is not accelerate parsing but to get precise control on our SQL syntax

to support customizable syntax this is required.
BABEL enabled lots of unnecessary non-reserved keyword that may or may not be intended (and so far we only tested 10~20ish such non-reserved keyword usages).

yes, we copied the whole list for backward compatibility but ideally speaking we should only kept the list of non-reserved keywords that we support in CalciteSqlCompilerTest:testReservedKeywords()

Can we get the same performance if switching to DEFAULT?

Switching to DEFAULT or MYSQL_5 standard is desired if possible (see my TODO comment), but we still need to customized the list of non-reserved keyword + maintain backward compatibility until we can warn users that certain reserved keywords (in BABEL) will be removed in the future (if we decided to do so)

I feel it might not worth the maintenance overhead if it is just for some extra preserved words.

there shouldn't be any additional maintenance overhead comparing with using BABEL. As the syntax codegen rarely changed. and it will remain functional unless we change calcite version (which will causes maintenance overhead anyway if use BABEL)

xiangfu0 · 2022-04-12T18:40:34Z

contrib/pinot-fmpp-maven-plugin/src/main/java/org/apache/pinot/fmpp/FMPPMojo.java

+    }
+    return report;
+  }
+}


nit: new line

xiangfu0 · 2022-04-12T21:46:18Z

pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java

-            + "    ELSE 'The quantity is under 30'\n"
-            + "END AS QuantityText\n"
-            + "FROM OrderDetails");
+        "SELECT OrderID, Quantity,\n" + "CASE\n" + "    WHEN Quantity > 30 THEN 'The quantity is greater than 30'\n"


xiangfu0 · 2022-04-13T09:04:49Z

pinot-common/src/test/java/org/apache/pinot/sql/parsers/CalciteSqlCompilerTest.java

+  @Test
+  public void testParserExtensionImpl() {
+    SqlNode sqlNode = testSqlWithCustomSqlParser("INSERT INTO db.tbl "
+        + "FROM FILE 'file:///tmp/file1', FILE 'file:///tmp/file2'");


maybe also add a test for INSERT with OPTIONS?

could you give an example? (i am not sure that's supported in my parser syntax)

if you are referring to the option regex. that is not part of the change and will be removed from the SQL string before entering the parser anyway

I'm referring to something like:

INSERT INTO "baseballStats" FROM FILE s3://<my-bucket>/baseballStats/rawdata/ OPTION(taskName=myTask-s3) OPTION(input.fs.className=org.apache.pinot.plugin.filesystem.S3PinotFS) OPTION(input.fs.prop.accessKey=<my-key>) OPTION(input.fs.prop.secretKey=<my-secret>) OPTION(input.fs.prop.region=us-west-2)

If it's already handled before then no problem from here.

siddharthteotia · 2022-04-13T16:01:39Z

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

So I think we are still using the BABEL conformance but removing the MYSQL lex analyzer ? There won't be any impact of this on existing queries that are running fine or are otherwise unsupported ?

Lex is different from conformance. Lex basically is equivalent to the line

sqlParser.switchTo(SqlAbstractParserImpl.LexicalState.DQID);

Understood. My concern was mainly around ifLex.MYSQL_ANSI will give same behavior as SqlAbstractParserImpl.LexicalState.DQID ?

yes they are. If you check the config impl behind the scene it uses the LEX enum to see a whole bunch of settings I did here as well

siddharthteotia · 2022-04-13T16:23:47Z

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

IIUC, the current code SqlParser sqlParser = SqlParser.create(expression, PARSER_CONFIG); will return SqlBabelParserImpl (created from SqlBabelParserImplFactory).

In this PR, I think we are going to use SqlParserImpl (SqlParserImplFactory) instead of SqlBabelParserImpl and that is going to give the same behavior as today because SqlConformance is still set to BABEL ?

partially, also because we copied the BABEL config.fmpp content over.

siddharthteotia · 2022-04-13T16:30:15Z

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

(nit) brief javadoc please

siddharthteotia · 2022-04-13T16:30:33Z

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

(nit) javadoc please ?

siddharthteotia · 2022-04-13T16:32:44Z

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

Are all cases covered by the new test in CalciteSqlCompilerTest.java ?

yes. (actually there's one only, no alternative/branch)

adding custome parser

5676c3c

walterddr commented Apr 7, 2022

View reviewed changes

Rong Rong added 3 commits April 8, 2022 08:52

adding stuff to use custom parser

c4e9b1b

adding SqlInsertFromFile SqlCall node. make E2E compilation work

acdb1ce

fix license

723b1c1

fix compilation and test

355e25b

walterddr commented Apr 9, 2022

View reviewed changes

xiangfu0 reviewed Apr 9, 2022

View reviewed changes

adding in presto driver pom change as well

f019ec9

xiangfu0 requested review from Jackie-Jiang, siddharthteotia and xiangfu0 April 10, 2022 00:06

walterddr changed the title ~~adding custome parser~~ adding customized parser Apr 10, 2022

walterddr changed the title ~~adding customized parser~~ Add customizable parser module Apr 10, 2022

Rong Rong added 3 commits April 11, 2022 07:44

also add customizable parser test

355c8dd

remove the non-used FILE token

e151634

adding TODO for next steps

5e1ff16

walterddr force-pushed the add_custom_parser branch from 531186a to d61e541 Compare April 11, 2022 21:31

move fmpp maven plugin to contrib

4ee7965

walterddr force-pushed the add_custom_parser branch from d61e541 to 4ee7965 Compare April 11, 2022 21:36

Jackie-Jiang reviewed Apr 12, 2022

View reviewed changes

walterddr marked this pull request as ready for review April 12, 2022 00:27

xiangfu0 approved these changes Apr 12, 2022

View reviewed changes

xiangfu0 reviewed Apr 13, 2022

View reviewed changes

siddharthteotia reviewed Apr 13, 2022

View reviewed changes

fix format

4df0f48

walterddr force-pushed the add_custom_parser branch from 2bd4a15 to 4df0f48 Compare April 13, 2022 17:04

add javadoc

ae4d2ec

siddharthteotia approved these changes Apr 13, 2022

View reviewed changes

xiangfu0 merged commit cb8bcc4 into apache:master Apr 13, 2022

walterddr deleted the add_custom_parser branch December 6, 2023 16:15

Conversation

walterddr commented Apr 7, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia commented Apr 7, 2022

Uh oh!

xiangfu0 commented Apr 7, 2022

Uh oh!

walterddr commented Apr 8, 2022

Uh oh!

walterddr left a comment

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Apr 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangfu0 commented Apr 9, 2022

Uh oh!

Jackie-Jiang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xiangfu0 commented Apr 12, 2022

Uh oh!

walterddr commented Apr 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

siddharthteotia Apr 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

walterddr commented Apr 7, 2022 •

edited

Loading

codecov-commenter commented Apr 9, 2022 •

edited

Loading

Jackie-Jiang left a comment •

edited

Loading

walterddr commented Apr 12, 2022 •

edited

Loading

siddharthteotia Apr 13, 2022 •

edited

Loading

siddharthteotia Apr 13, 2022 •

edited

Loading