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

Type mapping related improvements and refactoring #440

Merged
merged 8 commits into from
Jan 23, 2024

Conversation

hashhar
Copy link
Member

@hashhar hashhar commented Jan 21, 2024

Description

  • Added more thorough tests for ARRAY type mapping
  • Extracted some modules for mappers and type objects
  • Change _create_row_mapper to not depend on specific order of the if else branches

A follow-up PR will come which adds more tests for MAP and ROW types and fixes some bugs that exist there.

Release notes

(x) This is not user-visible or docs only and no release notes are required.

This helps keep the result parsing related logic isolated from client
module.
The value being compared is the rawType from the Trino type-signatures.
The raw-type is always just the type name without any type parameters so
there's no need to use `startswith` or `contains` and instead values can
be checked for equality.
This makes it easier to identify missing types for example.
@hashhar hashhar requested a review from hovaesco January 22, 2024 09:46
@hashhar hashhar marked this pull request as ready for review January 22, 2024 09:46
@hashhar
Copy link
Member Author

hashhar commented Jan 22, 2024

These are the only "functional" commits, others are mechanical refactors.

Copy link
Member

@hovaesco hovaesco left a comment

Choose a reason for hiding this comment

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

Looks good, just one comment regarding removed values from map.

Comment on lines 172 to 173
# 'qdigest': QDIGEST,
# 'tdigest': TDIGEST,
Copy link
Member

Choose a reason for hiding this comment

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

These two don't exist in SQLAlchemy as well.

Copy link
Member

Choose a reason for hiding this comment

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

Same for HyperLogLog types.

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's still keep those in the map. It's useful to have it to show what types are mapped to what.

Note that some databases for example map their custom types to a custom SQLA type - we can do this too in future if we ever want to.

Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Just skimmed.

@hashhar hashhar merged commit 5731329 into trinodb:master Jan 23, 2024
12 checks passed
@hashhar hashhar deleted the hashhar/type-improvements branch February 3, 2024 10:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

3 participants