You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@xtable.apache.org by "jalpan-randeri (via GitHub)" <gi...@apache.org> on 2025/01/28 08:33:40 UTC
[PR] [Bugfix] Populate table name from the identifier in Iceberge conversion [incubator-xtable]
jalpan-randeri opened a new pull request, #630:
URL: https://github.com/apache/incubator-xtable/pull/630
## What is the purpose of the pull request
While converting the iceberg table to other format such as Hudi, the icerberg source table do not populate the table name. This is due to iceberg table's behavior as it is treated as Hadoop tables. This leads to table identified as table-location, leading to confusing conversation.
BUG= [#494](https://github.com/apache/incubator-xtable/issues/494)
## Brief change log
This commit handles the conversation logic, when icebege table manager provides HadoopTable, it populate the table name from provided input TableIdentifier. This ensures that source table name is carried over to the transformation.
## Verify this pull request
- Added unit test to cover this scenario.
- Manually verified the change by running a job locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1949570427
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergConversionSource.java:
##########
@@ -123,7 +123,10 @@ public InternalTable getTable(Snapshot snapshot) {
return InternalTable.builder()
.tableFormat(TableFormat.ICEBERG)
.basePath(iceTable.location())
- .name(iceTable.name())
+ .name(
Review Comment:
Can you add a quick inline comment to explain that if the table name is not specified, Iceberg will default to the path so we use the name from the source config? It will be helpful for future developers to know the context when they come across this code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1953719674
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergConversionSource.java:
##########
@@ -120,10 +120,18 @@ public InternalTable getTable(Snapshot snapshot) {
irPartitionFields.size() > 0
? DataLayoutStrategy.HIVE_STYLE_PARTITION
: DataLayoutStrategy.FLAT;
+ // When the table name is not explicitly specified, Iceberg assumes the table is HDFS-based,
+ // treating the table name as the location in HDFS. This assumption can lead to mismatches
+ // during data conversion. To mitigate this issue, we rely on the table name provided in the
+ // source configuration of the conversation, ensuring consistency and avoiding potential errors.
Review Comment:
```suggestion
// during metadata conversion. To mitigate this issue, we rely on the table name provided in the
// source configuration of the conversation so target matches the user's expectations.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "jalpan-randeri (via GitHub)" <gi...@apache.org>.
jalpan-randeri commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1966666758
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergConversionSource.java:
##########
@@ -120,10 +120,18 @@ public InternalTable getTable(Snapshot snapshot) {
irPartitionFields.size() > 0
? DataLayoutStrategy.HIVE_STYLE_PARTITION
: DataLayoutStrategy.FLAT;
+ // When the table name is not explicitly specified, Iceberg assumes the table is HDFS-based,
+ // treating the table name as the location in HDFS. This assumption can lead to mismatches
+ // during metadata conversion. To mitigate this issue, we rely on the table name provided in the
+ // source configuration of the conversation so target matches the user's expectations.
+ // See https://github.com/apache/incubator-xtable/issues/494
return InternalTable.builder()
.tableFormat(TableFormat.ICEBERG)
.basePath(iceTable.location())
- .name(iceTable.name())
+ .name(
+ iceTable.name().contains(iceTable.location())
+ ? sourceTableConfig.getName()
+ : iceTable.name())
Review Comment:
Added new test with catalog-config
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1976486212
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergConversionSource.java:
##########
@@ -120,10 +120,18 @@ public InternalTable getTable(Snapshot snapshot) {
irPartitionFields.size() > 0
? DataLayoutStrategy.HIVE_STYLE_PARTITION
: DataLayoutStrategy.FLAT;
+ // When the table name is not explicitly specified, Iceberg assumes the table is HDFS-based,
+ // treating the table name as the location in HDFS. This assumption can lead to mismatches
+ // during metadata conversion. To mitigate this issue, we rely on the table name provided in the
+ // source configuration of the conversation so target matches the user's expectations.
+ // See https://github.com/apache/incubator-xtable/issues/494
return InternalTable.builder()
.tableFormat(TableFormat.ICEBERG)
.basePath(iceTable.location())
- .name(iceTable.name())
+ .name(
+ iceTable.name().contains(iceTable.location())
+ ? sourceTableConfig.getName()
+ : iceTable.name())
Review Comment:
@ashvina I confirmed that both branches are hit executed in our tests now. Is there anything else blocking this bug fix?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "jalpan-randeri (via GitHub)" <gi...@apache.org>.
jalpan-randeri commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1938239589
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergTableManager.java:
##########
@@ -100,6 +101,14 @@ Table getOrCreateTable(
}
}
+ Table loadHadoopTables(HadoopTables hadoopTables, String basePath, String tableName) {
Review Comment:
Good point. Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1936464864
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergTableManager.java:
##########
@@ -100,6 +101,14 @@ Table getOrCreateTable(
}
}
+ Table loadHadoopTables(HadoopTables hadoopTables, String basePath, String tableName) {
Review Comment:
Instead of manipulating the name at this level, it may be easier to just update the IcebergConversionSource which produces the `InternalTable` object
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "ashvina (via GitHub)" <gi...@apache.org>.
ashvina commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1953909895
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergConversionSource.java:
##########
@@ -120,10 +120,18 @@ public InternalTable getTable(Snapshot snapshot) {
irPartitionFields.size() > 0
? DataLayoutStrategy.HIVE_STYLE_PARTITION
: DataLayoutStrategy.FLAT;
+ // When the table name is not explicitly specified, Iceberg assumes the table is HDFS-based,
+ // treating the table name as the location in HDFS. This assumption can lead to mismatches
+ // during metadata conversion. To mitigate this issue, we rely on the table name provided in the
+ // source configuration of the conversation so target matches the user's expectations.
+ // See https://github.com/apache/incubator-xtable/issues/494
return InternalTable.builder()
.tableFormat(TableFormat.ICEBERG)
.basePath(iceTable.location())
- .name(iceTable.name())
+ .name(
+ iceTable.name().contains(iceTable.location())
+ ? sourceTableConfig.getName()
+ : iceTable.name())
Review Comment:
Could you please confirm if existing tests cases cover both the paths?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1943876428
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergTableManager.java:
##########
@@ -100,6 +101,14 @@ Table getOrCreateTable(
}
}
+ Table loadHadoopTables(HadoopTables hadoopTables, String basePath, String tableName) {
Review Comment:
I was referring to this section of the code: https://github.com/apache/incubator-xtable/pull/630/files#diff-aa668462e6a6dcd3a7ebee90de2678602f7cc68448fcf438a64ed593c221f39fR113
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "jalpan-randeri (via GitHub)" <gi...@apache.org>.
jalpan-randeri commented on code in PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#discussion_r1947965269
##########
xtable-core/src/main/java/org/apache/xtable/iceberg/IcebergTableManager.java:
##########
@@ -100,6 +101,14 @@ Table getOrCreateTable(
}
}
+ Table loadHadoopTables(HadoopTables hadoopTables, String basePath, String tableName) {
Review Comment:
Fixed
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [Bugfix] Populate table name from the identifier in Iceberg conversion [incubator-xtable]
Posted by "the-other-tim-brown (via GitHub)" <gi...@apache.org>.
the-other-tim-brown commented on PR #630:
URL: https://github.com/apache/incubator-xtable/pull/630#issuecomment-2714701245
@ashvina can you re-review now that the PR has been updated?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@xtable.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org