Repository navigation
refactor(clp-s): Add ANTLR-generated parsers to codebase to eliminate need for code dependencies at build time. #925
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
Changes from 28 commits
0a2f8bc
5b43788
19f1e80
19a6cc4
8c8a6f3
8ba8191
fc1f64c
cb707a7
a7072be
17e3800
6eda0ee
bd0abcd
d0daacf
ddb047c
b9f16ea
b2ec42f
9d121af
1210f2b
108f61d
f4742f2
b8b804e
7878b3b
f457bff
09faf7c
075ca5b
8b3191c
abcb91d
e88d914
da1523b
4f15581
7cec3fd
399803f
b7a0519
7507fec
4bb484e
cfd48c0
4a8ad65
0c3e5e1
5fee637
10baa2d
51bcf30
6760985
457254e
e30dc2b
7b27d63
0270ef2
c8277a0
4b83336
2980015
3ef0261
7727eed
b5b2909
6da4782
e9c6c36
9d9d91c
c493f54
9a30c0b
3bcf6fe
6b4f74e
616bb69
529787e
0f5cda6
660e2c5
b86cc0a
bcbd0fe
54b73cc
76fd933
37dbe46
b07a57a
9c741d0
8a9449e
b35a0c9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| name: "clp-s-antlr-generation" | ||
|
|
||
| on: | ||
| pull_request: | ||
| push: | ||
| schedule: | ||
| # Run daily at 00:15 UTC (the 15 is to avoid periods of high load) | ||
| - cron: "15 0 * * *" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need to run this workflow periodically, right? The dependencies related to this workflow are fixed, so unless something unexpected happens, nothing should change if we run the workflow at a different time. |
||
| workflow_dispatch: | ||
|
anlowee marked this conversation as resolved.
Resolved
|
||
|
|
||
| concurrency: | ||
| group: "${{github.workflow}}-${{github.ref}}" | ||
| # Cancel in-progress jobs for efficiency | ||
| cancel-in-progress: true | ||
|
|
||
| jobs: | ||
| antlr-generation-check: | ||
| name: "antlr-generation-check" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
||
| strategy: | ||
| matrix: | ||
| os: ["macos-latest", "ubuntu-latest"] | ||
| runs-on: "${{matrix.os}}" | ||
| steps: | ||
| - uses: "actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683" | ||
| with: | ||
| submodules: "recursive" | ||
|
|
||
|
anlowee marked this conversation as resolved.
Resolved
|
||
| - uses: "actions/setup-python@42375524e23c412d93fb67b49958b491fce71c38" | ||
| with: | ||
| python-version: "3.11" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need Python in this workflow? |
||
|
|
||
| - name: "Install task" | ||
| shell: "bash" | ||
| run: "npm install -g @go-task/cli" | ||
|
|
||
|
anlowee marked this conversation as resolved.
Resolved
|
||
| - if: "matrix.os == 'macos-latest'" | ||
| name: "Install coreutils (for md5sum)" | ||
| run: "brew install coreutils" | ||
|
anlowee marked this conversation as resolved.
Resolved
|
||
|
|
||
| - name: "Run clp-s-generate-antlr-check task" | ||
| shell: "bash" | ||
| run: "task clp-s-generate-antlr-check" | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,24 +1,33 @@ | ||
| ANTLR_TARGET( | ||
| KqlParser | ||
| Kql.g4 | ||
| LEXER PARSER VISITOR | ||
| PACKAGE kql | ||
| set(ANTLR_GENERATED_KQL_CODE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/antlr_generated) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about ANTLR_GENERATED_SOURCE_DIR (to be consistent with how CMake names source directories)? |
||
|
|
||
| set(ANTLR_GENERATED_KQL_CODE | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about |
||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlBaseVisitor.cpp | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlBaseVisitor.h | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlLexer.cpp | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlLexer.h | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlParser.cpp | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlParser.h | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlVisitor.cpp | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR}/KqlVisitor.h | ||
| ) | ||
|
|
||
| add_library( | ||
| kql | ||
| ../antlr_common/ErrorListener.hpp | ||
| ${ANTLR_KqlParser_CXX_OUTPUTS} | ||
| kql.cpp | ||
| kql.hpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/../antlr_common/ErrorListener.hpp | ||
| ${ANTLR_GENERATED_KQL_CODE} | ||
|
anlowee marked this conversation as resolved.
Resolved
|
||
| ${CMAKE_CURRENT_SOURCE_DIR}/kql.cpp | ||
| ${CMAKE_CURRENT_SOURCE_DIR}/kql.hpp | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do we absolutely need to use
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's sugested by coderabbit if I remember correctly.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And what was its reasoning?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess no. Because previously when I tried building Velox it told me these generated files cannot be found, so I just added them though it turned out this didn't fix that issue. Yea I can remove them, shall I?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, let's remove it to stay consistent with other CMakeLists.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Lets alphabetize these. |
||
| ) | ||
| target_compile_features(kql PRIVATE cxx_std_20) | ||
| target_include_directories(kql PRIVATE ${ANTLR_KqlParser_OUTPUT_DIR}) | ||
| target_include_directories(kql | ||
| PRIVATE | ||
| ${ANTLR_GENERATED_KQL_CODE_DIR} | ||
|
anlowee marked this conversation as resolved.
Resolved
|
||
| ) | ||
| target_link_libraries( | ||
| kql | ||
| PRIVATE | ||
| antlr4_static | ||
| clp::string_utils | ||
| clp_s::search::ast | ||
| clp_s::search::ast | ||
| spdlog::spdlog | ||
|
Comment on lines
30
to
34
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Link against the proper ANTLR runtime target 🤖 Prompt for AI Agents |
||
| ) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
|
|
||
| // Generated from clp_s/search/kql/Kql.g4 by ANTLR 4.13.1 | ||
|
|
||
|
|
||
| #include "KqlBaseVisitor.h" | ||
|
|
||
|
|
||
| using namespace clp_s::search::kql; | ||
|
|
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,71 @@ | ||
|
|
||
| // Generated from clp_s/search/kql/Kql.g4 by ANTLR 4.13.1 | ||
|
|
||
| #pragma once | ||
|
|
||
|
|
||
| #include "antlr4-runtime.h" | ||
| #include "KqlVisitor.h" | ||
|
|
||
|
|
||
| namespace clp_s::search::kql { | ||
|
|
||
| /** | ||
| * This class provides an empty implementation of KqlVisitor, which can be | ||
| * extended to create a visitor which only needs to handle a subset of the available methods. | ||
| */ | ||
| class KqlBaseVisitor : public KqlVisitor { | ||
| public: | ||
|
|
||
| virtual std::any visitStart(KqlParser::StartContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitExpr(KqlParser::ExprContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitNestedQuery(KqlParser::NestedQueryContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitNotQuery(KqlParser::NotQueryContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitSubQuery(KqlParser::SubQueryContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitOrAndQuery(KqlParser::OrAndQueryContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitExpression(KqlParser::ExpressionContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitColumn_range_expression(KqlParser::Column_range_expressionContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitColumn_value_expression(KqlParser::Column_value_expressionContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitColumn(KqlParser::ColumnContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitValue_expression(KqlParser::Value_expressionContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
| virtual std::any visitList_of_values(KqlParser::List_of_valuesContext *ctx) override { | ||
| return visitChildren(ctx); | ||
| } | ||
|
|
||
|
|
||
| }; | ||
|
|
||
| } // namespace clp_s::search::kql |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about
clp-s-generated-code-committed-check?