Skip to content

[native] Use Velox::TypeParser instead of Antlr and remove Antlr#21357

Merged
majetideepak merged 1 commit intoprestodb:masterfrom
majetideepak:hiveparser
Dec 8, 2023
Merged

[native] Use Velox::TypeParser instead of Antlr and remove Antlr#21357
majetideepak merged 1 commit intoprestodb:masterfrom
majetideepak:hiveparser

Conversation

@majetideepak
Copy link
Collaborator

@majetideepak majetideepak commented Nov 9, 2023

Description

Use Velox::TypeParser instead of Antlr.
Remove Antlr dependency.

Motivation and Context

Resolves #21334

Contributor checklist

  • Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
  • PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
  • Documented new properties (with its default value), SQL syntax, functions, or other functionality.
  • If release notes are required, they follow the release notes guidelines.
  • Adequate tests were added if applicable.
  • CI passed.
== NO RELEASE NOTE ==

@majetideepak majetideepak changed the title Hiveparser Use Velox::HiveParser instead of Antlr Nov 9, 2023
@majetideepak majetideepak changed the title Use Velox::HiveParser instead of Antlr Use Velox::TypeParser instead of Antlr Nov 29, 2023
@majetideepak majetideepak marked this pull request as ready for review November 29, 2023 06:20
@majetideepak majetideepak requested a review from a team as a code owner November 29, 2023 06:20
@majetideepak
Copy link
Collaborator Author

@kevinwilfong can you share your feedback? Thanks.

@majetideepak majetideepak changed the title Use Velox::TypeParser instead of Antlr Use Velox::TypeParser instead of Antlr and remove Antlr Nov 29, 2023
@kevinwilfong
Copy link
Contributor

Thanks! This looks great, it makes it more consistent with the rest of the code, and gets rid of that annoying issue with ANTLR messing with preprocessor identifiers. Also, thank you for preserving the caching logic.

cc: @xiaoxmeng @mbasmanova for a review, the change looks good to me, but I'm not a code owner yet

@kevinwilfong
Copy link
Contributor

@majetideepak the broken linux-*-e2e-tests look like they're due to a parsing error

@majetideepak
Copy link
Collaborator Author

@majetideepak the broken linux-*-e2e-tests look like they're due to a parsing error

@kevinwilfong The parsing error was fixed in Velox. CI now passed with rebasing.

@majetideepak
Copy link
Collaborator Author

@mbasmanova, @xiaoxmeng can you also review? Thanks.

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@majetideepak Please, add [native] prefix to commit and PR.

Copy link
Contributor

@mbasmanova mbasmanova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very nice. Thank you, Deepak.

@majetideepak majetideepak changed the title Use Velox::TypeParser instead of Antlr and remove Antlr [native] Use Velox::TypeParser instead of Antlr and remove Antlr Dec 7, 2023
@majetideepak majetideepak merged commit e718f7a into prestodb:master Dec 8, 2023
@majetideepak majetideepak deleted the hiveparser branch February 2, 2024 16:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Native] Use Flex/Bison to parse types and remove dependency on antlr4 runtime

3 participants