-
Notifications
You must be signed in to change notification settings - Fork 438
[paimon] Align error msg when altering table properties #2046
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
Conversation
d176209 to
06e5c57
Compare
06e5c57 to
ee8d1c3
Compare
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.
Pull request overview
This PR aligns the error message format when altering properties of a non-existing table. The error message now uses the Fluss TablePath format instead of the Paimon-specific identifier format, and the message text is updated from "not exists" to "does not exist" for grammatical correctness.
Key changes:
- Updated error message to use
TablePathinstead of PaimonIdentifierformat - Corrected grammar in error message from "not exists" to "does not exist"
- Added comprehensive test coverage for the error message in both scenarios (non-existing database and non-existing table)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| PaimonLakeCatalog.java | Refactored alterTable method to accept TablePath parameter and convert to Paimon format internally, ensuring error messages display the Fluss table path |
| PaimonLakeCatalogTest.java | Added test cases verifying the error message format when attempting to alter properties of non-existing tables |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@nastra Thanks for the PR. LGTM overall, but I think we can also change the msg in private void createTable(Identifier tablePath, Schema schema, boolean isCreatingFlussTable)
throws Catalog.DatabaseNotExistException {
try {
// not ignore if table exists
paimonCatalog.createTable(tablePath, schema, false);
} catch (Catalog.TableAlreadyExistException e) {
try {
Table table = paimonCatalog.getTable(tablePath);
FileStoreTable fileStoreTable = (FileStoreTable) table;
validatePaimonSchemaCompatible(
tablePath, fileStoreTable.schema().toSchema(), schema);
// if creating a new fluss table, we should ensure the lake table is empty
if (isCreatingFlussTable) {
checkTableIsEmpty(tablePath, fileStoreTable);
}
} catch (Catalog.TableNotExistException tableNotExistException) {
// shouldn't happen in normal cases
throw new RuntimeException(
String.format(
"Failed to create table %s in Paimon. The table already existed "
+ "during the initial creation attempt, but subsequently "
+ "could not be found when trying to get it. "
+ "Please check whether the Paimon table was manually deleted, and try again.",
tablePath));
}
}
} |
ee8d1c3 to
0e0f551
Compare
|
@LiebingYu I've updated the error msg of |
0e0f551 to
6db1b21
Compare
luoyuxia
left a comment
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.
@nastra Thanks. LGTM
[FLUSS-2113] Upgrade Deployed Maven to Latest Stable Release (3.9.11) [FLUSS-2113] Upgrade Deployed Maven to Latest Stable Release (3.9.11)
Purpose
This aligns the error msg that is thrown when altering table properties of a non-existing table. Previously the error msg contained a paimon-specific identifier and I've changed it to show the
TablePathinstead. I've also added a testBrief change log
Tests
Added a unit test to verify the error msg
API and Format
Documentation