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

Convert the Logical to Physical map to a visitor #43

Merged
merged 3 commits into from
Oct 7, 2023

Conversation

Fokko
Copy link
Contributor

@Fokko Fokko commented Oct 6, 2023

I noticed that the FixedType was missing.

I noticed that the FixedType was missing.
return visit(iceberg_type, _PRIMITIVE_TO_PHYISCAL_TYPE_VISITOR)


class PrimitiveToPyhsicalType(SchemaVisitorPerPrimitiveType[str]):
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: Pyhsical (unless maybe that's how it's spelled in the python community)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤣 Good old typo

return "BYTE_ARRAY"

def visit_decimal(self, decimal_type: DecimalType) -> str:
raise ValueError("unknown")
Copy link
Contributor

Choose a reason for hiding this comment

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

We could convert to FIXED_LEN_BYTE_ARRAY or INT64 or INT32 based on the precision, right?

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 would say FIXED_LEN_BYTE_ARRAY. This visitor is just to double-check that we get the type that we expect.

Copy link
Contributor

@rdblue rdblue 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 overall, but I'd fix the typo before merging.

@Fokko Fokko merged commit ce85358 into apache:main Oct 7, 2023
6 checks passed
@Fokko
Copy link
Contributor Author

Fokko commented Oct 7, 2023

Thanks @rdblue for the quick review! 🙌

@Fokko Fokko deleted the fd-convert-map-to-visitor branch October 7, 2023 13:46
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