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

BUG: Fix handling of Array types in Postgres UDF #2015

Merged
merged 1 commit into from
Oct 31, 2019

Conversation

scottcode
Copy link
Contributor

@scottcode scottcode commented Oct 28, 2019

Other scalar types can be represented either by the class or an instance, but Array types work differently. Array types must be an instance, because the Array class must be instantiated specifying the datatype of the elements of the array. This was causing an error in the translation.

ibis/sql/postgres/tests/test_udf.py:176 (test_array_type)
con_for_udf = <ibis.sql.postgres.client.PostgreSQLClient object at 0x11eb3ada0>
test_schema = 'udf_test_18'
table = PostgreSQLTable[table]
  name: udf_test_users
  schema:
    user_id : int32
    user_name : string
    name_length : int32

    def test_array_type(con_for_udf, test_schema, table):
        """Test that usage of Array types work
        Other scalar types can be represented either by the class or an instance,
        but Array types work differently. Array types must be an instance,
        because the Array class must be instantiated specifying the datatype
        of the elements of the array.
        """
        pysplit_udf = udf(
            con_for_udf,
            pysplit,
            (dt.string, dt.string),
            dt.Array(dt.string),
            schema=test_schema,
>           replace=True,
        )

test_udf.py:190: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../udf/api.py:179: in udf
    return_type = ibis_to_postgres_str(out_type)
../udf/api.py:43: in ibis_to_postgres_str
    return sa_type_to_postgres_str(ibis_to_pg_sa_type(ibis_type))
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

sa_type = ARRAY(Text())

    def sa_type_to_postgres_str(sa_type):
        """Map a Postgres-compatible sqlalchemy type to a Postgres-appropriate
        string"""
>       return sa_type().compile(dialect=sa_postgres_dialect())
E       TypeError: 'ARRAY' object is not callable

Other scalar types can be represented either by the class or an
instance, but Array types work differently. Array types must
be an instance, because the Array class must be instantiated
specifying the datatype of the elements of the array. This was
causing an error in the translation.
@scottcode scottcode changed the title Postgres UDF: Fix handling of Array types BUG: Fix handling of Array types in Postgres UDF Oct 29, 2019
@scottcode scottcode added bug Incorrect behavior inside of ibis postgres The PostgreSQL backend labels Oct 29, 2019
@xmnlab
Copy link
Contributor

xmnlab commented Oct 31, 2019

hey @scottcode sorry for the delay .. I will review that today after lunch

@xmnlab xmnlab self-assigned this Oct 31, 2019
@xmnlab
Copy link
Contributor

xmnlab commented Oct 31, 2019

thanks @scottcode for fixing this issue.

@xmnlab xmnlab merged commit 6de22c7 into ibis-project:master Oct 31, 2019
@jreback
Copy link
Contributor

jreback commented Oct 31, 2019

note that we should be doing release notes on the patches themselves

@xmnlab
Copy link
Contributor

xmnlab commented Oct 31, 2019

currently ibis is using https://github.com/ibis-project/ibis/blob/master/dev/genrelease.py to create automatically the release notes.

I don't know much about this script but maybe @cpcloud could give us an idea and we can add that to the documentation.

@xmnlab
Copy link
Contributor

xmnlab commented Oct 31, 2019

sympy has a nice bot for release notes https://github.com/sympy/sympy-bot .. not sure if it could be integrated easily to ibis current workflow

@jreback
Copy link
Contributor

jreback commented Oct 31, 2019

-1 on using bots or scripts for this

the notes should be in the patch PR it’s way more readable and ultimately causes much less work

@xmnlab
Copy link
Contributor

xmnlab commented Oct 31, 2019

I don't mind to use one or the other approach. I think the most important thing is to document the approach that should be used and make it clear for anyone during the PR phase .. maybe using a github PR template.

@xmnlab
Copy link
Contributor

xmnlab commented Oct 31, 2019

but as I commented .. the current approach is the script.
maybe we could discuss that on gitter or an issue .. or we can reschedule our meeting to be early and discuss that.

@scottcode
Copy link
Contributor Author

-1 on using bots or scripts for this

the notes should be in the patch PR it’s way more readable and ultimately causes much less work

@jreback Assuming we go this route, what should I do? Edit the message of the pull request? If so, could you point me to a good example to emulate? Thanks for any pointers. Writing better pull request messages or other documentation sounds like a good idea regardless.

@jreback
Copy link
Contributor

jreback commented Nov 1, 2019

like this: https://github.com/ibis-project/ibis/pull/2009/files

this is by far the easiest way to go as it packages the PR and the release note in a single commit on merge. its also community reviewed.

@scottcode
Copy link
Contributor Author

@jreback Thanks for pointing me to a good example of adding to the release notes. I'll follow that with my future pull requests.

@xmnlab
Copy link
Contributor

xmnlab commented Nov 5, 2019

if we will following this way .. maybe we need to take care about the previous PR merged .. maybe run for the last time the release script to update the release notes ... and update the documentation https://docs.ibis-project.org/contributing.html#releasing ... also maybe a PR template to guide the contributor to create the release notes.

@scottcode
Copy link
Contributor Author

@xmnlab For this current issue, should I create another PR adding to the release notes? Just let me know and I can do that.

@xmnlab
Copy link
Contributor

xmnlab commented Nov 5, 2019

@scottcode if you want to do it, go ahead :)
I will open an issue to keep this discussion opened if there is any objection.
maybe discuss guidelines etc.

@xmnlab xmnlab mentioned this pull request Nov 5, 2019
scottcode added a commit to scottcode/ibis that referenced this pull request Nov 5, 2019
scottcode added a commit to scottcode/ibis that referenced this pull request Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior inside of ibis postgres The PostgreSQL backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants