You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "zeroshade (via GitHub)" <gi...@apache.org> on 2025/03/20 15:44:12 UTC
[PR] fix(table): fix funky test paths [iceberg-go]
zeroshade opened a new pull request, #351:
URL: https://github.com/apache/iceberg-go/pull/351
fixes #346
Was using the wrong path when generating the Metadata in the tests `createTable` method.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): fix funky test paths [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #351:
URL: https://github.com/apache/iceberg-go/pull/351#discussion_r2006133656
##########
table/table_test.go:
##########
@@ -280,10 +280,10 @@ func (t *TableWritingTestSuite) writeParquet(fio iceio.WriteFileIO, filePath str
func (t *TableWritingTestSuite) createTable(identifier table.Identifier, formatVersion int, spec iceberg.PartitionSpec, sc *iceberg.Schema) *table.Table {
meta, err := table.NewMetadata(sc, &spec, table.UnsortedSortOrder,
- t.getMetadataLoc(), iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
+ t.location, iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
Review Comment:
Yea, it's a little weird, but I get the handle on it now. the metadata only knows the table's base location (`t.metadata.Location()`) while the table itself knows the actual location of the metadata json file (`t.metadataLocation`).
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): fix funky test paths [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade merged PR #351:
URL: https://github.com/apache/iceberg-go/pull/351
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): fix funky test paths [iceberg-go]
Posted by "zeroshade (via GitHub)" <gi...@apache.org>.
zeroshade commented on code in PR #351:
URL: https://github.com/apache/iceberg-go/pull/351#discussion_r2006135579
##########
table/table_test.go:
##########
@@ -280,10 +280,10 @@ func (t *TableWritingTestSuite) writeParquet(fio iceio.WriteFileIO, filePath str
func (t *TableWritingTestSuite) createTable(identifier table.Identifier, formatVersion int, spec iceberg.PartitionSpec, sc *iceberg.Schema) *table.Table {
meta, err := table.NewMetadata(sc, &spec, table.UnsortedSortOrder,
- t.getMetadataLoc(), iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
+ t.location, iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
t.Require().NoError(err)
- return table.New(identifier, meta, t.location, iceio.LocalFS{}, nil)
+ return table.New(identifier, meta, t.getMetadataLoc(), iceio.LocalFS{}, nil)
Review Comment:
yea, i should probably rename that parameter.
that `location` parameter is *supposed* to be the metadata location. While the tables base location should come from the Metadata that is passed in.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(table): fix funky test paths [iceberg-go]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #351:
URL: https://github.com/apache/iceberg-go/pull/351#discussion_r2006099386
##########
table/table_test.go:
##########
@@ -280,10 +280,10 @@ func (t *TableWritingTestSuite) writeParquet(fio iceio.WriteFileIO, filePath str
func (t *TableWritingTestSuite) createTable(identifier table.Identifier, formatVersion int, spec iceberg.PartitionSpec, sc *iceberg.Schema) *table.Table {
meta, err := table.NewMetadata(sc, &spec, table.UnsortedSortOrder,
- t.getMetadataLoc(), iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
+ t.location, iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
Review Comment:
👍 table's base location (`t.Location()`) is actually the table metadata's location (`t.metadata.Location()`).
https://github.com/apache/iceberg-go/blob/5142432b98b90b33c3f6af417de4d40aa3be8946/table/table.go#L57-L64
##########
table/table_test.go:
##########
@@ -280,10 +280,10 @@ func (t *TableWritingTestSuite) writeParquet(fio iceio.WriteFileIO, filePath str
func (t *TableWritingTestSuite) createTable(identifier table.Identifier, formatVersion int, spec iceberg.PartitionSpec, sc *iceberg.Schema) *table.Table {
meta, err := table.NewMetadata(sc, &spec, table.UnsortedSortOrder,
- t.getMetadataLoc(), iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
+ t.location, iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
t.Require().NoError(err)
- return table.New(identifier, meta, t.location, iceio.LocalFS{}, nil)
+ return table.New(identifier, meta, t.getMetadataLoc(), iceio.LocalFS{}, nil)
Review Comment:
this is slightly confusing since a table can have base location, data location, and metadata location.
##########
table/table_test.go:
##########
@@ -280,10 +280,10 @@ func (t *TableWritingTestSuite) writeParquet(fio iceio.WriteFileIO, filePath str
func (t *TableWritingTestSuite) createTable(identifier table.Identifier, formatVersion int, spec iceberg.PartitionSpec, sc *iceberg.Schema) *table.Table {
meta, err := table.NewMetadata(sc, &spec, table.UnsortedSortOrder,
- t.getMetadataLoc(), iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
+ t.location, iceberg.Properties{"format-version": strconv.Itoa(formatVersion)})
t.Require().NoError(err)
- return table.New(identifier, meta, t.location, iceio.LocalFS{}, nil)
+ return table.New(identifier, meta, t.getMetadataLoc(), iceio.LocalFS{}, nil)
Review Comment:
`table.New`'s `location` parameter overrides tables metadata location
https://github.com/apache/iceberg-go/blob/5142432b98b90b33c3f6af417de4d40aa3be8946/table/table.go#L311-L315
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org