FEAT: [OmniSciDB] Add method parameter to load_data #2165

xmnlab · 2020-04-01T15:25:33Z

Resolve #2164

xmnlab · 2020-04-01T19:49:55Z

this PR is done for review. thanks!

ibis/omniscidb/tests/test_client.py

xmnlab · 2020-05-28T19:09:33Z

this PR is ready again for a new review.

xmnlab · 2020-06-18T15:36:04Z

a friendly reminder about this PR :)

xmnlab · 2020-07-01T17:20:01Z

a friendly reminder about this PR :) thanks

datapythonista

lgtm, thanks @xmnlab

datapythonista · 2020-07-15T19:07:48Z

Release is conflicting.

xmnlab · 2020-07-15T19:52:09Z

thanks @datapythonista ! rebased!

xmnlab · 2020-07-26T23:36:28Z

branch rebased. thanks.

datapythonista · 2020-07-27T14:23:59Z

@xmnlab can you address this comment please? #2165 (comment)

xmnlab · 2020-07-27T14:42:58Z

@datapythonista sure. sorry I forgot that one. I am working on that. thanks

datapythonista

Thanks @xmnlab

@jreback I think this should be ready now.

setup.py

kcpevey · 2020-09-03T12:54:34Z

@jreback this is ready for another review. Thanks!

datapythonista

Thanks @xmnlab for this. Looks good. You've got some conflicts, and I added few suggestions that I think will make the code better.

datapythonista · 2020-10-15T12:44:59Z

ibis/omniscidb/client.py

+        obj: Union[pd.DataFrame, pyarrow.Table],
+        database: Optional[str] = None,
+        method: str = 'rows',
+    ):


Is removing **kwargs intentional?

yes, the initial idea for kwarg here was to allow a similar signature for all load_data functions across all the backends, and as it seems it is not necessary and it is not used inside the function, I am just removing that for now.

datapythonista · 2020-10-15T12:47:31Z

ibis/omniscidb/tests/test_client.py

+    if not isinstance(data, pd.DataFrame):
+        data = data.to_pandas()  # pyarrow.Table
+
+    pd.testing.assert_frame_equal(data, result)


Suggested change

pd.testing.assert_frame_equal(data, result)

pd.testing.assert_frame_equal(result, data)

I think it's clearer and more standard to use result == expected than the other way round.

datapythonista · 2020-10-15T12:48:50Z

ibis/omniscidb/tests/test_client.py

+    result = con.table(temp_table).execute()
+
+    if not isinstance(data, pd.DataFrame):
+        data = data.to_pandas()  # pyarrow.Table


I don't understand this, if you always want the pandas, we not simply use assert_frame_equal(result, df_salary) below instead?

yes, as we are using the same data for both, it works. thanks

datapythonista · 2020-10-15T12:51:36Z

ibis/omniscidb/tests/test_client.py

+)
+def test_load_data(con, temp_table, method, data):
+    con.create_table(temp_table, schema=sch_salary)
+    con.load_data(temp_table, data, method=method)


I would prefer if you define df_salary, arrow_salary... inside the function, and you use the strings pandas and arrow for the data parameter (that can be renamed to format). Then you can do:

Suggested change

con.load_data(temp_table, data, method=method)

con.load_data(temp_table, df_salary if format == 'pandas' else arrow_salary, method=method)

This will save memory in the testing job and will keep things more compact.

xmnlab · 2020-10-19T15:54:22Z

@datapythonista I applied the suggestion and CI is green. let me know if I missed anything here pls. Thanks!

datapythonista · 2020-10-19T15:59:54Z

ibis/backends/omniscidb/tests/test_client.py

+        'infer',
+        'arrow',
+        'arrow',
+        'arrow',


This doesn't seem to make a lot of sense, not sure if I'm missing something. method,format means that there should be two elements, so I'm wondering if pytest is iterating over the strings to get two values, and method is r and format is o for rows.

Also, if doesn't make sense that we have pandas, arrow... repeated several times, it will run the same exact test.

yep ... it doesn't make any sense ... sorry ... no idea why it is in this format :X ... I will pay more attention next time ... also .. No idea neither why the tests passed :X

but the way, the "matrix" of possibilities should be the same of the initial commit:

('rows', df_salary), ('columnar', df_salary), ('infer', df_salary), ('infer', arrow_salary), ('arrow', arrow_salary)

so it should be something like

('rows', 'pandas'), ('columnar', 'pandas'), ('infer', 'pandas'), ('infer', 'arrow'), ('arrow', 'arrow')

I will fix this issue .. and sorry again for the noise ... I really don't know what happened

datapythonista · 2020-10-19T16:00:43Z

setup.cfg

@@ -16,7 +16,7 @@ inherit = false
 convention = numpy

 [isort]
-known_third_party = asv,click,clickhouse_driver,dateutil,google,graphviz,impala,kudu,mock,multipledispatch,numpy,pandas,pkg_resources,plumbum,psycopg2,pyarrow,pydata_google_auth,pygit2,pymapd,pymysql,pyspark,pytest,pytz,regex,requests,setuptools,sphinx_rtd_theme,sqlalchemy,thrift,toolz
+known_third_party = click,clickhouse_driver,dateutil,google,graphviz,impala,kudu,mock,multipledispatch,numpy,pandas,pkg_resources,plumbum,psycopg2,pyarrow,pydata_google_auth,pygit2,pymapd,pymysql,pyspark,pytest,pytz,regex,requests,setuptools,sphinx_rtd_theme,sqlalchemy,thrift,toolz


I think we fixed this, and isort/the precommit hook should start changing it. Can you revert please?

ok ... I will rebase the code again and try :) thanks!

datapythonista

lgtm, thanks @xmnlab

xmnlab · 2020-10-21T15:07:24Z

@datapythonista , is there anything else I should address here? or can it be already merged? thanks

datapythonista · 2020-10-21T15:35:20Z

@datapythonista , is there anything else I should address here? or can it be already merged? thanks

lgtm as I mentioned in the past comment. I'll let @jreback have a look before merging.

datapythonista · 2020-10-26T17:49:38Z

Sorry @xmnlab this conflicted once more. Can you merge master and fix the conflicts please? I'll merge as soon as this is green.

@jreback I think your review comments are all addressed. But if you want to have a look...

xmnlab · 2020-10-26T17:53:40Z

thanks @datapythonista ! branch rebased!

datapythonista · 2020-10-26T22:46:00Z

Thanks @xmnlab

xmnlab requested a review from jreback April 1, 2020 19:49

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from 60d7344 to b71be44 Compare April 5, 2020 01:41

datapythonista added the omnisci label Apr 6, 2020

jreback suggested changes Apr 9, 2020

View reviewed changes

ibis/omniscidb/tests/test_client.py Outdated Show resolved Hide resolved

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from b71be44 to 35f2a7e Compare April 9, 2020 16:40

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch 2 times, most recently from 0ae2554 to 6e52e16 Compare May 28, 2020 15:30

datapythonista approved these changes Jul 15, 2020

View reviewed changes

datapythonista assigned jreback Jul 15, 2020

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from 6e52e16 to ba54417 Compare July 15, 2020 19:49

kcpevey mentioned this pull request Jul 24, 2020

[Ibis] FEAT: [OmniSciDB] Add method parameter to load_data Quansight/omnisci#130

Closed

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from ba54417 to f34c65a Compare July 26, 2020 01:50

datapythonista approved these changes Jul 27, 2020

View reviewed changes

xmnlab requested a review from jreback August 13, 2020 15:46

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from aace172 to 0753dd8 Compare August 13, 2020 15:47

jreback suggested changes Aug 13, 2020

View reviewed changes

setup.py Outdated Show resolved Hide resolved

datapythonista reviewed Oct 15, 2020

View reviewed changes

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from 538afc4 to 9f6b33d Compare October 15, 2020 15:21

datapythonista reviewed Oct 19, 2020

View reviewed changes

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from 9f6b33d to 856dbe5 Compare October 19, 2020 16:25

datapythonista approved these changes Oct 19, 2020

View reviewed changes

FEAT: [OmniSciDB] Add method parameter to load_data

583632f

xmnlab force-pushed the omniscidb-add-method-parameter-to-load-data branch from 856dbe5 to 583632f Compare October 26, 2020 17:52

datapythonista merged commit e10e386 into ibis-project:master Oct 26, 2020

xmnlab deleted the omniscidb-add-method-parameter-to-load-data branch October 27, 2020 00:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT: [OmniSciDB] Add method parameter to load_data #2165

FEAT: [OmniSciDB] Add method parameter to load_data #2165

xmnlab commented Apr 1, 2020 •

edited

Loading

xmnlab commented Apr 1, 2020

xmnlab commented May 28, 2020

xmnlab commented Jun 18, 2020

xmnlab commented Jul 1, 2020

datapythonista left a comment

datapythonista commented Jul 15, 2020

xmnlab commented Jul 15, 2020

xmnlab commented Jul 26, 2020

datapythonista commented Jul 27, 2020

xmnlab commented Jul 27, 2020

datapythonista left a comment

kcpevey commented Sep 3, 2020

datapythonista left a comment

datapythonista Oct 15, 2020

xmnlab Oct 15, 2020

datapythonista Oct 15, 2020

datapythonista Oct 15, 2020

xmnlab Oct 15, 2020

datapythonista Oct 15, 2020

xmnlab commented Oct 19, 2020

datapythonista Oct 19, 2020

xmnlab Oct 19, 2020

datapythonista Oct 19, 2020

xmnlab Oct 19, 2020

datapythonista left a comment

xmnlab commented Oct 21, 2020

datapythonista commented Oct 21, 2020

datapythonista commented Oct 26, 2020

xmnlab commented Oct 26, 2020

datapythonista commented Oct 26, 2020

	pd.testing.assert_frame_equal(data, result)
	pd.testing.assert_frame_equal(result, data)

	con.load_data(temp_table, data, method=method)
	con.load_data(temp_table, df_salary if format == 'pandas' else arrow_salary, method=method)

FEAT: [OmniSciDB] Add method parameter to load_data #2165

FEAT: [OmniSciDB] Add method parameter to load_data #2165

Conversation

xmnlab commented Apr 1, 2020 • edited Loading

xmnlab commented Apr 1, 2020

xmnlab commented May 28, 2020

xmnlab commented Jun 18, 2020

xmnlab commented Jul 1, 2020

datapythonista left a comment

Choose a reason for hiding this comment

datapythonista commented Jul 15, 2020

xmnlab commented Jul 15, 2020

xmnlab commented Jul 26, 2020

datapythonista commented Jul 27, 2020

xmnlab commented Jul 27, 2020

datapythonista left a comment

Choose a reason for hiding this comment

kcpevey commented Sep 3, 2020

datapythonista left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xmnlab commented Oct 19, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datapythonista left a comment

Choose a reason for hiding this comment

xmnlab commented Oct 21, 2020

datapythonista commented Oct 21, 2020

datapythonista commented Oct 26, 2020

xmnlab commented Oct 26, 2020

datapythonista commented Oct 26, 2020

xmnlab commented Apr 1, 2020 •

edited

Loading