Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add antlr4-runtime-cpp installation in setup scripts #7449

Closed
wants to merge 1 commit into from

Conversation

duanmeng
Copy link
Collaborator

@duanmeng duanmeng commented Nov 7, 2023

As discussed in #7417 PrestoDB has a parseTypeSignature API,
which can parse a type string to a Velox type. However, we do
not have this kind of API in Velox. To add this parser, we need
to have antlr4-cpp-runtime first. Use the same way in PrestoDB
to add antlr4 through setup scripts, such as function install_antlr4
in https://github.com/prestodb/presto/blob/master/presto-native-execution/scripts/setup-ubuntu.sh#L48

Copy link

netlify bot commented Nov 7, 2023

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit bea6a2a
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/654a65b82685dd0008054b7b

@facebook-github-bot 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 7, 2023
@majetideepak
Copy link
Collaborator

We should avoid adding a dependency to just parse types. In the past, there was a discussion to use flex/bison to parse types and remove the dependency on antlr4 runtime.
I created an issue here prestodb/presto#21334.
We can implement this in Velox so that Prestissimo can use it as well.

@majetideepak
Copy link
Collaborator

I can help quickly implement this. Please let me know.

@duanmeng
Copy link
Collaborator Author

duanmeng commented Nov 8, 2023

I can help quickly implement this. Please let me know.

@majetideepak Awesome! Thanks for your help. Parse type should be the last blocker of adding PrestoQueryRunner in Velox.

@assignUser
Copy link
Collaborator

+1 on using the parser generator we already have as a core dependency over adding another dependency 🚀

@majetideepak
Copy link
Collaborator

I started a discussion here #7502 to extend the HiveTypeParser to support Presto Types as well. We don't need a new Flex/Bison implementation if we can support this.

@duanmeng duanmeng closed this Dec 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants