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(vertica-driver): VerticaDriver #7289

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

Conversation

timbrownls20
Copy link

@timbrownls20 timbrownls20 commented Oct 24, 2023

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

[#690]

Description of Changes Made (if issue reference is not provided)

New @cubejs-backend/vertica-driver package based on vertica-nodejs -- it includes driver itself and basic tests for overridden BaseDriver methods.

published to npm npm i @knowitall/vertica-driver

continuation of this stale PR #5100

@timbrownls20 timbrownls20 requested review from a team as code owners October 24, 2023 04:19
@vercel
Copy link

vercel bot commented Oct 24, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

8 Ignored Deployments
Name Status Preview Comments Updated (UTC)
examples-angular-dashboard ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am
examples-react-d3 ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am
examples-react-dashboard ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am
examples-react-data-table ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am
examples-react-highcharts ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am
examples-react-material-ui ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am
examples-react-pivot-table ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am
examples-vue-query-builder ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2024 3:58am

@github-actions github-actions bot added the pr:community Contribution from Cube.js community members. label Oct 24, 2023
@timbrownls20 timbrownls20 changed the title Vertica driver feat(vertica-driver): VerticaDriver Oct 24, 2023
@paveltiunov
Copy link
Member

@timbrownls20 Thanks for contributing! Could you please write a readme on your npm package on how to install and use it? We'll add a backlink to it in the third-party driver list.

@paveltiunov paveltiunov self-assigned this Oct 25, 2023
@timbrownls20
Copy link
Author

@timbrownls20 Thanks for contributing! Could you please write a readme on your npm package on how to install and use it? We'll add a backlink to it in the third-party driver list.

No problem. Added instructions to README for the npm package. Wasn't sure what you wanted, so hope this is OK. If you need amends could you post a link to an example. Many Thanks

@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 47.04%. Comparing base (5a27200) to head (ea68485).
Report is 32 commits behind head on master.

❗ Current head ea68485 differs from pull request most recent head 04334c0. Consider uploading reports for the commit 04334c0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7289      +/-   ##
==========================================
- Coverage   48.02%   47.04%   -0.99%     
==========================================
  Files         154      155       +1     
  Lines       21019    20768     -251     
  Branches     5264     5224      -40     
==========================================
- Hits        10095     9770     -325     
+ Misses      10723    10678      -45     
- Partials      201      320     +119     
Flag Coverage Δ
cube-backend 47.04% <ø> (-0.99%) ⬇️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@paveltiunov
Copy link
Member

paveltiunov commented Oct 28, 2023

@timbrownls20 I actually meant README of @knowitall/vertica-driver. Please see the process here https://github.com/cube-js/cube/blob/master/CONTRIBUTING.md#contributing-database-drivers. You can see https://www.npmjs.com/package/arangodb-cubejs-driver as an example.

@timbrownls20
Copy link
Author

@timbrownls20 I actually meant README of @knowitall/vertica-driver. Please see the process here https://github.com/cube-js/cube/blob/master/CONTRIBUTING.md#contributing-database-drivers. You can see https://www.npmjs.com/package/arangodb-cubejs-driver as an example.

No problem. README update in knowitall npm

I've also updated the package refs in the PR to be @knowitall/vertica-driver otherwise you get errors with the refresh as it loads a package which doesn't yet exist. I assume we'll change the package to @cubejs-backend/vertoca-driver at the point the PR is merged.

Let me know if you need anything else. Thanks

@paveltiunov
Copy link
Member

@timbrownls20 Great! Added it here https://cube.dev/docs/product/configuration/data-sources#third-party-drivers. What's the best way to communicate with you regarding issues? We'll start forwarding all conversations about this Vertica driver to you.

@timbrownls20
Copy link
Author

@timbrownls20 Great! Added it here https://cube.dev/docs/product/configuration/data-sources#third-party-drivers. What's the best way to communicate with you regarding issues? We'll start forwarding all conversations about this Vertica driver to you.

I've had a shared mailbox set up for our development team. I'll assign a dev to answer queries as they come up - dev_kia@knowitall.net.au

@guyzilb
Copy link

guyzilb commented Mar 13, 2024

@paveltiunov there any ETA for merging this PR ?
something is missing here?

@igorlukanin
Copy link
Member

@guyzilb Did you have success using it as @knowitall/vertica-driver? Could you please describe your experience?

@guyzilb
Copy link

guyzilb commented Mar 19, 2024

@igorlukanin
Hey,
we also wrote Vertica driver and we already work with it in production and it works pretty well, also I tested @knowitall/vertica-driver,(and t looks the same as we did, we also used korowa)
the main problem I see here is with cubestore, you can create preagg with it but you cant change the granularity from day to month for example the reason for it is that the type of the timestamp column written inside the parquet file is not correct, we try to find where to change it but still not found where we need to implement solution for it.
also, roll-up join and Lamda Join not work with the Vertica driver because is generate the join condition not correct ( and also I think is because of the problem with the parquet file )
we using external preagg and just with roll-up preagg the problem here is the table that was created inside Vertica without partitions and projection which is very important in Vertica.
this is my experience, I hope it helps :)

@enhanse
Copy link

enhanse commented Mar 22, 2024

Hi @guyzilb, in the latest commits on this branch we started to implement the SQL dialect. Do you know if there is an override on the BaseQuery that needs to be added for the timestamp and join conditions there perhaps?

@paveltiunov / @igorlukanin

  • Is it ok if the driver is changed to TS? I can see a number of other implementations are, though I also saw a note which mentioned the driver deps should ideally be browser and node compatible
  • Should commits on the branch be squashed? Typically we squash when we merge a PR, so we left all individual commits in for now

@guyzilb
Copy link

guyzilb commented Mar 25, 2024

@enhanse
Hey,
we worked on this a few months ago, but if I remember well we did not override something in BaseQuery and
it was in PreAggregations we saw the join for roll-up join preagg take not the correct name of join column, is take the name of the dimension before the pre-agg
example :

the join between preagg should be on a.user_id=b.user_id
first pre agg called: test_1
second pre agg called: test_2

When Preagg runs some prefix is added to the column name so the join should be
test_1_user_id = test_2_user_id

but what happened is :
a.user_id=b.user_id
And because it is not true, we got an error.

I hope I remember it well

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.

None yet

6 participants