feat: Add Ocient support #22812

alexclavel-ocient · 2023-01-20T20:39:29Z

WIP

SUMMARY

Adding support for Ocient databases

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

* doc: add docstrings to functions * chore: clean up comments * perf: avoid copying row data that doesn't need to be sanitized

chore: code cleanup

Adding custom errors to Ocient engine spec

* Fix: Replace invalid inplace type conversion * Nit: Add comment clarifying why we cannot update in place * Nit: Generate functions based on cursor, not rows

* Adding support for query cancellation * Removing double import * Adding support for query cancellation Removing double import Inverting logic for fetch data. Re-add missing function * Fiding doubled up merge inputs * Handle Query cancellation for multiple queries * Adding mutex lock to query id mapping * Removing unecesssary iteration over values of cache * Fixing locks in db engine spec * Fix error capture in fetch_data

jwilliams-ocient · 2023-01-31T22:08:33Z

Thanks for putting this PR up @alexclavel-ocient !

A note for the rock star reviewers: expect 1 additional commit for the user doc 🙂 !

betodealmeida · 2023-02-03T18:49:11Z

This looks great! Feel free to ping me when when doc is up (here or on Slack) and I'll review it! <3

betodealmeida

This looks really good, it's nice to see a DB engine spec so complete! 💪🏼

I left a question about using threading and async query execution.

betodealmeida · 2023-02-06T19:15:00Z

docs/docs/databases/ocient.mdx

