BUG: sql method doesn't work when the query uses LIMIT clause
#1903
Conversation
xmnlab
changed the title
ibis/mapd/client.py
Outdated
| @@ -448,6 +471,12 @@ def _get_schema_using_query(self, query): | |||
|
|
|||
| return sch.Schema(names, ibis_types) | |||
|
|
|||
| def _get_schema_using_validator(self, query): | |||
| result = self.con._client.sql_validate(self.con._session, query) | |||
| return sch.Schema.from_dict({r: MapDDataType._mapd_to_ibis_dtypes[ | |||
There was a problem hiding this comment.
Use sch.Schema.from_tuples instead of from_dict so that order of columns is preserved.
There was a problem hiding this comment.
Working testing. I'm still a little uncertain about how ibis testing is dispatched, and what specific testing criteria would be appropriate for this change.
There was a problem hiding this comment.
@mpeaton maybe you can improve this https://github.com/ibis-project/ibis/blob/master/ibis/tests/all/test_client.py#L69
also if you want to add tests that could break in another backend .. you can also add tests for sql at https://github.com/ibis-project/ibis/blob/master/ibis/mapd/tests/test_client.py
if you want any help related to tests ping me :)
There was a problem hiding this comment.
After some deliberation, it seems that _get_schema already has test coverage within ibis/tests/all/test_client.py::test_query_schema. I was thinking that finer granularity testing would entail testing raw SQL strings against sql_validate, and would be somewhat into the weeds.
There was a problem hiding this comment.
For this particular PR at least...
There was a problem hiding this comment.
sure raw SQLs would be much better!
There was a problem hiding this comment.
Looks good @mpeaton !
I think we could change this PR from BUG to SUPP as self.con._client.sql_validate is a better implementation than limit 1 approach, and we can treat limit clause and ; and sql comment issues in another PR.
just check the comment related to _TDatumType_enum
current we are using the datumtype from pymapd:
- https://github.com/ibis-project/ibis/blob/master/ibis/mapd/client.py#L22
- https://github.com/omnisci/pymapd/blob/master/pymapd/dtypes.py#L6
- https://github.com/omnisci/pymapd/blob/master/omnisci/common/ttypes.py
if you need a dictionary .. maybe you can use it from here: https://github.com/omnisci/pymapd/blob/master/omnisci/common/ttypes.py#L34
|
@mpeaton it seems you merged master into your branch, is it right? |
|
I am doing the rebase here .. and I will work to finish this PR. |
xmnlab
changed the title
xmnlab
changed the title
sql method doesn't work when the query uses LIMIT clause
|
|
@cpcloud it seems it is done for a new review! :) thanks! |
| sys.version_info < (3, 6), | ||
| reason='`sql` method just available for Python 3.6 or greater.', | ||
| ) | ||
| def test_sql(con, sql): |
There was a problem hiding this comment.
Do these tests actually fail without the regex munging?
There was a problem hiding this comment.
yes ... also for another backend if I recall correctly .. that is why I needed to move these tests from all to mapd
There was a problem hiding this comment.
https://dev.azure.com/ibis-project/ibis/_build/results?buildId=1286&view=logs
here you can see that these tests failed for impala too (LinuxTest py37)
|
@cpcloud any thoughts about this PR? |
Currently, SQL expressions which contain a LIMIT statement trigger an exception from the Omnisci Validator, as 'LIMIT 1' is used in the get_schema_using_query routine.
Exception: Sort node not supported as input to another sortThis PR introduced a BUGFIX which utilizes the built in mapd sql_validate routine, and associated thrift type mappings to build the schema via direct reference, without the one line query.
UPDATE:
;Noneto avoid defaultLIMIT 10000