clp-s: Add boilerplate for new sql parser #504
Conversation
Co-authored-by: Lin Zhihao <[email protected]>
| class ErrorListener : public BaseErrorListener { | ||
| public: | ||
| void syntaxError( | ||
| Recognizer* recognizer, | ||
| Token* offending_symbol, | ||
| size_t line, | ||
| size_t char_position_in_line, | ||
| std::string const& msg, | ||
| std::exception_ptr e | ||
| ) override { | ||
| m_error = true; | ||
| m_error_message = msg; | ||
| } | ||
|
|
||
| bool error() const { return m_error; } | ||
|
|
||
| std::string const& message() const { return m_error_message; } | ||
|
|
||
| private: | ||
| bool m_error{false}; | ||
| std::string m_error_message; | ||
| }; | ||
|
|
||
| class ParseTreeVisitor : public SqlBaseVisitor { | ||
| public: | ||
| std::any visitStart(SqlParser::StartContext* ctx) override { return EmptyExpr::create(); } | ||
| }; |
There was a problem hiding this comment.
Sorry to miss this in the last round of review. Shall we move these two classes into the anon namespace? Or if there's plan to reuse them in other places, we should probably move them into the dedicated headers.
There was a problem hiding this comment.
I'll move the ErrorListener into its own header, and put the visitor classes for sql and kql into the anonymous namespace.
I was worried that BaseErrorListener was part of the generated code for each grammar but it's part of the generic antlr runtime so there shouldn't be any issues.
| using namespace sql; | ||
|
|
||
| namespace clp_s::search::sql { | ||
| class ErrorListener : public BaseErrorListener { |
There was a problem hiding this comment.
yeah it seems like we already have this class in clp_s::search::kql
…cate ErrorListener
| using clp_s::search::antlr_common::ErrorListener; | ||
|
|
||
| namespace clp_s::search::sql { | ||
| class ErrorListener : public BaseErrorListener { |
There was a problem hiding this comment.
I will probably leave the change of kql to the next PR. Maybe ask Kirk's advice @kirkrodrigues
components/core/src/clp_s/search/antlr_common/ErrorListener.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp_s/search/antlr_common/ErrorListener.hpp
Outdated
Show resolved
Hide resolved
Co-authored-by: Lin Zhihao <[email protected]>
|
@LinZhihao-723 fixed clang-tidy warnings. Re-review? |
components/core/src/clp_s/search/antlr_common/ErrorListener.hpp
Outdated
Show resolved
Hide resolved
components/core/src/clp_s/search/antlr_common/ErrorListener.hpp
Outdated
Show resolved
Hide resolved
components/core/tests/test-sql.cpp
Outdated
| // Suppress logging | ||
| LogSuppressor suppressor{}; |
There was a problem hiding this comment.
Its use is entirely implemented in the constructor/destructor. It just turns off spdlog logging once created, and re-enables it once destroyed.
Just allows you to run parsing unit tests without emitting tons of log messages for parsing failure.
Co-authored-by: Lin Zhihao <[email protected]>
WalkthroughThe changes introduce a new SQL parsing feature into the project, which includes the addition of a SQL grammar definition and an implementation of a SQL expression parser. A new error listener class is created for error handling during parsing. Unit tests for the SQL parser are also added to ensure its functionality. The modifications to CMake configuration files accommodate the new SQL components, and updates to existing files integrate the new functionality. Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (7)
components/core/src/clp_s/search/sql/Sql.g4 (2)
3-3: Consider adding a comment to explain the placeholder 'start' rule.The current 'start' rule only matches EOF, which appears to be a placeholder. To improve clarity for future development, consider adding a comment explaining that this is a initial setup and will be expanded in future iterations.
Here's a suggested modification:
-start: EOF ; +// TODO: Expand this rule to parse SQL statements +start: EOF ;
1-5: Consider enhancing the grammar file with additional documentation and structure.While this file provides a good starting point for the SQL grammar, consider the following suggestions to improve its structure and documentation:
- Add a file-level comment explaining the purpose and scope of this grammar.
- Include a TODO list or roadmap comment outlining the planned SQL constructs to be added.
- Consider adding placeholder rules for major SQL components (e.g., SELECT, FROM, WHERE) with TODO comments.
These additions would provide more context for future development and make the file more self-documenting.
components/core/tests/test-sql.cpp (2)
15-24: Test case implementation looks good. Consider adding more test cases.The test case for parsing an empty string is well-implemented and serves as a good starting point. The use of
std::dynamic_pointer_castis appropriate for type checking in this context.To improve test coverage, consider adding more test cases for:
- Valid SQL expressions
- Invalid SQL syntax
- Edge cases (e.g., very long expressions, expressions with special characters)
- Different types of SQL expressions (e.g., SELECT, INSERT, UPDATE)
This will help ensure the robustness of the SQL parser implementation.
16-17: Consider addressing the static analysis hint for LogSuppressor.While not directly related to this file, the static analysis tool (cppcheck) suggests using an initialization list for 'm_previous_logging_level' in the LogSuppressor class.
In the file where LogSuppressor is implemented, consider updating the constructor to use an initialization list:
LogSuppressor::LogSuppressor() : m_previous_logging_level(spdlog::get_level()) { spdlog::set_level(spdlog::level::off); }This change can potentially improve performance slightly and follows C++ best practices.
components/core/src/clp_s/CMakeLists.txt (1)
1-2: LGTM! Consider grouping related subdirectories.The addition of the
search/sqlsubdirectory aligns well with the PR objectives of introducing a new SQL parser. This change complements the existingsearch/kqlsubdirectory and is consistent with the overall structure of the project.For improved readability and organization, consider grouping related subdirectories together. You could apply this diff:
-add_subdirectory(search/kql) -add_subdirectory(search/sql) +# Search-related subdirectories +add_subdirectory(search/kql) +add_subdirectory(search/sql)This minor change would make it easier to identify and manage search-related components in the future.
components/core/src/clp_s/search/kql/kql.cpp (2)
10-10: LGTM! Consider grouping related includes.The addition of the
ErrorListener.hppinclude and the reordering of KQL-related includes are appropriate. To further improve readability, consider grouping related includes together. For example, you could move the KQL-related includes (KqlBaseVisitor.h, KqlLexer.h, KqlParser.h) next to the new ErrorListener.hpp include.Also applies to: 20-22
204-204: LGTM! Consider improving error message formatting.The removal of the custom
ErrorListenerclass and the use ofantlr_common::ErrorListeneris a good improvement. It likely reduces code duplication and improves consistency across the project. The error handling logic inparse_kql_expressionremains robust.A minor suggestion: Consider using a more structured format for error messages, such as including the error type in the message. For example:
SPDLOG_ERROR("[LEXER_ERROR] {}", lexer_error_listener.message()); SPDLOG_ERROR("[PARSER_ERROR] {}", parser_error_listener.message());This could make log parsing and error identification easier in the future.
Also applies to: 207-231
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
- components/core/CMakeLists.txt (2 hunks)
- components/core/src/clp_s/CMakeLists.txt (1 hunks)
- components/core/src/clp_s/search/antlr_common/ErrorListener.hpp (1 hunks)
- components/core/src/clp_s/search/kql/CMakeLists.txt (2 hunks)
- components/core/src/clp_s/search/kql/kql.cpp (3 hunks)
- components/core/src/clp_s/search/sql/CMakeLists.txt (1 hunks)
- components/core/src/clp_s/search/sql/Sql.g4 (1 hunks)
- components/core/src/clp_s/search/sql/sql.cpp (1 hunks)
- components/core/src/clp_s/search/sql/sql.hpp (1 hunks)
- components/core/tests/test-sql.cpp (1 hunks)
🧰 Additional context used
cppcheck
components/core/tests/test-sql.cpp
[performance] 7-7: Variable 'm_previous_logging_level' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments not posted (19)
components/core/src/clp_s/search/sql/Sql.g4 (2)
1-1: LGTM: Grammar declaration is appropriate.The grammar name 'Sql' is suitable for its intended purpose of SQL parsing.
5-5: LGTM: 'SPACE' token definition is correct and appropriate.The 'SPACE' token correctly defines all common whitespace characters (space, tab, carriage return, and newline) and is set to be skipped during parsing. This is a standard and appropriate way to handle whitespace in most parsing scenarios.
components/core/src/clp_s/search/sql/sql.hpp (4)
1-8: LGTM: Header guards and includes are appropriate.The header guards and include statements are correctly implemented and provide the necessary dependencies for the SQL parsing functionality.
9-9: LGTM: Namespace declaration is appropriate.The namespace
clp_s::search::sqlaligns well with the file structure and provides a clear scope for the SQL parsing functionality.
10-14: LGTM: Function documentation is clear and concise.The documentation for
parse_sql_expressionprovides a clear description of its purpose, input, and return value. The previously suggested changes have been incorporated successfully.
15-15: LGTM: Function declaration follows modern C++ practices.The declaration of
parse_sql_expressionis well-structured:
- The
[[nodiscard]]attribute encourages proper error handling.- The use of
autowith a trailing return type enhances readability.- Taking
std::istream&as input provides flexibility in parsing sources.- Returning
std::shared_ptr<Expression>allows for nullable returns and shared ownership.components/core/src/clp_s/search/kql/CMakeLists.txt (1)
11-11: LGTM: Addition of ErrorListener headerThe inclusion of the
ErrorListener.hppheader file is a good addition. It aligns with the PR objective of introducing a new SQL parser and suggests improved error handling capabilities for the parsing process.components/core/tests/test-sql.cpp (2)
1-13: LGTM! Include statements and using declarations are well-organized.The necessary headers have been included, and the using declarations are appropriate for the test case. The suggestion to add the header has been addressed.
🧰 Tools
cppcheck
[performance] 7-7: Variable 'm_previous_logging_level' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
16-17: LogSuppressor usage is appropriate for test execution.The LogSuppressor is correctly used to prevent excessive log output during test execution, as explained in the previous discussion. This is a good practice for maintaining clean test output.
components/core/src/clp_s/search/sql/CMakeLists.txt (2)
1-6: LGTM: ANTLR target configuration is well-structured.The ANTLR target configuration for SqlParser is correctly set up. It properly specifies the grammar file, generates the necessary components (lexer, parser, and visitor), and sets the package name. This configuration follows best practices for ANTLR integration in CMake.
27-29: LGTM: Compiler and linker settings are appropriate.The compiler and linker settings for the SQL library are well-configured:
- The use of C++20 features is excellent for modern C++ development.
- Including the ANTLR output directory is necessary for accessing the generated files.
- Linking against antlr4_static and spdlog::spdlog is appropriate for a parser library with logging capabilities.
It's good to see that the previous suggestion to remove Boost::filesystem has been addressed in this version.
components/core/src/clp_s/search/antlr_common/ErrorListener.hpp (5)
1-9: Include statements and guards look good!The include guards are properly formatted, and all necessary headers are included. The suggested
<string_view>header from the past review has been added, which is great for usingstd::string_viewin the class.
11-28: Class declaration and public methods are well-implemented!The ErrorListener class is correctly declared within the
clp_s::search::antlr_commonnamespace and inherits fromantlr4::BaseErrorListener. The public methods have been improved as per previous suggestions:
- The
syntaxErrormethod signature now uses[[maybe_unused]]attributes appropriately.- Both
error()andmessage()methods are marked with[[nodiscard]]and use the trailing return type syntax.- The
message()method now returns astd::string_view, which is more efficient for read-only access.These changes enhance the code's clarity and efficiency.
14-24: The syntaxError method implementation is correct and efficient.The method properly sets the error flag and stores the error message. The use of
[[maybe_unused]]for parameters not used in the current implementation is a good practice to prevent compiler warnings while leaving room for future expansion if needed.
30-32: Private member variables are well-defined.The private section correctly declares two member variables:
m_error: A boolean initialized tofalse, which is a good practice.m_error_message: A std::string to store the error message.These members appropriately support the class's functionality.
1-36: Excellent implementation of the ErrorListener class!This file presents a well-structured and efficient implementation of an error listener for ANTLR4-based parsing. Key points:
- Proper use of include guards and necessary headers.
- Clear namespace declaration.
- Effective use of modern C++ features like
[[nodiscard]],[[maybe_unused]], andstd::string_view.- Concise and correct implementation of the
syntaxErrormethod.- Appropriate use of private member variables for maintaining state.
All previous review comments have been addressed, resulting in a high-quality, maintainable piece of code. Great job on implementing this ErrorListener class!
components/core/src/clp_s/search/kql/kql.cpp (1)
26-26: LGTM! Good use of namespace alias.The addition of the using statement for
clp_s::search::antlr_common::ErrorListeneris appropriate and consistent with the changes made to error handling. It improves code readability by allowing direct use ofErrorListenerwithout the full namespace.components/core/CMakeLists.txt (2)
522-522: LGTM: New SQL test file addedThe addition of
tests/test-sql.cppto theSOURCE_FILES_unitTestlist is appropriate. This aligns with the PR objective of introducing a new SQL parser and follows good development practices by including unit tests for the new functionality.
543-543: LGTM: SQL library linked to unit testsThe addition of the
sqllibrary to thetarget_link_librariessection for theunitTesttarget is correct and necessary for the new SQL functionality.To ensure the SQL library is properly integrated, please run the following verification script:
There was a problem hiding this comment.
Can we add a comment explaining this is a boilerplate? Asked Kirk to take a brief view and his feedback is this file can be confusing to people new to it without a comment explaining it's a WIP config
There was a problem hiding this comment.
Fwiw, the Rabbit agrees with me, lol.
Co-authored-by: Lin Zhihao <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/core/tests/test-sql.cpp (2)
18-22: Consider simplifying the REQUIRE statement for better readability.The test logic for parsing an empty string looks good. However, the REQUIRE statement can be simplified for better readability.
Consider changing:
REQUIRE((nullptr != filter));to:
REQUIRE(filter != nullptr);This change improves readability while maintaining the same functionality.
1-23: Consider expanding test coverage in future iterations.The current test file provides a good starting point by testing the empty string scenario. However, to ensure robust SQL parsing functionality, it would be beneficial to add more test cases in the future. These could include:
- Parsing simple SQL queries
- Testing different SQL clauses (SELECT, WHERE, JOIN, etc.)
- Handling of various data types in SQL expressions
- Error cases and invalid SQL syntax
Expanding the test suite will help catch potential issues and ensure the reliability of the SQL parser as it evolves.
Would you like assistance in outlining additional test cases for future implementation?
🧰 Tools
🪛 cppcheck
[performance] 7-7: Variable 'm_previous_logging_level' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- components/core/src/clp_s/search/sql/sql.cpp (1 hunks)
- components/core/tests/test-sql.cpp (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/src/clp_s/search/sql/sql.cpp
🧰 Additional context used
🪛 cppcheck
components/core/tests/test-sql.cpp
[performance] 7-7: Variable 'm_previous_logging_level' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
🔇 Additional comments (2)
components/core/tests/test-sql.cpp (2)
1-12: LGTM! Include statements and using declarations are appropriate.The necessary headers are included, and the using declarations are correctly implemented. The suggestions from previous comments have been addressed appropriately.
🧰 Tools
🪛 cppcheck
[performance] 7-7: Variable 'm_previous_logging_level' is assigned in constructor body. Consider performing initialization in initialization list.
(useInitializationList)
14-17: Test case setup looks good.The test case is well-defined using the Catch2 framework, and the use of LogSuppressor is appropriate for suppressing unnecessary log output during testing.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
components/core/tests/LogSuppressor.hpp (2)
1-2: Consider using a more specific header guard name.The current header guard
TESTS_LOGSUPPRESSOR_HPPcould potentially clash with other test utilities. Consider prefixing it with the project or component name, e.g.,CORE_TESTS_LOGSUPPRESSOR_HPP.
Line range hint
6-26: Add thread safety documentation and improve exception safety.The class modifies global logger state which has thread safety implications. Consider the following improvements:
- Document thread safety requirements
- Mark the member variable as const after initialization
- Add noexcept to the destructor
Here's the suggested implementation:
/** * A class that suppresses logs so long as it's instantiated. + * + * Note: This class modifies global logger state and should be used with caution + * in multi-threaded contexts as it may affect logging from other threads. */ class LogSuppressor { public: LogSuppressor() { m_previous_logging_level = spdlog::default_logger()->level(); spdlog::default_logger()->set_level(spdlog::level::off); } LogSuppressor(LogSuppressor const& other) = default; LogSuppressor(LogSuppressor&& other) noexcept = default; LogSuppressor& operator=(LogSuppressor const& other) = default; LogSuppressor& operator=(LogSuppressor&& other) noexcept = default; - ~LogSuppressor() { spdlog::default_logger()->set_level(m_previous_logging_level); } + ~LogSuppressor() noexcept { spdlog::default_logger()->set_level(m_previous_logging_level); } private: - spdlog::level::level_enum m_previous_logging_level; + const spdlog::level::level_enum m_previous_logging_level; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- components/core/src/clp_s/search/sql/Sql.g4 (1 hunks)
- components/core/src/clp_s/search/sql/sql.cpp (1 hunks)
- components/core/tests/LogSuppressor.hpp (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/core/src/clp_s/search/sql/Sql.g4
🧰 Additional context used
🔇 Additional comments (4)
components/core/tests/LogSuppressor.hpp (1)
27-28: LGTM!The header guard closure is properly implemented with a descriptive comment.
components/core/src/clp_s/search/sql/sql.cpp (3)
1-2: Remove unused iostream includeThe iostream header is not used in this implementation.
47-54: LGTM: Consistent error handlingThe error handling implementation is well-structured with:
- Clear error messages using SPDLOG
- Consistent use of nullptr for error conditions
- Separate handling of lexer and parser errors
24-30: LGTM: Good boilerplate implementationThe visitor class provides a solid foundation for future SQL parsing implementation. The use of anonymous namespace and [[nodiscard]] attribute follows best practices.
Let's verify the EmptyExpr implementation:
✅ Verification successful
The previous ast-grep pattern might have failed due to syntax or formatting differences. Let's try to locate the EmptyExpr implementation using alternative search methods to verify its existence and implementation details.
Implementation verified and properly used throughout the codebase
The EmptyExpr class is well-implemented in
components/core/src/clp_s/search/EmptyExpr.hppwith:
- Proper inheritance from Expression base class
- Static factory method
create()that returnsstd::shared_ptr<Expression>- Appropriate usage throughout the codebase for representing empty/null expressions
- Consistent usage in transformation passes and SQL parsing
The ParseTreeVisitor's use of EmptyExpr::create() aligns with the established pattern in the codebase, making it a suitable foundation for the SQL parsing implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify EmptyExpr implementation ast-grep --pattern 'class EmptyExpr : public Expression { $$$ static auto create() -> std::shared_ptr<EmptyExpr> { $$$ } $$$ }'Length of output: 141
Script:
#!/bin/bash # Let's try to find EmptyExpr implementation with ripgrep rg -A 10 "class EmptyExpr" --type cpp # Also search for Expression base class rg -A 5 "class Expression" --type cpp # Search for any files containing EmptyExpr fd -e cpp -e hpp -x rg "EmptyExpr"Length of output: 4278
|
Can we merge main? The latest workflow failure should be resolved by #578. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/core/src/clp_s/search/kql/kql.cpp (2)
Line range hint
31-206: Consider enhancing code maintainability with these improvements, eh?The ParseTreeVisitor implementation is solid but could benefit from these refinements:
The string manipulation methods like
unquote_stringandunquote_date_stringcontain magic numbers. Consider extracting these as named constants, eh?The catch block in
parse_kql_expressioncatches all exceptions, which might mask specific issues. Consider handling specific exceptions separately.Example for string constants:
namespace { constexpr char kDatePrefix[] = "date("; constexpr size_t kDatePrefixLength = 5; constexpr size_t kDateSuffixLength = 1; // closing parenthesis static std::string unquote_date_string(std::string const& text) { return unquote_string(text.substr(kDatePrefixLength, text.size() - (kDatePrefixLength + kDateSuffixLength))); } }
Line range hint
209-238: Consider enhancing error handling robustness, eh?The current implementation returns an empty shared_ptr on errors, which might make it difficult for callers to distinguish between different error cases. Consider these improvements:
- Return a Result/Expected type or throw specific exceptions for different error cases
- Include more context in error messages (like line/column numbers)
- Add debug logging for successful parsing
Example approach:
struct ParseResult { std::shared_ptr<Expression> expression; std::string error_message; bool success() const { return expression != nullptr; } }; ParseResult parse_kql_expression(std::istream& in) { // ... existing setup code ... if (lexer_error_listener.error()) { return ParseResult{ nullptr, fmt::format("Lexer error at line {}: {}", lexer.getCurrentToken()->getLine(), lexer_error_listener.message()) }; } // ... rest of the implementation ... }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/src/clp_s/search/kql/kql.cpp(3 hunks)
🔇 Additional comments (1)
components/core/src/clp_s/search/kql/kql.cpp (1)
10-10: Consider deferring KQL changes to a separate PR, eh?
These changes to the KQL parser, while beneficial for standardizing error handling, appear to be outside the scope of the SQL parser implementation. As mentioned in the previous review, it might be better to handle these modifications in a dedicated PR.
Also applies to: 26-26
Description
This PR adds boilerplate for the new sql parser. This includes a stubbed out g4 grammar and visitor, as well as a basic unit test that tells us that the whole thing compiles and runs.
Validation performed
Summary by CodeRabbit
New Features
LogSuppressorclass to manage logging during tests.Bug Fixes