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

Support postgresql enums #414

Merged
merged 9 commits into from
Jan 19, 2024
Merged

Support postgresql enums #414

merged 9 commits into from
Jan 19, 2024

Conversation

mjperrone
Copy link
Contributor

@mjperrone mjperrone commented Jan 17, 2024

Support postgresql enums

Adds support for postgresql enums, addressing #415.

This uses the pg_type and pg_enum tables to discover the valid enum values for any columns using

Limitations

There are other postgresql custom types that I am not attempting to support in this PR:

  • composite type
  • a range type
  • a base type
  • a shell type

Validation

Using the enum example in the postgres docs, the integration test creates a new custom enum type, uses it in the test table, and expects the recap EnumType to be found.

I also added a unit test for basically the same thing.

@mjperrone mjperrone changed the title add test showing enums not being converted Support postgresql enums Jan 18, 2024
recap/clients/postgresql.py Show resolved Hide resolved
array_agg(enumlabel) AS enum_values
FROM pg_catalog.pg_enum
GROUP BY enumtypid
) enums ON enums.enumtypid = pg_type.oid
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: pg_catalog.pg_type.oid for consistency?

@@ -55,7 +67,8 @@ def setup_class(cls):
test_bit_array BIT(8)[],
test_not_null_array INTEGER[] NOT NULL,
test_int_array_2d INTEGER[][],
test_text_array_3d TEXT[][][]
test_text_array_3d TEXT[][][],
test_enum_mood test_enum_type_mood
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about adding a secnod col that's test_enum_type_mood[] to validate enum arrays?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and it fails as-is. With that column type data_type is ARRAY and udt_name is _test_enum_type_mood interestingly.

I'd like to keep addressing arrays out of scope of this PR if that's okay with you.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ticket to track supporting that: #416

@criccomini
Copy link
Contributor

Thanks for submitting this PR! I left some comments and thoughts, but this looks good overall.

@mjperrone
Copy link
Contributor Author

Regarding the failing integration test, expected:

EnumType(type_=enum, logical=None, alias=None, doc=None, extra_attrs={'default': None}, symbols=['sad', 'ok', 'happy'])

received:

EnumType(type_=enum, logical=None, alias=None, doc=None, extra_attrs={}, symbols=['sad', 'ok', 'happy'])

Need to look into this more, but I don't think the converter is including the default value for any of the types.

@mjperrone
Copy link
Contributor Author

Looks like it may be possible by looking at COLUMN_DEFAULT in the existing query output, but I don't want to blaze that path in this PR. I'll just remove default: None in the converter and test expectations.

@criccomini criccomini merged commit e364dc6 into gabledata:main Jan 19, 2024
3 checks passed
@criccomini
Copy link
Contributor

Awesome! Merged in. Thanks for opening the enum+array ticket. :)

@criccomini
Copy link
Contributor

Released in 0.10.1! https://pypi.org/project/recap-core/0.10.1/

criccomini pushed a commit that referenced this pull request Jan 26, 2024
@alexdemeo gave this to me to help me run the integration tests locally
when implementing #414.

I assume he grabbed the docker config from [here(https://github.com/mjperrone/recap/blob/a0cbb2a47d2e3a789d76b5ba0f222b9ea8c25f8a/.github/workflows/ci.yaml#L61-L88).
I grabbed the `RECAP_URLS` value from [here(https://github.com/mjperrone/recap/blob/a0cbb2a47d2e3a789d76b5ba0f222b9ea8c25f8a/.github/workflows/ci.yaml#L165).

Admittedly I did not run all the integration tests as that was too long (just `RECAP_URLS='["postgresql://postgres:password@localhost:5432/testdb"]' pdm run pytest tests/integration/clients/test_postgresql.py` but this should be closer to describing how to all the int tests.
criccomini pushed a commit that referenced this pull request Jan 26, 2024
@alexdemeo gave this to me to help me run the integration tests locally
when implementing #414.

I assume he grabbed the docker config from here:

https://github.com/mjperrone/recap/blob/a0cbb2a47d2e3a789d76b5ba0f222b9ea8c25f8a/.github/workflows/ci.yaml#L61-L88

I grabbed the `RECAP_URLS` value from here:

https://github.com/mjperrone/recap/blob/a0cbb2a47d2e3a789d76b5ba0f222b9ea8c25f8a/.github/workflows/ci.yaml#L165

Admittedly I did not run all the integration tests as that was too long (just
`RECAP_URLS='["postgresql://postgres:password@localhost:5432/testdb"]' pdm run
pytest tests/integration/clients/test_postgresql.py` but this should be closer
to describing how to all the int tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants