Add TypeSignature Parser using Flex and Bison by majetideepak · Pull Request #7590 · facebookincubator/velox

majetideepak · 2023-11-15T16:18:11Z

No description provided.

netlify · 2023-11-15T16:18:18Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`376b1c0`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65720329b1810400086be50f

majetideepak · 2023-11-15T22:58:02Z

This PR works on MacOS. I will fix the build problem on Ubuntu after #7568 lands.

mbasmanova

@majetideepak Deepak, this parser is specific to Presto, right? It can parse type strings generated by Presto and supports Presto-specific types like TIMESTAMP WITH TIME ZONE and JSON, right? If that's the case, perhaps, it is better placed in Presto-specific directory. Maybe somewhere under functions/prestosql.

CC: @pedroerp

majetideepak · 2023-11-16T23:05:31Z

@mbasmanova This PR is to parse TypeSignatures which is invoked when binding function signatures in Velox. This extends the Presto-specific parser you are referring to which is here #7568

majetideepak · 2023-11-16T23:07:10Z

Currently, only the flex file is re-used. But we could decouple both and move #7568 under functions/prestosql.

pedroerp · 2023-11-28T17:03:44Z

velox/expression/TypeSignature.cpp

+
+namespace facebook::velox::exec {
+
+void toAppend(


where is this used? should this be in an anonymous namespace?

This is required by folly::join. I moved this as is from FunctionSignature.*. I will try an anonymous namespace.

an anonymous namespace does not work here.

pedroerp · 2023-11-28T17:04:27Z

velox/expression/signature_parser/ParseUtil.cpp

+namespace facebook::velox::exec {
+
+TypeSignaturePtr inferTypeWithSpaces(
+    std::vector<std::string>& words,


pedroerp · 2023-11-28T17:04:58Z

velox/expression/signature_parser/ParseUtil.cpp

+    std::vector<std::string>& words,
+    bool cannotHaveFieldName) {
+  VELOX_CHECK_GE(words.size(), 2);
+  std::string fieldName = words[0];


const auto&

pedroerp · 2023-11-28T17:06:58Z

velox/expression/signature_parser/ParseUtil.h

+/// the remaining words are a Velox type.
+TypeSignaturePtr inferTypeWithSpaces(
+    std::vector<std::string>& words,
+    bool cannotHaveFieldName = true);


nit: can we flip the flag to canHaveFieldName instead? Makes it a little easier to understand that the double negative.

pedroerp · 2023-11-28T17:08:50Z

velox/expression/signature_parser/SignatureParser.ll

+
+#include "velox/expression/signature_parser/SignatureParser.h"
+
+facebook::velox::exec::TypeSignaturePtr facebook::velox::exec::parseTypeSignature(const std::string& signatureText)


Scanner takes a string_view. Could we also take a string_view here?

std::istringstream is(signatureText); below does not accept a string_view.

pedroerp · 2023-11-28T17:11:21Z

velox/expression/signature_parser/SignatureParser.yy

+
+%%
+
+type_spec : type                 { scanner->setTypeSignature($1); }


it's a bummer we need to repeat the type grammar here. Is there no way refactor and include that part?

Each rule returns a TypeSignature here vs. a TypePtr in the Type Parser.
To unify both, we have to make each rule return strings and finally return a parse tree of strings. Then each parser has to recurse the tree again to convert the string to a TypeSignature or TypePtr.
I am planning to use the TypeParser in Prestissimo to replace Antlr here.
I feel the double-pass approach would be inefficient.

I looked at this further.
flex/bison are not designed to reuse the grammar.
We could use a union/std::variant to encapsulate TypeSignature TypePtr into a single variable and make this work via virtual functions.
I started this and it turned out to be more complex. Some observations below:

The lex file (.ll) cannot be reused since it implements the parseType or parseSignature APIs. This cannot be moved out. This means only the Bison parser file (.yy) can be reused if at all.

There are some differences in the decimal grammar. eg. DECIMAL(a_precision, b_precision) is a valid function signature but not a valid type.

Row fields layout is different for TypeSignature vs RowType. This will introduce more customization using virtual functions.

The parsing grammar is not going to change in the future. So duplication of the Bison file should be okay.

Ok, thanks for looking into this!

pedroerp · 2023-11-28T17:12:21Z

velox/expression/signature_parser/tests/ParseTypeSignatureTest.cpp

+  ASSERT_EQ(signature.baseName(), "row");
+  ASSERT_EQ(signature.parameters().size(), 1);
+}
+


the missing feature from the existing code was to support quoted named row fields. Could you add a few tests to ensure that works now?

I added a test for this.

kgpai · 2023-11-28T18:17:22Z

FYI - The signature check job seems to be failing because 'DECIMAL' changed to 'decimal' and right now that is taken as a different type :) . I will make the signature check job type insensitive.

majetideepak · 2023-12-04T07:21:06Z

@kgpai thank you for looking. I resolved the case insensitive issue in my PR.

pedroerp

Thank you! Just a small last comment but looks like it's almost ready to go :)

pedroerp · 2023-12-05T21:05:19Z

velox/expression/signature_parser/ParseUtil.cpp

+  const auto& fieldName = words[0];
+  auto typeName = words[1];
+  for (int i = 2; i < words.size(); ++i) {
+    typeName = fmt::format("{} {}", typeName, words[i]);


I guess this will have quadratic time. Could we use folly::join() instead?

pedroerp · 2023-12-05T21:40:00Z

velox/expression/signature_parser/ParseUtil.cpp

+    typeName = fmt::format("{} {}", typeName, words[i]);
+  }
+  const auto allWords = fmt::format("{} {}", fieldName, typeName);
+  if (hasType(allWords) || !canHaveFieldName) {


maybe you just concatenate all of them, and use a std::string_view to skip the first token? (assuming hasToken() can take a string_view)

I did this for both TypeParser and SignatureParser. For TypeParser, we will need a substring if there is a field name.

pedroerp · 2023-12-05T21:41:04Z

velox/expression/signature_parser/SignatureParser.yy

+
+%%
+
+type_spec : type                 { scanner->setTypeSignature($1); }


Ok, thanks for looking into this!

facebook-github-bot · 2023-12-08T18:59:06Z

@pedroerp has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot · 2023-12-22T05:28:26Z

@pedroerp merged this pull request in 8948b9e.

conbench-facebook · 2023-12-22T06:13:31Z

Conbench analyzed the 1 benchmark run on commit 8948b9e9.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

Summary: Pull Request resolved: facebookincubator#7590 Reviewed By: kgpai Differential Revision: D51989804 Pulled By: pedroerp fbshipit-source-id: 82ac7a26fa8619ba13288c2fc15d70a6781c0911

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 15, 2023

majetideepak requested a review from pedroerp November 15, 2023 16:18

majetideepak force-pushed the signatureParser branch 2 times, most recently from e632be9 to 5dad298 Compare November 15, 2023 18:06

mbasmanova reviewed Nov 16, 2023

View reviewed changes

majetideepak force-pushed the signatureParser branch 2 times, most recently from addc80e to 3982990 Compare November 22, 2023 14:17

majetideepak marked this pull request as ready for review November 22, 2023 14:17

majetideepak marked this pull request as draft November 22, 2023 17:12

majetideepak force-pushed the signatureParser branch 2 times, most recently from 0871c5a to cf02cd4 Compare November 23, 2023 01:41

majetideepak marked this pull request as ready for review November 23, 2023 01:43

majetideepak force-pushed the signatureParser branch 2 times, most recently from 2e8eaa1 to 13eeea0 Compare November 24, 2023 09:53

pedroerp reviewed Nov 28, 2023

View reviewed changes

majetideepak force-pushed the signatureParser branch 2 times, most recently from 46e8f57 to d088ca0 Compare December 4, 2023 04:28

pedroerp reviewed Dec 5, 2023

View reviewed changes

majetideepak added 4 commits December 7, 2023 22:32

Add TypeSignature Parser using Flex and Bison

d6169e0

address review comments

cf4e355

fix new line

b822a24

address review comments

376b1c0

majetideepak force-pushed the signatureParser branch from ab5ef3c to 376b1c0 Compare December 7, 2023 17:38

kgpai approved these changes Dec 8, 2023

View reviewed changes

facebook-github-bot closed this in 8948b9e Dec 22, 2023

facebook-github-bot added the Merged label Dec 22, 2023

majetideepak deleted the signatureParser branch January 3, 2024 13:56


		#include "velox/expression/signature_parser/SignatureParser.h"

		facebook::velox::exec::TypeSignaturePtr facebook::velox::exec::parseTypeSignature(const std::string& signatureText)

Conversation

majetideepak commented Nov 15, 2023

Uh oh!

netlify bot commented Nov 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Deploy Preview for meta-velox canceled.

Uh oh!

majetideepak commented Nov 15, 2023

Uh oh!

mbasmanova left a comment

Choose a reason for hiding this comment

Uh oh!

majetideepak commented Nov 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

majetideepak commented Nov 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak Nov 29, 2023 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

majetideepak Nov 29, 2023 • 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!

kgpai commented Nov 28, 2023

Uh oh!

majetideepak commented Dec 4, 2023

Uh oh!

pedroerp left a comment

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!

facebook-github-bot commented Dec 8, 2023

Uh oh!

facebook-github-bot commented Dec 22, 2023

Uh oh!

conbench-facebook bot commented Dec 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

netlify bot commented Nov 15, 2023 •

edited

Loading

majetideepak commented Nov 16, 2023 •

edited

Loading

majetideepak Nov 29, 2023 •

edited

Loading

majetideepak Nov 29, 2023 •

edited

Loading