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

Core: Move internal struct projection to SupportsIndexProjection #11132

Merged
merged 1 commit into from
Sep 18, 2024

Conversation

rdblue
Copy link
Contributor

@rdblue rdblue commented Sep 13, 2024

This makes projection more generic and reusable for internal types.

@github-actions github-actions bot added the core label Sep 13, 2024
@rdblue rdblue force-pushed the refactor-internal-class-projection branch 2 times, most recently from 0307b93 to 98cc1c6 Compare September 13, 2024 23:59
@rdblue rdblue force-pushed the refactor-internal-class-projection branch from 98cc1c6 to 46a0cf5 Compare September 16, 2024 17:46
}

/** Base constructor for building the type mapping */
protected SupportsIndexProjection(Types.StructType baseType, Types.StructType projectionType) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer direct imports for nested classes (if the context is clear) to shorten lines. Given that this is a new class, I'd consider importing StructType and NestedField directly. That said, our codebase isn't very consistent. Up to you.

Copy link
Contributor

@aokolnychyi aokolnychyi left a comment

Choose a reason for hiding this comment

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

Looks amazing!

@rdblue rdblue merged commit bbeadea into apache:main Sep 18, 2024
46 checks passed
rdblue added a commit to rdblue/iceberg that referenced this pull request Sep 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants