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

SAP HANA Driver implemented #5648

Open
wants to merge 33 commits into
base: master
Choose a base branch
from
Open

Conversation

zhjuncai
Copy link

@zhjuncai zhjuncai commented Nov 17, 2022

Check List

  • Tests has been run in packages where changes made if available
  • Linter has been run for changed code
  • Tests for the changes have been added if not covered yet
  • Docs have been added / updated if required

Issue Reference this PR resolves
This is the initial driver implementation for SAP HANA, so no issue was resolved

Description of Changes Made (if issue reference is not provided)
SAP HANA is quite heavy database thus can't create a docker container to run test cases against it. I verified all the test on a remote HANA box. see the results

Latest Test results
Screenshot 2022-12-07 at 14 13 02

@zhjuncai zhjuncai requested review from a team as code owners November 17, 2022 10:30
@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Nov 17, 2022
@zhjuncai
Copy link
Author

@ovr may I ask who will review my PR?

@codecov
Copy link

codecov bot commented Nov 18, 2022

Codecov Report

Base: 82.42% // Head: 40.81% // Decreases project coverage by -41.60% ⚠️

Coverage data is based on head (744970a) compared to base (028895d).
Patch coverage: 11.29% of modified lines in pull request are covered.

❗ Current head 744970a differs from pull request most recent head 6e4943e. Consider uploading reports for the commit 6e4943e to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #5648       +/-   ##
===========================================
- Coverage   82.42%   40.81%   -41.61%     
===========================================
  Files         139      150       +11     
  Lines       19621    19533       -88     
  Branches        0     5068     +5068     
===========================================
- Hits        16172     7973     -8199     
- Misses       3449    10736     +7287     
- Partials        0      824      +824     
Flag Coverage Δ
cube-backend 40.81% <11.29%> (?)
cubesql ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
.../cubejs-server-core/src/core/DriverDependencies.js 100.00% <ø> (ø)
...es/cubejs-schema-compiler/src/adapter/HanaQuery.js 9.83% <9.83%> (ø)
...cubejs-schema-compiler/src/adapter/QueryBuilder.js 71.42% <100.00%> (ø)
rust/cubesql/cubesql/src/compile/builder.rs
...gine/information_schema/postgres/character_sets.rs
...l/src/sql/database_variables/mysql/session_vars.rs
...nt/src/models/v1_load_request_query_filter_base.rs
...pile/engine/information_schema/postgres/pg_type.rs
...besql/cubesql/src/compile/rewrite/rules/members.rs
...le/engine/information_schema/postgres/pg_depend.rs
... and 281 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@zhjuncai zhjuncai requested a review from a team as a code owner November 21, 2022 05:58
@paveltiunov
Copy link
Member

Hey @zhjuncai ! Thanks for contributing to this one! Could you please also add at least a smoke E2E test? You can find an example here: https://github.com/cube-js/cube.js/blob/master/packages/cubejs-testing/test/smoke-materialize.test.ts.

@paveltiunov paveltiunov self-assigned this Nov 29, 2022
This reverts commit 09bbb25.
This reverts commit d730c6a.
This reverts commit 59fa47c.
@zhjuncai
Copy link
Author

Hey @zhjuncai ! Thanks for contributing to this one! Could you please also add at least a smoke E2E test? You can find an example here: https://github.com/cube-js/cube.js/blob/master/packages/cubejs-testing/test/smoke-materialize.test.ts.

Thanks @paveltiunov I will try my best to add a smoke E2E test.

zjczhjuncai and others added 7 commits November 30, 2022 12:27
CREATE TABLE test AS (SELECT * from TEST)
The HANAObjectStream doesn't work with cubestore to import csv
to pre-aggregations schema

Thus fallback to rows import which works well with pre-aggregations
It's not needed because the generated sql is not executed by HANA
@zhjuncai
Copy link
Author

zhjuncai commented Dec 7, 2022

Hello @paveltiunov I tried to add smoke test for hana driver, but it's not working as it still require test docker container for HANA. could you please review the driver first, once it's reviewed and merged. I will continue to smoke testing...

@zhjuncai
Copy link
Author

@paveltiunov could you help to review?

@paveltiunov
Copy link
Member

@zhjuncai Do you mean there's no docker image of SAP HANA available?

@paveltiunov
Copy link
Member

@zhjuncai
Copy link
Author

zhjuncai commented Jan 1, 2023

There is no supported docker image for HANA for now, Hana-express docker image is deprecated, meanwhile Hana express also requires lots of memory to run. I don't want to run the deprecated image and drain your pipeline. > @zhjuncai Do you mean there's no docker image of SAP HANA available?

@zhjuncai
Copy link
Author

zhjuncai commented Jan 1, 2023

@igorlukanin igorlukanin removed the request for review from a team January 3, 2023 10:20
@zhjuncai
Copy link
Author

I've updated the README, please start review process...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr:community Contribution from Cube.js community members.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants