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

HIVE-28226: Split Replication Commands from HiveParser.g. #5220

Merged
merged 1 commit into from Apr 29, 2024

Conversation

ayushtkn
Copy link
Member

What changes were proposed in this pull request?

Refactor HiveParser to split Replication Commands

Why are the changes needed?

Reduce HiveParser size

Does this PR introduce any user-facing change?

No

Is the change a dependency upgrade?

No

How was this patch tested?

Existing Tests

Copy link

sonarcloud bot commented Apr 27, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

See the License for the specific language governing permissions and
limitations under the License.
*/
parser grammar ReplClauseParser;
Copy link
Contributor

Choose a reason for hiding this comment

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

Will we consider using Antlr4 in the future? IMO, most of OSS use Antlr4, and only Hive uses Antlr3, and Antlr has not been updated for many years & Antlr4 is more easy to use than Antlr3.
It would be great if we could incrementally update HiveParser.g it to a higher version HiveParser.g4. :)

A related uncompleted PR is #4058.

This comment may go beyond the scope of the PR. We can make this change happen in the future.

Copy link
Member Author

@ayushtkn ayushtkn Apr 28, 2024

Choose a reason for hiding this comment

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

Yahh, there are some efforts going on for Antlr4, but that might take some time. But we should definitely give it a shot, will explore how things can move there

this change anyway shouldn’t impact that upgrade efforts I believe, this one is just a basic refactor :-)

Copy link
Contributor

@zhangbutao zhangbutao left a comment

Choose a reason for hiding this comment

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

+1 LGTM
This can make HiveParser.g more cleaner & readable.

@ayushtkn ayushtkn merged commit dea968f into apache:master Apr 29, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants