Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fixes #5448: Implement initial Iceberg Connector using PyIceberg #14825
Fixes #5448: Implement initial Iceberg Connector using PyIceberg #14825
Changes from 8 commits
43a717f
647b72f
26acda5
60ac93d
1047f4a
c2d764b
6d2777d
f02d937
079e052
c56232f
373aa39
55b9b21
b882f58
0563d78
54712a6
e20b58e
f157980
15d2e4d
7c1f4a2
2d6ebfc
fb091ba
af960ed
9c65960
2a7df35
915ec02
2801dcf
79dc0dd
84ed783
dd1e6c1
cd67f76
0632639
3a012bf
2b1fec2
92dde13
5787fdb
93a7f04
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think of making
IcebergCatalogFactory
either an instantiable class or just breaking it down withcatalog_type_map
being a global andfrom_connection
being a simple function? I understand the intent but we don't really need that object if we won't use it as an object.Do you see any advantage in that context of defining this piece of code directly in
_init__.py
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, to be honest I don't have a really strong opinion... Been messing around with both ways actually xD
I believe that trying to wrap things with specific classes tends to make the code a bit more readable because it gives you context straight away, but if we avoid doing that in this project I wouldn't mind having just a
from_connection
function.About defining it on init.py, for me it made sense as the
IcebergCatalogFactory
is basically the entrypoint for the sub module.Again if it's against our common practices I wouldn't mind defining it on another file and to make the import statement more directy we can "re-export" it on the init.py.