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

clp-s: Add boilerplate for new sql parser #504

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
b41eed1
Add boilerplate for new sql parser
gibber9809 Jul 31, 2024
86acbf5
Fix lint
gibber9809 Jul 31, 2024
93cb37c
Apply suggestions from code review
gibber9809 Aug 2, 2024
b5f55d3
Merge branch 'main' into basic-sql
gibber9809 Aug 2, 2024
d96505e
Move antlr visitor derived classes into private namespace and dedupli…
gibber9809 Aug 9, 2024
436c396
Apply suggestions from code review
gibber9809 Aug 12, 2024
4037d5b
Remove duplicated code
gibber9809 Aug 12, 2024
2b5db82
Merge remote-tracking branch 'upstream/main' into basic-sql
gibber9809 Sep 16, 2024
f8d67fb
Fix clang-tidy warnings
gibber9809 Sep 16, 2024
ce70d88
Undo incorrect clang-tidy suggestion
gibber9809 Sep 16, 2024
0f768a5
More clang-tidy fixes
gibber9809 Sep 16, 2024
a1d8fe7
Fix compilation error
gibber9809 Sep 22, 2024
1805505
Apply suggestions from code review
gibber9809 Sep 25, 2024
4af7f86
Lint fix
gibber9809 Sep 25, 2024
d09e6c4
Fix macOS build
gibber9809 Sep 25, 2024
991ca73
Apply suggestions from code review
gibber9809 Oct 22, 2024
ce13759
Update components/core/src/clp_s/search/sql/sql.cpp
gibber9809 Oct 22, 2024
5696ac3
Address more review comments
gibber9809 Oct 22, 2024
712afbe
Add comment indicating sql grammar is incomplete boilerplate
gibber9809 Oct 22, 2024
c2432bb
Minor fix
gibber9809 Oct 23, 2024
cd7f164
Update components/core/src/clp_s/search/sql/sql.cpp
gibber9809 Oct 23, 2024
d687fe4
Merge remote-tracking branch 'upstream/main' into basic-sql
gibber9809 Nov 7, 2024
8f00de2
Merge remote-tracking branch 'upstream/main' into basic-sql
gibber9809 Nov 13, 2024
29f8744
Remove spdlog dependency to fix build issue on macOS
gibber9809 Nov 18, 2024
f0118f1
Revert "Remove spdlog dependency to fix build issue on macOS"
gibber9809 Nov 18, 2024
866dcfc
Attempt to fix macOS build issues
gibber9809 Nov 18, 2024
e17ebd0
Merge remote-tracking branch 'upstream/main' into basic-sql
gibber9809 Nov 18, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions components/core/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,7 @@ set(SOURCE_FILES_unitTest
tests/test-Stopwatch.cpp
tests/test-StreamingCompression.cpp
tests/test-string_utils.cpp
tests/test-sql.cpp
tests/test-TimestampPattern.cpp
tests/test-utf8_utils.cpp
tests/test-Utils.cpp
Expand All @@ -513,6 +514,7 @@ target_link_libraries(unitTest
LibArchive::LibArchive
MariaDBClient::MariaDBClient
spdlog::spdlog
sql
OpenSSL::Crypto
${sqlite_LIBRARY_DEPENDENCIES}
${STD_FS_LIBS}
Expand Down
1 change: 1 addition & 0 deletions components/core/src/clp_s/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
add_subdirectory(search/kql)
add_subdirectory(search/sql)

set(
CLP_SOURCES
Expand Down
28 changes: 28 additions & 0 deletions components/core/src/clp_s/search/sql/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
ANTLR_TARGET(
SqlParser
Sql.g4
LEXER PARSER VISITOR
PACKAGE sql
)

add_library(
sql
../../Utils.hpp
../AndExpr.hpp
../BooleanLiteral.hpp
../ColumnDescriptor.hpp
../DateLiteral.hpp
../EmptyExpr.hpp
../Expression.hpp
../FilterExpr.hpp
../Integral.hpp
../NullLiteral.hpp
../OrExpr.hpp
../StringLiteral.hpp
${ANTLR_SqlParser_CXX_OUTPUTS}
sql.cpp
sql.hpp
)
target_compile_features(sql PRIVATE cxx_std_20)
target_include_directories(sql PRIVATE ${ANTLR_SqlParser_OUTPUT_DIR})
target_link_libraries(sql PRIVATE antlr4_static Boost::filesystem)
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
5 changes: 5 additions & 0 deletions components/core/src/clp_s/search/sql/Sql.g4
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Fwiw, the Rabbit agrees with me, lol.

Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
grammar Sql;

start: EOF ;

SPACE: [ \t\r\n] -> skip ;
71 changes: 71 additions & 0 deletions components/core/src/clp_s/search/sql/sql.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
#include <any>
#include <iostream>
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
#include <string>

#include <antlr4-runtime.h>
#include <spdlog/spdlog.h>

#include "../EmptyExpr.hpp"
#include "SqlBaseVisitor.h"
#include "SqlLexer.h"
#include "SqlParser.h"

using namespace antlr4;
using namespace sql;

namespace clp_s::search::sql {
class ErrorListener : public BaseErrorListener {
Copy link
Member

Choose a reason for hiding this comment

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

yeah it seems like we already have this class in clp_s::search::kql

Copy link
Member

Choose a reason for hiding this comment

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

Should be removed right?

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(); }
};
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


std::shared_ptr<Expression> parse_sql_expression(std::istream& in) {
ErrorListener lexer_error_listener;
ErrorListener parser_error_listener;

ANTLRInputStream input(in);
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
SqlLexer lexer(&input);
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
lexer.removeErrorListeners();
lexer.addErrorListener(&lexer_error_listener);
CommonTokenStream tokens(&lexer);
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
SqlParser parser(&tokens);
parser.removeErrorListeners();
parser.addErrorListener(&parser_error_listener);
SqlParser::StartContext* tree = parser.start();
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved

if (lexer_error_listener.error()) {
SPDLOG_ERROR("Lexer error: {}", lexer_error_listener.message());
return {};
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
}
if (parser_error_listener.error()) {
SPDLOG_ERROR("Parser error: {}", parser_error_listener.message());
return {};
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
}

ParseTreeVisitor visitor;
return std::any_cast<std::shared_ptr<Expression>>(visitor.visitStart(tree));
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
}
} // namespace clp_s::search::sql
17 changes: 17 additions & 0 deletions components/core/src/clp_s/search/sql/sql.hpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#ifndef CLP_S_SEARCH_SQL_SQL_HPP
#define CLP_S_SEARCH_SQL_SQL_HPP

#include <istream>

#include "../Expression.hpp"

namespace clp_s::search::sql {
/**
* Parses an SQL expression from the given stream to generate a search AST.
* @param in Input stream containing an SQL expression followed by EOF
* @return a search AST on success, nullptr otherwise
*/
std::shared_ptr<Expression> parse_sql_expression(std::istream& in);
} // namespace clp_s::search::sql

#endif // CLP_S_SEARCH_SQL_SQL_HPP
23 changes: 23 additions & 0 deletions components/core/tests/test-sql.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#include <sstream>
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved

#include <Catch2/single_include/catch2/catch.hpp>
#include <spdlog/spdlog.h>
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved

#include "../src/clp_s/search/EmptyExpr.hpp"
#include "../src/clp_s/search/sql/sql.hpp"
#include "LogSuppressor.hpp"

using clp_s::search::EmptyExpr;
using clp_s::search::sql::parse_sql_expression;
using std::stringstream;

TEST_CASE("Test parsing SQL", "[SQL]") {
// Suppress logging
LogSuppressor suppressor{};
Copy link
Member

Choose a reason for hiding this comment

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

Not used?

Copy link
Contributor Author

@gibber9809 gibber9809 Sep 25, 2024

Choose a reason for hiding this comment

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

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.

gibber9809 marked this conversation as resolved.
Show resolved Hide resolved

SECTION("Stub accepts empty string") {
stringstream empty_string{""};
auto filter = std::dynamic_pointer_cast<EmptyExpr>(parse_sql_expression(empty_string));
REQUIRE(nullptr != filter);
gibber9809 marked this conversation as resolved.
Show resolved Hide resolved
}
}
Loading