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.

Sign up for GitHub

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

Jump to bottom

Core: Add support for view-default property in catalog #11064

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Core: Add support for view-default property in catalog #11064

wants to merge 1 commit into from

Conversation

ebyhr
Copy link
Contributor

@ebyhr ebyhr commented Sep 2, 2024

Fixes #10822

@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from a0ed40f to a130aaa Compare September 2, 2024 03:05
@ebyhr ebyhr marked this pull request as draft September 2, 2024 03:17
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from a130aaa to 2d41458 Compare September 2, 2024 04:03
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch 5 times, most recently from c4aa6ae to 1b935cb Compare September 2, 2024 05:41
@ebyhr
Copy link
Contributor Author

ebyhr commented Sep 2, 2024

CI hit #10172

@ebyhr
Copy link
Contributor Author

ebyhr commented Sep 2, 2024

@RussellSpitzer Can you review this PR when you have time?

@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from 1b935cb to 8206c5f Compare September 3, 2024 23:51
@ebyhr ebyhr marked this pull request as ready for review September 4, 2024 00:00
@nastra nastra mentioned this pull request Sep 12, 2024
Closed
nk1506
nk1506 reviewed Sep 12, 2024
View reviewed changes
core/src/main/java/org/apache/iceberg/CatalogProperties.java
@@ -29,6 +29,7 @@ private CatalogProperties() {}
public static final String WAREHOUSE_LOCATION = "warehouse";
public static final String TABLE_DEFAULT_PREFIX = "table-default.";
public static final String TABLE_OVERRIDE_PREFIX = "table-override.";
public static final String VIEW_DEFAULT_PREFIX = "view-default.";
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @ebyhr for working on this. I also raised a similar PR which I think is not required anymore. Do you think we should also add "view-override." ?

Copy link
Member

Choose a reason for hiding this comment

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

That probably makes sense since we have the table-default as well, but i have no problem this being in a followup

@github-actions GitHub Actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Nov 13, 2024
@nastra
Copy link
Contributor

nastra commented Nov 13, 2024

@ebyhr are you still working on this?

@ebyhr
Copy link
Contributor Author

ebyhr commented Nov 13, 2024

@nastra Yes, I was actually waiting for next review round.

RussellSpitzer
RussellSpitzer reviewed Nov 13, 2024
View reviewed changes
core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java Outdated
@@ -85,6 +86,7 @@ public String name() {
@Override
public void initialize(String name, Map<String, String> properties) {
this.catalogName = name != null ? name : InMemoryCatalog.class.getSimpleName();
this.catalogProperties = ImmutableMap.copyOf(properties);
Copy link
Member

@RussellSpitzer RussellSpitzer Nov 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Careful here, ImmutableMap.copyOf isn't null safe

RussellSpitzer
RussellSpitzer reviewed Nov 13, 2024
View reviewed changes
core/src/main/java/org/apache/iceberg/inmemory/InMemoryCatalog.java Outdated
@@ -94,6 +96,11 @@ public void initialize(String name, Map<String, String> properties) {
closeableGroup.setSuppressCloseFailure(true);
}

@Override
protected Map<String, String> properties() {
return catalogProperties == null ? ImmutableMap.of() : catalogProperties;
Copy link
Member

Choose a reason for hiding this comment

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

Depending on what you do above this may or may not be necessary

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is still needed in case initialize() hasn't been called

@github-actions github-actions bot removed the stale label Nov 14, 2024
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from 2f6f9fc to 5a19171 Compare November 14, 2024 12:52
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch 2 times, most recently from 4afe084 to 7911403 Compare November 16, 2024 07:14
@github-actions github-actions bot added the hive label Nov 16, 2024
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch 2 times, most recently from 178a06f to eb8b60b Compare November 24, 2024 23:43
@ebyhr ebyhr mentioned this pull request Nov 25, 2024
Open
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from eb8b60b to 08cb705 Compare November 25, 2024 09:53
@github-actions GitHub Actions
Copy link

This pull request has been marked as stale due to 30 days of inactivity. It will be closed in 1 week if no further activity occurs. If you think that’s incorrect or this pull request requires a review, please simply write any comment. If closed, you can revive the PR at any time and @mention a reviewer or discuss it on the dev@iceberg.apache.org list. Thank you for your contributions.

@github-actions github-actions bot added the stale label Dec 26, 2024
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from 08cb705 to 17c3043 Compare December 26, 2024 00:18
@ebyhr
Copy link
Contributor Author

ebyhr commented Dec 26, 2024

Rebased on main branch to resolve conflicts.

@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from 17c3043 to a9d239a Compare December 26, 2024 00:41
@github-actions github-actions bot removed the stale label Dec 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@singhpk234 singhpk234 singhpk234 left review comments

@nk1506 nk1506 nk1506 left review comments

@RussellSpitzer RussellSpitzer RussellSpitzer left review comments

@nastra nastra nastra approved these changes

Assignees
No one assigned
Labels
core docs hive NESSIE OPENAPI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

View Default Properties in the Catalog
5 participants
@ebyhr @nastra @RussellSpitzer @nk1506 @singhpk234