[#12996] fix(catalog-glue): Derive table location from the Glue database LocationUri - #12997
Open
diqiu50 wants to merge 2 commits into
Open
[#12996] fix(catalog-glue): Derive table location from the Glue database LocationUri#12997diqiu50 wants to merge 2 commits into
diqiu50 wants to merge 2 commits into
Conversation
… database LocationUri Use the LocationUri declared by the Glue database as the base for a table created without an explicit location property, falling back to the catalog warehouse property only when the database declares no location. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019pkAzwoZNs6BaxE9JHfK8q
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
This PR updates Glue table location derivation to prefer the Glue database LocationUri when a table is created without an explicit location, falling back to the catalog warehouse only when the database has no location.
Changes:
- Update
resolveTableLocationto derivedatabase-location/tablefrom Glue DBLocationUriand trim trailing slashes - Add/extend unit tests covering DB location precedence, warehouse fallback, explicit location override, and schema-not-found behavior
- Update Glue catalog documentation and property descriptions to reflect the new precedence rules
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/aws-glue-catalog.md | Updates user-facing docs for warehouse and location derivation behavior |
| catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogTableOperations.java | Adds unit tests for new location resolution logic and ensures alterTable doesn’t trigger DB lookup when not needed |
| catalogs/catalog-glue/src/test/java/org/apache/gravitino/catalog/glue/TestGlueCatalogOperationsForIceberg.java | Updates Iceberg test to stub DB lookup to reflect new resolution path |
| catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogPropertiesMetadata.java | Updates warehouse property description to match new semantics |
| catalogs/catalog-glue/src/main/java/org/apache/gravitino/catalog/glue/GlueCatalogOperations.java | Implements DB LocationUri precedence and shared trailing-slash trimming helper |
| catalogs/catalog-common/src/main/java/org/apache/gravitino/catalog/glue/GlueConstants.java | Updates Javadoc for warehouse property semantics |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
diqiu50
added
1.3.0
Code Coverage Report
Files
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changes were proposed in this pull request?
GlueCatalogOperations.resolveTableLocationnow uses theLocationUrideclared by the Glue database as the base for a table that is created without an explicitlocationproperty, and falls back to the catalogwarehouseproperty only when the database declares no location.Why are the changes needed?
The table location was always derived as
warehouse/database/table, so tables Gravitino created sat under a different prefix from tables other tools created in the same Glue database, and a catalog whosewarehousewas set to the database's own location produced a doubled database segment in the path.Fix: #12996
Does this PR introduce any user-facing change?
Yes, a table created in a Glue database that declares a
LocationUriis now placed under that URI instead of under the catalog warehouse; no property is added or removed, and existing data is not moved.How was this patch tested?
Added unit tests in
TestGlueCatalogTableOperationscovering the database location, a trailing slash on it, the warehouse fallback, an explicitlocationproperty and a failed database lookup, and ran./gradlew :catalogs:catalog-glue:test -PskipITstogether with./gradlew spotlessApply.