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] Support partition API #4786

Merged
merged 17 commits into from
Dec 30, 2024
Merged

[core] Support partition API #4786

merged 17 commits into from
Dec 30, 2024

Conversation

jerry-024
Copy link
Contributor

@jerry-024 jerry-024 commented Dec 26, 2024

Purpose

Support partition API

  • createPartition
  • dropPartition(ps: just metadata)
  • listPartitions

Linked issue: #4540

Tests

  • RESTObjectMapperTest.createPartitionRequestParseTest
  • RESTObjectMapperTest.dropPartitionRequestParseTest
  • RESTObjectMapperTest.listPartitionsResponseParseTest
  • RESTCatalogTest.testCreatePartition
  • RESTCatalogTest.testCreatePartitionWhenTableNotExist
  • RESTCatalogTest.testCreatePartitionWhenTableNoPermissionException
  • RESTCatalogTest.testDropPartition
  • RESTCatalogTest.testDropPartitionWhenPartitionNoExist
  • RESTCatalogTest.testDropPartitionWhenTableNoPermission
  • RESTCatalogTest.testDropPartitionWhenTableNoExist
  • RESTCatalogTest.testListPartitionsWhenMetastorePartitionedIsTrue
  • RESTCatalogTest.testListPartitionsFromFile
  • RESTCatalogTest.convertToPartitionEntryTest

API and Format

Documentation

@jerry-024 jerry-024 marked this pull request as draft December 26, 2024 09:49
@jerry-024 jerry-024 marked this pull request as ready for review December 30, 2024 03:03
private final Map<String, String> spec;

@JsonProperty(FIELD_PARTITION_TYPE)
private final RowType partitionType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do not add type in this class.

checkNotSystemTable(identifier, "dropPartition");
dropPartitionMetadata(identifier, partitions);
Table table = getTable(identifier);
if (table != null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

it is null? I think maybe getTable will throw TableNotExistsException.

@@ -71,4 +71,10 @@ public class RESTCatalogOptions {
.stringType()
.noDefaultValue()
.withDescription("REST Catalog auth token provider path.");

public static final ConfigOption<Boolean> METASTORE_PARTITIONED =
Copy link
Contributor

Choose a reason for hiding this comment

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

Use CoreOptions.metastore.partitioned-table.

resourcePaths.partitions(
identifier.getDatabaseName(), identifier.getTableName()),
request,
SuccessResponse.class,
Copy link
Contributor

Choose a reason for hiding this comment

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

here should return Partition?

request,
headers());
} catch (NoSuchResourceException ignore) {
return new SuccessResponse();
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 here should throw table not exist exception?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

when the partition's metadata doesn't exist in metadata, there may still be data in the filesystem. So we ignore this exception when dropping the partition.

Copy link
Contributor

Choose a reason for hiding this comment

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

should return false instead of NoSuchResourceException?

throws TableNoPermissionException {
try {
DropPartitionRequest request = new DropPartitionRequest(partitions);
return client.delete(
Copy link
Contributor

Choose a reason for hiding this comment

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

we should return Partition?

@@ -434,6 +524,23 @@ protected GetTableResponse getTableResponse(Identifier identifier)
}
}

protected boolean dropPartitionMetadata(Identifier identifier, Map<String, String> partitions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why need these tests invoking this method? Why not invoking catalog methods?

@JingsongLi
Copy link
Contributor

+1

@JingsongLi JingsongLi merged commit de7a50d into apache:master Dec 30, 2024
12 checks passed
@jerry-024 jerry-024 deleted the rest-catalog branch December 31, 2024 02:20
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