Add Presto Type Parser based on Flex and Bison #7568

majetideepak · 2023-11-14T21:38:56Z

A Presto Type Parser is needed to support PrestoQueryRunner.
This will eventually replace the Antlr implementation in Prestissimo and remove the dependency on Antlr.

netlify · 2023-11-14T21:39:01Z

✅ Deploy Preview for meta-velox canceled.

Name	Link
🔨 Latest commit	`26ecd6b`
🔍 Latest deploy log	https://app.netlify.com/sites/meta-velox/deploys/65595d677baaa5000896ac68

duanmeng

LTGM, Nice!

pedroerp · 2023-11-15T16:12:17Z

velox/type/CMakeLists.txt

@@ -16,6 +16,7 @@ if(${VELOX_BUILD_TESTING})
 endif()

 add_subdirectory(tz)
+add_subdirectory(type_parser)


nit: doesn't only "parser" here already implies this is related to types? velox/type/parser vs /velox/type/type_parser`?

pedroerp · 2023-11-15T16:13:13Z

velox/type/type_parser/CMakeLists.txt

+  add_subdirectory(tests)
+endif()
+
+bison_target(


does this mean we now have bison and flex as compile time dependencies? Does our CMake automatically install them if they are not found?

All the dependency management was done when we added the type calculation for Decimal Types
https://github.com/facebookincubator/velox/blob/main/velox/expression/type_calculation/CMakeLists.txt

pedroerp · 2023-11-15T18:18:00Z

velox/type/type_parser/Scanner.h

+      std::istream& arg_yyin,
+      std::ostream& arg_yyout,
+      TypePtr& outputType,
+      const std::string& input)


Wonder if this should operate over std::string_view to avoid all the copies below

pedroerp · 2023-11-15T18:18:37Z

velox/type/type_parser/Scanner.h

+
+ private:
+  TypePtr& outputType_;
+  std::string input_;


pedroerp · 2023-11-15T18:19:12Z

velox/type/type_parser/TypeParser.h

