FEAT: Implement read_csv for omniscidb backend

[anmyachev]

continuation of #1977

The advantage of this approach is the avoidance of additional memory costs.

Problem:

• for test purpose I should have dataset inside omniscidb container. What the best way for this?

[jreback]

is there a way to add a test on CI for this?

[anmyachev]

is there a way to add a test on CI for this?

@jreback I can create new omnisci container with needed test datasets inside. After that, it remains only to write a test

[xmnlab]

@anmyachev thanks for work on that!

maybe you can add a test that creates a dataframe, use that to create a csv file (to_csv), load this csv using the backend read_csv and check if the result is the same.

[xmnlab]

hey @anmyachev it seems there are some errors on CI related to black format. to avoid that you can install git pre-commit hooks using make develop

[anmyachev]

clickhouse-sqlalchemy 0.1.2 is not compatible with clickhouse-driver 0.1.1

[anmyachev]

@jreback @xmnlab PR is ready for review

[xmnlab]

@anmyachev in general this PR looks very good.

I liked your approach to create the csv file inside the OmniSciDB container!

I just have some questions/comments :

• related to the clickhouse-driver, why was it pinned to >=0.1.3? doesn't work >=0.1.2?
• maybe you could change from quoted to quotechar .. so it could be more generic.
• not sure, maybe @jreback could give us a direction, but maybe you can also add read_csv to ir.TableExpr just raising NotImplementedError, so another backend could implement the same method with the same parameters. Also you move your tests from omniscidb/tests to tests/all ... so it would be easier when another backend implements this function. @jreback what is your thoughts about this?

[jreback]

IIRC this is just efficiently puts a file into the db from a csv, so a data-loading operation? I know postgresql supports this as well, not sure if its implemented.

This is an alternative way of constructing a table (e.g. we normally introspect an existing one), or use the Client to create ones.

Have to think about if we actually should add any api for this. For now I would not, but certainly raise an issue.

[anmyachev]

@xmnlab PR is ready for review. While there is no exact decision whether all backends need to have this function, I suggest leaving it only for Omnisci and adding it already in release 1.3. This way we can get feedback from users.

[xmnlab]

@jreback any feedback about this ?

[anmyachev]

Green CI depends on #2104

[xmnlab]

@anmyachev could you check CI for py36 y py37
it seems tests are failing just for read_csv:

FAILED ibis/omniscidb/tests/test_client.py::test_read_csv[/omnisci/test_read_csv.csv]
FAILED ibis/omniscidb/tests/test_client.py::test_read_csv[filename1] - Except...

py38 is green because currently it skips tests for omniscidb

[anmyachev]

@anmyachev could you check CI for py36 y py37
it seems tests are failing just for read_csv:

FAILED ibis/omniscidb/tests/test_client.py::test_read_csv[/omnisci/test_read_csv.csv]
FAILED ibis/omniscidb/tests/test_client.py::test_read_csv[filename1] - Except...

py38 is green because currently it skips tests for omniscidb

These two tests were fixed in last commit.

[anmyachev]

@jreback PR is ready for review.

[jreback]

use a fixture to handle resource creation & destruction rather than using a try/finally (you can do that in the fixture itself)

[jreback]

thanks @anmyachev