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

[Improvement] revisit all the purgeXXX implementations to have a consistent behavior #3685

Closed
mchades opened this issue May 31, 2024 · 4 comments · Fixed by #4091
Closed
Assignees
Labels
0.6.0 Release v0.6.0 improvement Improvements on everything

Comments

@mchades
Copy link
Contributor

mchades commented May 31, 2024

What would you like to be improved?

we should revisit all the purgeTable and purgePartition implementations to make sure that:

  1. return True if the target was purged, false if the target does not exist
  2. thrown UnsupportedOperationException If target purging is not supported

How should we improve?

see above

@zivali
Copy link
Contributor

zivali commented Jun 15, 2024

@mchades May I take a look into this one? I think we can refer to purgeTable in RelationalCatalog as the desired behavior
https://github.com/datastrato/gravitino/blob/main/clients/client-java/src/main/java/com/datastrato/gravitino/client/RelationalCatalog.java#L221-L243

@mchades
Copy link
Contributor Author

mchades commented Jun 16, 2024

@mchades May I take a look into this one? I think we can refer to purgeTable in RelationalCatalog as the desired behavior https://github.com/datastrato/gravitino/blob/main/clients/client-java/src/main/java/com/datastrato/gravitino/client/RelationalCatalog.java#L221-L243

The behavior of purgeTable in RelationalCatalog is:

  • thrown UnsupportedOperationException If target purging is not supported
  • return true if the table is purged successfully, false otherwise

One issue with this behavior is that if return false, the client will not be able to obtain the specific reason for failure.

Can you refer to the behavior of dropTable and then think about the behavior of purge? @zivali

@zivali
Copy link
Contributor

zivali commented Jun 16, 2024

Got it. Thank you for the clarification! :)

  • false should be returned only if the target does not exist (NoSuchTableException, NoSuchSchemaException or NoSuchEntityException)
  • UnsupportedOperationException if catalog doesn't support
  • All other exceptions should be thrown

Also, I didn't find detail implementations of purgePartition in the code. Is it still work in progress?

@mchades
Copy link
Contributor Author

mchades commented Jun 17, 2024

Got it. Thank you for the clarification! :)

  • false should be returned only if the target does not exist (NoSuchTableException, NoSuchSchemaException or NoSuchEntityException)
  • UnsupportedOperationException if catalog doesn't support
  • All other exceptions should be thrown

This proposal LGTM

Also, I didn't find detail implementations of purgePartition in the code. Is it still work in progress?

It's not started yet. Currently, we have already supported partition operations in the Hive catalog, but we have not yet implemented purge partition. If you are interested, you can start researching it first.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0.6.0 Release v0.6.0 improvement Improvements on everything
Projects
None yet
2 participants