feat(plugin-delta): Add support to show the external table location of Delta tables when running the SHOW CREATE TABLE command#26986
Conversation
Reviewer's GuideAdds support in the Delta connector to surface the external table location in SHOW CREATE TABLE by propagating the Delta table location into ConnectorTableMetadata properties, and adds an integration test to verify the generated CREATE TABLE statement includes external_location. Sequence diagram for SHOW CREATE TABLE with external_location in Delta connectorsequenceDiagram
actor User
participant PrestoCoordinator
participant DeltaMetadata
participant DeltaTableHandle
participant DeltaTable
participant DeltaTableProperties
participant ConnectorTableMetadata
User->>PrestoCoordinator: SHOW CREATE TABLE delta_schema.table
PrestoCoordinator->>DeltaMetadata: getTableMetadata(session, schemaTableName, tableHandle)
DeltaMetadata->>DeltaTableHandle: getDeltaTable()
DeltaTableHandle-->>DeltaMetadata: deltaTable
DeltaMetadata->>DeltaTable: getColumns()
DeltaTable-->>DeltaMetadata: List columns
DeltaMetadata->>DeltaTable: getTableLocation()
DeltaTable-->>DeltaMetadata: tableLocation
DeltaMetadata->>DeltaTableProperties: EXTERNAL_LOCATION_PROPERTY
DeltaTableProperties-->>DeltaMetadata: external_location_key
DeltaMetadata->>ConnectorTableMetadata: new ConnectorTableMetadata(tableName, columnMetadata, properties)
ConnectorTableMetadata-->>DeltaMetadata: connectorTableMetadata
DeltaMetadata-->>PrestoCoordinator: connectorTableMetadata
PrestoCoordinator-->>User: CREATE TABLE statement with external_location property
Updated class diagram for DeltaMetadata and related Delta table metadata classesclassDiagram
class DeltaMetadata {
+ConnectorTableMetadata getTableMetadata(ConnectorSession session, SchemaTableName tableName, DeltaTableHandle tableHandle)
}
class DeltaTableHandle {
+DeltaTable getDeltaTable()
}
class DeltaTable {
+List columns
+String tableLocation
+List getColumns()
+String getTableLocation()
}
class ConnectorTableMetadata {
+SchemaTableName table
+List columns
+Map properties
+ConnectorTableMetadata(SchemaTableName tableName, List columns, Map properties)
}
class DeltaTableProperties {
<<static>> String EXTERNAL_LOCATION_PROPERTY
}
DeltaMetadata --> DeltaTableHandle : uses
DeltaTableHandle --> DeltaTable : returns
DeltaMetadata --> DeltaTable : reads columns and tableLocation
DeltaMetadata --> ConnectorTableMetadata : constructs
DeltaMetadata --> DeltaTableProperties : uses constant
DeltaTable --> ConnectorTableMetadata : columns data
DeltaTableProperties --> ConnectorTableMetadata : property key
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The new
getTableMetadata(ConnectorSession, SchemaTableName, DeltaTable)API feels a bit awkward: callers now have to optionally pass aDeltaTablejust to populate properties while the method still looks uptableHandle.getDeltaTable()for columns; consider either always deriving theDeltaTableinside this method (noDeltaTableparameter) or consistently using the passedDeltaTablefor both properties and columns to avoid divergence and null plumbing. - In the
testShowCreateTableassertion, the expected SQL string is tightly coupled to the exact formatting (whitespace, property ordering); if the SHOW CREATE TABLE printer evolves slightly this test may become fragile, so it might be worth normalizing or comparing in a more structure-aware way rather than strict string equality.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `getTableMetadata(ConnectorSession, SchemaTableName, DeltaTable)` API feels a bit awkward: callers now have to optionally pass a `DeltaTable` just to populate properties while the method still looks up `tableHandle.getDeltaTable()` for columns; consider either always deriving the `DeltaTable` inside this method (no `DeltaTable` parameter) or consistently using the passed `DeltaTable` for both properties and columns to avoid divergence and null plumbing.
- In the `testShowCreateTable` assertion, the expected SQL string is tightly coupled to the exact formatting (whitespace, property ordering); if the SHOW CREATE TABLE printer evolves slightly this test may become fragile, so it might be worth normalizing or comparing in a more structure-aware way rather than strict string equality.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
1dfc506 to
af37040
Compare
presto-delta/src/main/java/com/facebook/presto/delta/DeltaMetadata.java
Outdated
Show resolved
Hide resolved
presto-delta/src/test/java/com/facebook/presto/delta/TestDeltaIntegration.java
Outdated
Show resolved
Hide resolved
presto-delta/src/test/java/com/facebook/presto/delta/TestDeltaIntegration.java
Outdated
Show resolved
Hide resolved
PingLiuPing
left a comment
There was a problem hiding this comment.
Thanks for the feature.
af37040 to
27e6d67
Compare
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
DeltaMetadata.getTableMetadata, consider returning an immutable empty map when there are no properties and an immutable singleton map whentableLocationis non-null instead of constructing a mutableHashMap<>(1), which avoids exposing a mutable properties map and removes the magic initial capacity. - The
testShowCreateTableassertion does a strict string equality comparison on the entire generated SQL; you might want to normalize or compare structurally (e.g., ignoring whitespace or property ordering) to make the test less brittle to harmless formatting changes in the SHOW CREATE TABLE output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `DeltaMetadata.getTableMetadata`, consider returning an immutable empty map when there are no properties and an immutable singleton map when `tableLocation` is non-null instead of constructing a mutable `HashMap<>(1)`, which avoids exposing a mutable properties map and removes the magic initial capacity.
- The `testShowCreateTable` assertion does a strict string equality comparison on the entire generated SQL; you might want to normalize or compare structurally (e.g., ignoring whitespace or property ordering) to make the test less brittle to harmless formatting changes in the SHOW CREATE TABLE output.
## Individual Comments
### Comment 1
<location> `presto-delta/src/test/java/com/facebook/presto/delta/TestDeltaIntegration.java:364-366` </location>
<code_context>
+ " external_location = '%s'\n" +
+ ")";
+
+ String expectedSqlCommand = format(createTableQueryTemplate, fullTableName, goldenTablePath(tableName));
+
+ String showCreateTableCommandResult = (String) computeActual("SHOW CREATE TABLE " + fullTableName).getOnlyValue();
+
+ assertEquals(showCreateTableCommandResult, expectedSqlCommand);
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case covering a Delta table with no `tableLocation` to exercise the null/absent-location path
Since `external_location` is now only added when `deltaTable.getTableLocation()` is non-null, please add a test where `tableLocation` is null/absent. For instance, create a Delta table via the connector without specifying `external_location` and assert that `SHOW CREATE TABLE` omits the `WITH ( external_location = ...)` clause. This will exercise the empty-property-map branch and guard against regressions in metadata population.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| String expectedSqlCommand = format(createTableQueryTemplate, fullTableName, goldenTablePath(tableName)); | ||
|
|
||
| String showCreateTableCommandResult = (String) computeActual("SHOW CREATE TABLE " + fullTableName).getOnlyValue(); |
There was a problem hiding this comment.
suggestion (testing): Add a test case covering a Delta table with no tableLocation to exercise the null/absent-location path
Since external_location is now only added when deltaTable.getTableLocation() is non-null, please add a test where tableLocation is null/absent. For instance, create a Delta table via the connector without specifying external_location and assert that SHOW CREATE TABLE omits the WITH ( external_location = ...) clause. This will exercise the empty-property-map branch and guard against regressions in metadata population.
…f Delta tables when running the SHOW CREATE TABLE command.
27e6d67 to
d96d4e9
Compare
PingLiuPing
left a comment
There was a problem hiding this comment.
Thanks @acarpente-denodo
hantangwangd
left a comment
There was a problem hiding this comment.
Thanks @acarpente-denodo
Description
Add support to show the external table location of Delta tables when running the SHOW CREATE TABLE command.
Motivation and Context
Test Plan
Integration test added
Contributor checklist
Release Notes