+#include "velox/type/Type.h"
+
+namespace facebook::velox {
+TypePtr parseType(const std::string& typeText);


can we add some documentation on this method? This file is problem where developers will look for more information about this API

pedroerp · 2023-11-15T18:19:53Z

velox/type/type_parser/TypeParser.h

+#include <string>
+#include "velox/type/Type.h"
+
+namespace facebook::velox {


nit: blank line after namespace definition

pedroerp · 2023-11-15T18:23:05Z

velox/type/type_parser/TypeParser.ll

+
+%option c++ noyywrap noyylineno nodefault caseless
+
+A   [A|a]


why do we need this? To upper case characters? Maybe add a comment

We need this to support handling the input in a case-insensitive manner.
This also enables use to define custom types such as TYPES_WITH_SPACES.
I followed the Antlr grammar for the most part to reduce risk.
https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/types/TypeSignature.g4#L86

pedroerp · 2023-11-15T18:25:02Z

velox/type/type_parser/TypeParser.ll

+NUMBER            ([[:digit:]]+)
+ROW               (ROW|STRUCT)
+VARIABLE          (VARCHAR|VARBINARY)
+TYPE_WITH_SPACES  ((DOUBLE[ ]PRECISION)|(TIME[ ]WITH[ ]TIME[ ]ZONE)|(TIMESTAMP[ ]WITH[ ]TIME[ ]ZONE)|(INTERVAL[ ]YEAR[ ]TO[ ]MONTH)|(INTERVAL[ ]DAY[ ]TO[ ]SECOND))


the tricky part here is that these types can be extended by users (some of these are Presto specific, for example). How can we make the parser extensible, based on the types that are registered?

also, for example, if "timestamp with timezone" was not registered, we probably want to fail the parsing.

The current implementation follows the antlr semantics. https://github.com/prestodb/presto/blob/master/presto-native-execution/presto_cpp/main/types/TypeParser.cpp#L58
The Presto types are manually handled and not based on the registration.
All other types are handled based on the registration. See TypePtr typeFromString(const std::string& type) inside TypeParser.yy

New types as long as they are scalar will be handled automatically. Parameterized types and types with spaces have to be handled manually since they have custom syntax.

pedroerp · 2023-11-16T01:03:54Z

velox/type/parser/CMakeLists.txt

+endif()
+
+bison_target(
+  TypeParserParser TypeParser.yy ${CMAKE_CURRENT_BINARY_DIR}/TypeParser.yy.cc


nit: can we call the target only "TypeParser"?

pedroerp · 2023-11-16T01:04:46Z

velox/type/parser/Scanner.h

+      std::istream& arg_yyin,
+      std::ostream& arg_yyout,
+      TypePtr& outputType,
+      const std::string& input)


you need to take a string_view here as well or you will end up copying to a temporary and acquiring a string_view to the temporary.

pedroerp · 2023-11-16T01:05:57Z

velox/type/parser/TypeParser.h

+/// otherwise.
+/// Uses the Type::hasType and Type::getType APIs to convert a string to Velox
+/// type.
+


nit: move blank line to between the function and namespace end

pedroerp · 2023-11-16T01:08:34Z

velox/type/parser/TypeParser.ll

+(DECIMAL)          return Parser::token::DECIMAL;
+{ROW}              return Parser::token::ROW;
+{VARIABLE}         yylval->build<std::string>(YYText()); return Parser::token::VARIABLE;
+{NUMBER}           yylval->build<long long>(strtoll(YYText(), nullptr, 10)); return Parser::token::NUMBER;


can we use folly::to here?

pedroerp · 2023-11-16T01:09:46Z

velox/type/parser/TypeParser.yy

+    using namespace facebook::velox;
+    TypePtr typeFromString(const std::string& type) {
+        auto upper = type;
+        std::transform(upper.begin(), upper.end(), upper.begin(), ::toupper);


I think folly has an in-place upper case method as well

I could not find one. Velox uses boost in other places.

It actually only support lower case:

https://github.com/facebook/folly/blob/main/folly/String.h#L742

never mind

pedroerp · 2023-11-16T01:13:01Z

velox/type/parser/TypeParser.yy

+    TypePtr typeFromString(const std::string& type) {
+        auto upper = type;
+        std::transform(upper.begin(), upper.end(), upper.begin(), ::toupper);
+        if (upper == "UNKNOWN") {


isn't this already covered in getType()? Can't we just rely on getType() since that already honors types that are and are not registered?

pedroerp · 2023-11-16T01:21:29Z

velox/type/parser/TypeParser.ll

+Y   [Y|y]
+Z   [Z|z]
+
+WORD              ([[:alpha:][:alnum:]_]*)


feels like the WORD, since it's used for the type names, will need to support whitespaces as well - even though this will make it ambiguous in some cases, but I remember there was a way to specify how these should be handled.

QUOTED_ID will support names with spaces. There is a test as well.

I think my point is that since WORD cannot have spaces, things like TIMESTAMP WITH TIMEZONE will not be parsed as a single WORD, and thus will need special flex/grammar support to understand that these three WORDs together form a type name. And I guess we can't update the grammar as more types are registered/unregistered. For example, if I register a new type named "PEDROs DATA TYPE", the parser won't be able to understand it.

This is ok for now, but we might need to think through it eventually.

Even Postgres has tokens for types with spaces. https://github.com/postgres/postgres/blob/master/src/backend/parser/gram.y#L14336
The issue with your proposal is how do we decide if TIMESTAMP WITH TIME ZONE VELOX is a new type vs. an error parsing TIMESTAMP WITH TIME ZONE?

The other ambiguity is if ROW(PEDROs DATA TYPE) is Row with field name PEDROs with type name DATA TYPE or a type PEDROs DATA TYPE without a field name.

What Presto Java does is to first check if there's a type that matches the full name ("PEDROs DATA TYPE"), and if not, assume the first token is the name and proceed to try to match the remaining as the type ("DATA TYPE" as the type name and "PEDROs" the field name). Could we mimic that logic here somehow?

I see. Yes we can. We can give precedence between two rules and achieve this. I will implement this in a follow up PR.

pedroerp · 2023-11-16T01:22:25Z

velox/type/parser/tests/TypeParserTest.cpp

+  ASSERT_EQ(*parseType("row(col iNt)"), *ROW({"col"}, {INTEGER()}));
+}
+
+TEST_F(TestTypeSignature, typesWithSpaces) {


I guess what is not covered today is if you add a new custom type which has blankspaces in the name.

I modified the implementation to remove the manual Prestosql types inference.
JSON and Timestamp with time zone are now registered in the test. Hyperloglog is not registered in the test.
These take the custom type path.

See my comment above. Ideally we would like to add a custom test which is not explicitly defined in the flex file (so it's extensible).

pedroerp · 2023-11-16T01:23:44Z

velox/type/parser/tests/TypeParserTest.cpp

+
+class TestTypeSignature : public ::testing::Test {};
+
+TEST_F(TestTypeSignature, booleanType) {


love the extensive test suite :)

I copied most from the Prestissimo parser test suite to be fair.

mbasmanova · 2023-11-17T13:44:10Z

velox/type/parser/tests/TypeParserTest.cpp

+
+#include "velox/common/base/tests/GTestUtils.h"
+#include "velox/functions/prestosql/types/HyperLogLogType.h"
+#include "velox/functions/prestosql/types/JsonType.h"


It doesn't seem right to have a dependency on prestosql here. CC: @pedroerp

@mbasmanova this is a test file. Can you share why it would be not right to depend on prestosql?

I see that even a dependency across modules for tests is not ideal. If we decide to keep the implementation here, I can define these types here again and remove the dependency.

+1. Seems like what we really want to test is whether the parser understands user defined types. Could we rather define a super tiny and simple type in this file, register, then test that we can parse it? Ideally, one without and one with blank spaces in the name

pedroerp

Thanks for making the changes. Let's get this one merged, but discuss offline is there's a way to make it more general to user defined type names in the future.

Thanks!

facebook-github-bot · 2023-11-20T21:28:09Z

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

facebook-github-bot · 2023-11-21T03:02:37Z

@pedroerp merged this pull request in d7e8348.

conbench-facebook · 2023-11-21T03:28:27Z

Conbench analyzed the 1 benchmark run on commit d7e83486.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details.

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 14, 2023

mbasmanova requested review from duanmeng and pedroerp November 14, 2023 21:39

majetideepak force-pushed the typeparser branch 2 times, most recently from 78c2e6e to 6f11b6b Compare November 15, 2023 05:27

Add TypeParser based on Flex and Bison

260f66a

majetideepak force-pushed the typeparser branch from 6f11b6b to 260f66a Compare November 15, 2023 05:33

majetideepak marked this pull request as ready for review November 15, 2023 05:33

rename API to parseType to avoid conflict

35de81e

duanmeng approved these changes Nov 15, 2023

View reviewed changes

pedroerp reviewed Nov 15, 2023

View reviewed changes

majetideepak mentioned this pull request Nov 15, 2023

Add TypeSignature Parser using Flex and Bison #7590

Closed

majetideepak force-pushed the typeparser branch 2 times, most recently from 9768c44 to 674aab8 Compare November 15, 2023 23:12

address review comments

6c348dd

majetideepak force-pushed the typeparser branch from 674aab8 to 6c348dd Compare November 15, 2023 23:15

majetideepak mentioned this pull request Nov 15, 2023

[native] Add caching of parsed Types prestodb/presto#21325

Merged

pedroerp reviewed Nov 16, 2023

View reviewed changes

majetideepak force-pushed the typeparser branch from e5bd144 to c3910b4 Compare November 16, 2023 09:23

address review comments

9612498

majetideepak force-pushed the typeparser branch from c3910b4 to 9612498 Compare November 16, 2023 09:26

majetideepak changed the title ~~Add TypeParser based on Flex and Bison~~ Add Presto TypeParser based on Flex and Bison Nov 16, 2023

majetideepak changed the title ~~Add Presto TypeParser based on Flex and Bison~~ Add Presto Type Parser based on Flex and Bison Nov 16, 2023

duanmeng mentioned this pull request Nov 17, 2023

Add PrestoQueryRunner #7628

Closed

mbasmanova reviewed Nov 17, 2023

View reviewed changes

address review comments

26ecd6b

majetideepak force-pushed the typeparser branch from 3de4ccb to 26ecd6b Compare November 19, 2023 00:57

pedroerp approved these changes Nov 20, 2023

View reviewed changes

facebook-github-bot closed this in d7e8348 Nov 21, 2023

facebook-github-bot added the Merged label Nov 21, 2023

majetideepak deleted the typeparser branch January 3, 2024 14:03


		class TestTypeSignature : public ::testing::Test {};

		TEST_F(TestTypeSignature, booleanType) {

Add Presto Type Parser based on Flex and Bison #7568

Add Presto Type Parser based on Flex and Bison #7568

Conversation

majetideepak commented Nov 14, 2023 • edited Loading

netlify bot commented Nov 14, 2023 • edited Loading

✅ Deploy Preview for meta-velox canceled.

duanmeng 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

majetideepak Nov 15, 2023 • edited Loading

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

majetideepak Nov 18, 2023 • edited Loading

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

majetideepak Nov 16, 2023 • edited Loading

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

majetideepak Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedroerp left a comment

Choose a reason for hiding this comment

facebook-github-bot commented Nov 20, 2023

facebook-github-bot commented Nov 21, 2023

conbench-facebook bot commented Nov 21, 2023

majetideepak commented Nov 14, 2023 •

edited

Loading

netlify bot commented Nov 14, 2023 •

edited

Loading

majetideepak Nov 15, 2023 •

edited

Loading

majetideepak Nov 18, 2023 •

edited

Loading

majetideepak Nov 16, 2023 •

edited

Loading

majetideepak Nov 17, 2023 •

edited

Loading