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

feat: support parameterized query sdk #248

Merged

Conversation

jingchen2222
Copy link
Collaborator

@jingchen2222 jingchen2222 commented Aug 11, 2021

  • Please check if the PR fulfills these requirements
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

feature and bug fix.

  • What is the current behavior? (You can also link to an open issue here)
    SqlRouter support Execute Batch SQL with ParameterRow/Request Row #168

  • What is the new behavior (if this is a feature change)?

    • CICD: allow openmldb to use hybridse build from local 03cdb04
    • BUILD: fix: mini cluster core on mac. d7c5e6c
    • Hybridse: fix engine cache trigger by parameterized query. 2d2d6d2
    • Hybridse: refactor Session interface, only BatchRunSession can support parameterized run method.29e2680
    • Hybridse: remove session.Run() interface. 76542f7
    • C++ SDK: insert sdk bug fix for parameterized insert. d59f5dc
    • C++ SDK: support parameterized query. 0598932
    • TEST: fix:UninstantiatedParameterizedTestSuite error. 90dc3ae
  • Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)

    • C++ SDK: Method change. ExecuteSQL(db, sql,request) change to ExecuteRequestSQL(db, sql,request)
    • C++ SDK: Add Method. ExecuteSQL(db, sql, parameter_schema, parameter)

Demo

// ... ignore
    hybridse::codec::Schema parameter_schema;
    {
        auto column = parameter_schema.Add();
        column->set_type(::hybridse::type::kVarchar);
    }
    {
        auto column = parameter_schema.Add();
        column->set_type(::hybridse::type::kInt64);
    }
    const std::shared_ptr<::hybridse::sdk::Schema> schema_impl = std::make_shared<hybridse::sdk::SchemaImpl>(schema);
    std::string where_exist = "select * from trans where merch_id = ? and txn_time < ?;";
    // parameterized query
    auto parameter_row = std::make_shared<SQLRequestRow>(schema_impl, std::set<std::string>());
    {
        ASSERT_EQ(2, parameter_row->GetSchema()->GetColumnCnt());
        ASSERT_TRUE(parameter_row->Init(4));
        ASSERT_TRUE(parameter_row->AppendString("mc_0"));
        ASSERT_TRUE(parameter_row->AppendInt64(1594800959830));
        ASSERT_TRUE(parameter_row->Build());

        auto rs = router->ExecuteSQL(db, where_exist, parameter_row, &status);

        if (!rs) {
            FAIL() << "fail to execute sql";
        }
        ASSERT_EQ(rs->Size(), 3);
    }

@jingchen2222 jingchen2222 added this to the v0.3 milestone Aug 11, 2021
@jingchen2222 jingchen2222 self-assigned this Aug 11, 2021
@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2021

Linux Test Report

     48 files  ±0     121 suites  ±0   35m 24s ⏱️ ±0s
7 959 tests ±0  7 959 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
7 963 runs  ±0  7 963 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit ae71267. ± Comparison against base commit ae71267.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Aug 11, 2021

Codecov Report

Merging #248 (a58b2d2) into main (237fa28) will increase coverage by 0.04%.
The diff coverage is 90.99%.

Impacted file tree graph

@@             Coverage Diff              @@
##               main     #248      +/-   ##
============================================
+ Coverage     81.77%   81.82%   +0.04%     
  Complexity       13       13              
============================================
  Files           283      283              
  Lines         50417    50458      +41     
  Branches         29       29              
============================================
+ Hits          41230    41285      +55     
+ Misses         9178     9164      -14     
  Partials          9        9              
Impacted Files Coverage Δ
hybridse/include/base/fe_status.h 91.66% <ø> (ø)
hybridse/src/cmd/simple_engine_demo.cc 0.00% <0.00%> (ø)
hybridse/src/vm/runner.h 92.32% <ø> (-0.19%) ⬇️
hybridse/src/vm/engine.cc 67.84% <76.47%> (+2.14%) ⬆️
hybridse/src/testing/engine_test_base.h 88.33% <80.00%> (+0.06%) ⬆️
...se/examples/toydb/src/tablet/tablet_server_impl.cc 59.21% <100.00%> (-0.90%) ⬇️
hybridse/include/node/sql_node.h 69.32% <100.00%> (ø)
hybridse/include/vm/engine.h 73.23% <100.00%> (+2.40%) ⬆️
hybridse/src/testing/engine_test_base.cc 78.93% <100.00%> (+0.16%) ⬆️
hybridse/src/vm/engine_compile_test.cc 100.00% <100.00%> (+0.50%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 237fa28...a58b2d2. Read the comment docs.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2021

HybridSE Linux Test Report

       71 files  ±0       220 suites  ±0   4m 44s ⏱️ ±0s
17 935 tests ±0  17 935 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
17 942 runs  ±0  17 942 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit ae71267. ± Comparison against base commit ae71267.

♻️ This comment has been updated with latest results.

@github-actions
Copy link
Contributor

github-actions bot commented Aug 11, 2021

HybridSE Mac Test Report

       71 files  ±0       220 suites  ±0   5m 44s ⏱️ ±0s
17 935 tests ±0  17 935 ✔️ ±0  0 💤 ±0  0 ❌ ±0 
17 942 runs  ±0  17 942 ✔️ ±0  0 💤 ±0  0 ❌ ±0 

Results for commit ae71267. ± Comparison against base commit ae71267.

♻️ This comment has been updated with latest results.

@jingchen2222 jingchen2222 linked an issue Aug 14, 2021 that may be closed by this pull request
CHANGELOG.md Show resolved Hide resolved
CPPLINT.cfg Outdated Show resolved Hide resolved
hybridse/include/base/fe_status.h Show resolved Hide resolved
.github/workflows/cicd.yaml Outdated Show resolved Hide resolved
src/sdk/mini_cluster.h Show resolved Hide resolved
@jingchen2222 jingchen2222 mentioned this pull request Aug 20, 2021
3 tasks
@imotai imotai merged commit ae71267 into 4paradigm:main Aug 20, 2021
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.

SqlRouter support Execute Batch SQL with ParameterRow/Request Row
3 participants