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

fix: add uuid to duckdb, postgres #1790

Closed
wants to merge 1 commit into from
Closed

fix: add uuid to duckdb, postgres #1790

wants to merge 1 commit into from

Conversation

skokenes
Copy link
Contributor

Adds uuid support to DuckDB and Postgres. I believe this fixes #1764 and #1761, but need some manual verification still

Copy link
Collaborator

@mtoy-googly-moogly mtoy-googly-moogly left a comment

Choose a reason for hiding this comment

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

I do not approve this change. We used to do this, I backed it out when I added unsupported (now called unsupported native sql) types.

This is not "supporting" uuid. This is pretending that a uuid is a string.

I am open to argument about how wrong I am, but I need the argument.

@leandro-hermes
Copy link

leandro-hermes commented Jul 24, 2024

I don't think (in my poor knowledge) that we need some kind of full support for UUID in Malloy - I don't see any use for this.

But on the other hand, I see benefits in just considering them as string fields, because currently Malloy doesn't allow us to create expressions with unsupported types:

line 4: Unsupported type not allowed in expression
  |   where: id = '6ffae411-6852-4131-9ba9-e022838991b7'
  |          ^ 

The need to cast the field as string causes the database to not properly use the indexes created on these fields, degrading the performance.

@skokenes
Copy link
Contributor Author

I do not approve this change. We used to do this, I backed it out when I added unsupported (now called unsupported native sql) types.

This is not "supporting" uuid. This is pretending that a uuid is a string.

I am open to argument about how wrong I am, but I need the argument.

Ok. This came across my desk because the legacy renderer isn't showing uuid columns at all, they just disappear in the output tables. I'll look into why. The "Unsupported" renderer is supposed to render the unsupported data type as a string if it can. And it would seem like we should be able to do that with a uuid column, so I'm not sure whats going on there.

@skokenes skokenes closed this Jul 25, 2024
@mtoy-googly-moogly
Copy link
Collaborator

@leandro-hermes this is a great example, the idea of unsupported native types is newish and we definitely need to listen to people trying to use them and work on them a little

the workaround for that might be to write, (i think)

where: id = '6ffae411-6852-4131-9ba9-e022838991b7'::"uuid"

how does that feel?

@leandro-hermes
Copy link

@mtoy-googly-moogly I don't like the idea of casting the string, since we don't need to do this in SQL (at least in PostgreSQL and DuckDB). Since Malloy is supposed to simplify SQL, by adding this, we would hurt this idea.

But, it is not a thing that would make me stop using Malloy at all.

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.

When using refinements, unsupported type appears in JSON but not in HTML
3 participants