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 3 commits into
base: main
Choose a base branch
from
Open

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

wants to merge 3 commits 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 [email protected] 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
@@ -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
@@ -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
nastra
nastra reviewed Nov 14, 2024
View reviewed changes
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java Outdated
@@ -1200,6 +1200,8 @@ private RESTViewBuilder(SessionContext context, TableIdentifier identifier) {
checkViewIdentifierIsValid(identifier);
this.identifier = identifier;
this.context = context;
properties.putAll(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised that the same isn't done for tables. Can you please double-check whether default/override properties properly work with the rest catalog?

Copy link
Contributor

Choose a reason for hiding this comment

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

I checked and it seems this change here isn't necessary because the default properties will be set on the server when the view builder is used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

RESTCompatibilityKitViewCatalogTests starts failing due to lack of default properties without this change. Should I revert or update the different place?

Copy link
Contributor

Choose a reason for hiding this comment

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

in that case maybe we should set the default props here too, but it would be good to investigate why this isn't necessary for tables

Copy link
Contributor

Choose a reason for hiding this comment

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

@ebyhr did you have a chance looking into this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nastra Perhaps, we need a similar change for tables. The new CatalogTests.testDefaultTableProperties doesn't pass without RESTSessionCatalog change. Added a new commit to fix it.

@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 from 7911403 to 178a06f Compare November 24, 2024 13:22
@ebyhr ebyhr force-pushed the ebi/view-default-properties branch from 178a06f to eb8b60b Compare November 24, 2024 23:43
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