+```
+
+**NOTE**: You must enter the `user` and `password` credentials.  `host` defaults to localhost,
+port defaults to 4050, database defaults to `system` and `tls` defaults


How is tls passed to the URL?

Great question! We should definitely provide an example here - will fix

superset/db_engine_specs/ocient.py

betodealmeida · 2023-02-06T19:21:38Z

superset/db_engine_specs/ocient.py

+    # These are inserted into the cache when executing the query
+    # They are then removed, either upon cancellation or query completion
+    query_id_mapping: Dict[str, str] = dict()
+    query_id_mapping_lock = threading.Lock()


Did you also test this with async queries? (with the query being executed by Celery)

Hi @betodealmeida , I am the Ocient developer tasked with pushing this PR over the finish line, thanks for all of your help thus far!

Is there an integration test that stresses this async queries + ocient, or just a manual look-and-pray-for-racy-condition-if-exists? Pardon my ignorance, I found this doc, but couldn't find anything w.r.t. testing.

superset/db_engine_specs/ocient.py

* Fix: make cte_alias compatible with Ocient * Fixing indentation in ocient.py

alexclavel-ocient · 2023-02-09T19:02:22Z

@betodealmeida Any Idea why the CI cannot find pyocient? We have sqlalchemy-ocient in setup.py, and pyocient is a requirement for sqlalchemy-ocient.

betodealmeida · 2023-02-27T21:18:07Z

@betodealmeida Any Idea why the CI cannot find pyocient? We have sqlalchemy-ocient in setup.py, and pyocient is a requirement for sqlalchemy-ocient.

The dependency in setup.py is optional, to have it run with CI you need to add it to requirements/testing.in:

superset/requirements/testing.in

Line 19 in 42db7e5

-e file:.[bigquery,hive,presto,trino]

* v1.0.15 removes upper bound on protobuf version (conflict)

jwilliams-ocient · 2023-04-11T13:58:01Z

Is it possible for me to kick off a CI run using the Superset infra on my end?

villebro · 2023-04-11T14:49:54Z

@jwilliams-ocient I started CI (after your first merged PR CI will start automatically)

* docs: add example of setting DSN params * docs: apply review suggestions Co-authored-by: rmasters1 <100157128+rmasters1@users.noreply.github.com> --------- Co-authored-by: rmasters1 <100157128+rmasters1@users.noreply.github.com>

…alled on the target host (#22)

jwilliams-ocient · 2023-04-20T14:08:05Z

Hi @villebro , sorry to bug you again, but could you kick off another CI run for me? Crossing my fingers this is the final attempt 🤞🏾

villebro · 2023-04-20T14:08:50Z

Sure 👍

jwilliams-ocient · 2023-04-20T14:46:40Z

Ugh, once more, please? I'm an idiot and pushed a partial commit.

In the event this run fails causing me to have to push another commit,, should I just create a separate PR with a single, empty commit in order to gain write access / stop bugging your team for trivial hit-the-run-button tasks? @villebro 🙏🏾

villebro · 2023-04-20T15:26:49Z

@jwilliams-ocient absolutely no problem! 🙂👍

jwilliams-ocient · 2023-04-20T15:47:38Z

Alright, @villebro , hopefully this is the last time I bug you today 😆. Here is a request for write access #23755

villebro · 2023-04-20T16:12:10Z

Alright, @villebro , hopefully this is the last time I bug you today 😆. Here is a request for write access #23755

@jwilliams-ocient on a slightly related note, I recently read James Dyson's book "Invention, a life", in which he mentions that it took him 5127 prototypes before his first vacuum cleaner worked. So we're still far off from that figure 😂

* refactor: holy shit I botched another commit lol * fix: address warnings generated by pylint * fix: fix custom errors matchers * test: fix broken unit tests

codecov · 2023-04-20T19:43:22Z

Codecov Report

Merging #22812 (2b52c93) into master (9dfaad7) will increase coverage by 0.78%.
The diff coverage is 35.20%.

❗ Current head 2b52c93 differs from pull request most recent head e82386e. Consider uploading reports for the commit e82386e to get more accurate results

@@            Coverage Diff             @@
##           master   #22812      +/-   ##
==========================================
+ Coverage   67.29%   68.07%   +0.78%     
==========================================
  Files        1878     1924      +46     
  Lines       72137    74238    +2101     
  Branches     7866     8103     +237     
==========================================
+ Hits        48541    50537    +1996     
- Misses      21577    21624      +47     
- Partials     2019     2077      +58

Flag	Coverage Δ
hive	`53.27% <ø> (?)`
mysql	`79.18% <ø> (+0.93%)`	⬆️
postgres	`79.25% <ø> (+0.95%)`	⬆️
presto	`53.18% <ø> (?)`
python	`83.10% <ø> (+1.27%)`	⬆️
sqlite	`77.75% <ø> (+1.12%)`	⬆️
unit	`53.06% <ø> (+0.56%)`	⬆️

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

Impacted Files	Coverage Δ
...ackages/superset-ui-chart-controls/src/fixtures.ts	`100.00% <ø> (ø)`
...et-ui-chart-controls/src/operators/sortOperator.ts	`100.00% <ø> (ø)`
...t-ui-chart-controls/src/shared-controls/mixins.tsx	`16.66% <ø> (ø)`
...d/packages/superset-ui-chart-controls/src/types.ts	`100.00% <ø> (ø)`
.../packages/superset-ui-core/src/chart/types/Base.ts	`100.00% <ø> (ø)`
...-core/src/hooks/useChangeEffect/useChangeEffect.ts	`100.00% <ø> (ø)`
...hooks/useComponentDidMount/useComponentDidMount.ts	`100.00% <ø> (ø)`
...oks/useComponentDidUpdate/useComponentDidUpdate.ts	`100.00% <ø> (ø)`
...src/hooks/useElementOnScreen/useElementOnScreen.ts	`100.00% <ø> (ø)`
...erset-ui-core/src/hooks/usePrevious/usePrevious.ts	`100.00% <ø> (ø)`
... and 122 more

... and 463 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

* style: run pre-commit hooks * style: remove type annotations that may not exist * build: remove ocient from requirements/testing.in

jwilliams-ocient · 2023-04-21T14:26:25Z

Is the failure from E2E / cypress-matrix (2, chrome) (pull_request) a flaky test? I'm unsure if the failure is related to our changes.

CC: @villebro , @rusackas

jwilliams-ocient · 2023-04-24T13:12:07Z

Hi @rusackas , @betodealmeida , and @villebro. On behalf of Ocient, I just wanted to say thank you for helping us push this PR through, much appreciated!

rusackas · 2023-04-25T01:08:00Z

Thanks to @jwilliams-ocient and @alexclavel-ocient for contributing this! If you want to talk about next steps to spread the word, hit me up on Slack!

Co-authored-by: jwilliams-ocient <60358443+jwilliams-ocient@users.noreply.github.com> Co-authored-by: Jordan Williams <jwilliams@ocient.com> Co-authored-by: rmasters1 <100157128+rmasters1@users.noreply.github.com>

jwilliams-ocient and others added 2 commits January 13, 2023 08:44

feat: add OcientEngineSpec

a8b023c

chore: code cleanup

879f522

* doc: add docstrings to functions * chore: clean up comments * perf: avoid copying row data that doesn't need to be sanitized

pull-request-size bot added the size/L label Jan 20, 2023

alexclavel-ocient marked this pull request as draft January 20, 2023 20:39

jwilliams-ocient and others added 6 commits January 23, 2023 06:57

Merge pull request #4 from ocient/user/jwilliams/docstrings

e53ef91

chore: code cleanup

Adding custom errors to Ocient engine spec

9e3ef85

Fixing syntax regex to match all ocient syntax errors

818474d

Cleaning up imports

e8c97ba

Merge pull request #5 from ocient/user/aclavel/DB-22258

47a7678

Adding custom errors to Ocient engine spec

chore: add Ocient dependencies to setup.py (#3)

d465588

alexclavel-ocient changed the title ~~wip: feat: Add Ocient support~~ feat: Add Ocient support Jan 24, 2023

rusackas marked this pull request as ready for review January 24, 2023 16:16

rusackas marked this pull request as draft January 24, 2023 16:16

jwilliams-ocient and others added 5 commits January 24, 2023 12:02

fix: add missing comma to inline dict element

bee5368

Merge pull request #7 from ocient/jwilliams-ocient-patch-2

6e773cc

test: add unit tests for Ocient engine spec (#6)

ffb76bc

DB-23237 Superset: datatypes are not being converted properly (#8)

8059833

* Fix: Replace invalid inplace type conversion * Nit: Add comment clarifying why we cannot update in place * Nit: Generate functions based on cursor, not rows

pull-request-size bot added size/XL and removed size/L labels Jan 30, 2023

Run Black Formatting on Ocient DB Engine Spec (#9)

6979aed

rusackas requested review from betodealmeida and eschutho January 31, 2023 18:07

alexclavel-ocient marked this pull request as ready for review January 31, 2023 20:41

jwilliams-ocient added 2 commits February 6, 2023 12:01

chore: add userdoc for Ocient DB (#10)

a83a363

fix: rename 'sqlalchemy_ocient' package to 'sqlalchemy-ocient' (#11)

4dd143c

betodealmeida approved these changes Feb 6, 2023

View reviewed changes

betodealmeida reviewed Feb 6, 2023

View reviewed changes

superset/db_engine_specs/ocient.py Outdated Show resolved Hide resolved

Fix: make cte_alias compatible with Ocient (#16)

75e4e35

* Fix: make cte_alias compatible with Ocient * Fixing indentation in ocient.py

alexclavel-ocient and others added 3 commits February 28, 2023 10:59

Adding Ocient to testing requirements (#18)

4d88c78

Fix: make fetch_data check for rows properly (#19)

c01ccda

build[pyocient]: set min version for pyocient (#20)

1f1c5a3

* v1.0.15 removes upper bound on protobuf version (conflict)

jwilliams-ocient and others added 2 commits April 12, 2023 09:10

docs: add example of setting DSN params (#21)

30b13e5

* docs: add example of setting DSN params * docs: apply review suggestions Co-authored-by: rmasters1 <100157128+rmasters1@users.noreply.github.com> --------- Co-authored-by: rmasters1 <100157128+rmasters1@users.noreply.github.com>

fix: recover from pyocient import errors. The library may not be inst…

b2ba77d

…alled on the target host (#22)

fix: forgot to remove the unchecked imports at the top of the file...

33891c4

jwilliams-ocient added 2 commits April 20, 2023 15:31

chore: remove potentially unknown type annotations

3f802d8

chore: add epoch_to_dttm override

d4a4d0b

jwilliams-ocient mentioned this pull request Apr 20, 2023

chore: write access request for jwilliams-ocient #23755

Closed

9 tasks

test: fix broken unit tests (#23)

787e303

* refactor: holy shit I botched another commit lol * fix: address warnings generated by pylint * fix: fix custom errors matchers * test: fix broken unit tests

chore: pass pre-commit checks (#24)

e82386e

* style: run pre-commit hooks * style: remove type annotations that may not exist * build: remove ocient from requirements/testing.in

rusackas merged commit adde667 into apache:master Apr 23, 2023

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add Ocient support #22812

feat: Add Ocient support #22812

alexclavel-ocient commented Jan 20, 2023

jwilliams-ocient commented Jan 31, 2023

betodealmeida commented Feb 3, 2023

betodealmeida left a comment

betodealmeida Feb 6, 2023

jwilliams-ocient Feb 7, 2023

betodealmeida Feb 6, 2023

jwilliams-ocient Apr 10, 2023

alexclavel-ocient commented Feb 9, 2023

betodealmeida commented Feb 27, 2023

jwilliams-ocient commented Apr 11, 2023

villebro commented Apr 11, 2023

jwilliams-ocient commented Apr 20, 2023

villebro commented Apr 20, 2023

jwilliams-ocient commented Apr 20, 2023

villebro commented Apr 20, 2023

jwilliams-ocient commented Apr 20, 2023

villebro commented Apr 20, 2023

codecov bot commented Apr 20, 2023 •

edited

Loading

jwilliams-ocient commented Apr 21, 2023 •

edited

Loading

jwilliams-ocient commented Apr 24, 2023

rusackas commented Apr 25, 2023

feat: Add Ocient support #22812

feat: Add Ocient support #22812

Conversation

alexclavel-ocient commented Jan 20, 2023

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

jwilliams-ocient commented Jan 31, 2023

betodealmeida commented Feb 3, 2023

betodealmeida left a comment

Choose a reason for hiding this comment

betodealmeida Feb 6, 2023

Choose a reason for hiding this comment

jwilliams-ocient Feb 7, 2023

Choose a reason for hiding this comment

betodealmeida Feb 6, 2023

Choose a reason for hiding this comment

jwilliams-ocient Apr 10, 2023

Choose a reason for hiding this comment

alexclavel-ocient commented Feb 9, 2023

betodealmeida commented Feb 27, 2023

jwilliams-ocient commented Apr 11, 2023

villebro commented Apr 11, 2023

jwilliams-ocient commented Apr 20, 2023

villebro commented Apr 20, 2023

jwilliams-ocient commented Apr 20, 2023

villebro commented Apr 20, 2023

jwilliams-ocient commented Apr 20, 2023

villebro commented Apr 20, 2023

codecov bot commented Apr 20, 2023 • edited Loading

Codecov Report

jwilliams-ocient commented Apr 21, 2023 • edited Loading

jwilliams-ocient commented Apr 24, 2023

rusackas commented Apr 25, 2023

codecov bot commented Apr 20, 2023 •

edited

Loading

jwilliams-ocient commented Apr 21, 2023 •

edited

Loading