Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
wolfeidau commented on PR #59: URL: https://github.com/apache/iceberg-go/pull/59#issuecomment-2156294155 @zeroshade I have had a shot at tidying this feature and addressing your feedback. -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
wolfeidau commented on PR #59: URL: https://github.com/apache/iceberg-go/pull/59#issuecomment-2078141567 @zeroshade I will dig into this in the next few days, I have implemented the builder for tables but need a way to have a generic path builder. Given I have time off next week I can dig into it more. -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
zeroshade commented on PR #59: URL: https://github.com/apache/iceberg-go/pull/59#issuecomment-2073185271 any updates @wolfeidau? -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
wolfeidau commented on code in PR #59:
URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1504998764
##
table/metadata.go:
##
@@ -399,3 +400,32 @@ func (m *MetadataV2) UnmarshalJSON(b []byte) error {
m.preValidate()
return m.validate()
}
+
+func NewMetadataV2(schema *iceberg.Schema, partitionSpec
iceberg.PartitionSpec, sortOrder SortOrder, location string, tableUUID
uuid.UUID, properties iceberg.Properties) (*MetadataV2, error) {
Review Comment:
@zeroshade Yeah i thought about that, the only "optional" thing is
properties, some have "empty values" like sort order and partition spec so it
may work OK.
I will try out a builder and post an update.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
wolfeidau commented on code in PR #59: URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1504982899 ## docs/cfn/AWS_TESTING.md: ## @@ -0,0 +1,74 @@ + + +# AWS integration testing + Review Comment: @zeroshade good question, it was currently a bit adhoc, and mostly my "notes" and code related to manaul testing so I put it in docs. Happy to move it up a level, it was my view that it would probably form some examples or reference infrastructure at some point. Again interested in your thoughts, I like to provide lots of resources to consumers, giving them as much as possible to ensure they succeed. -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
wolfeidau commented on code in PR #59:
URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1504979600
##
catalog/catalog.go:
##
@@ -185,3 +197,33 @@ func TableNameFromIdent(ident table.Identifier) string {
func NamespaceFromIdent(ident table.Identifier) table.Identifier {
return ident[:len(ident)-1]
}
+
+func getMetadataPath(locationPath string, newVersion int) (string, error) {
+ if newVersion < 0 {
+ return "", fmt.Errorf("invalid table version: %d must be a
non-negative integer", newVersion)
+ }
+
+ metaDataPath, err := url.JoinPath(strings.TrimLeft(locationPath, "/"),
"metadata", fmt.Sprintf("%05d-%s.metadata.json", newVersion,
uuid.New().String()))
+ if err != nil {
+ return "", fmt.Errorf("failed to build metadata path: %w", err)
+ }
+
+ return metaDataPath, nil
+}
Review Comment:
Yeah I think there are a few couple of things like this that came out of me
"using it in anger", I was interested in your thoughts but wasn't super
stressed as it is private.
Hopefully we can push some of this down to the table.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
wolfeidau commented on code in PR #59:
URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1504975878
##
catalog/catalog.go:
##
@@ -185,3 +197,33 @@ func TableNameFromIdent(ident table.Identifier) string {
func NamespaceFromIdent(ident table.Identifier) table.Identifier {
return ident[:len(ident)-1]
}
+
+func getMetadataPath(locationPath string, newVersion int) (string, error) {
+ if newVersion < 0 {
+ return "", fmt.Errorf("invalid table version: %d must be a
non-negative integer", newVersion)
+ }
+
+ metaDataPath, err := url.JoinPath(strings.TrimLeft(locationPath, "/"),
"metadata", fmt.Sprintf("%05d-%s.metadata.json", newVersion,
uuid.New().String()))
+ if err != nil {
+ return "", fmt.Errorf("failed to build metadata path: %w", err)
+ }
+
+ return metaDataPath, nil
+}
+
+func getLocationForTable(location, defaultLocation, database, tableName
string) (*url.URL, error) {
+ if location != "" {
+ return url.Parse(location)
+ }
+
+ if defaultLocation == "" {
+ return nil, fmt.Errorf("no default path is set, please specify
a location when creating a table")
+ }
+
+ u, err := url.Parse(defaultLocation)
+ if err != nil {
+ return nil, fmt.Errorf("failed to parse location URL: %w", err)
+ }
+
+ return u.JoinPath(fmt.Sprintf("%s.db", database), tableName), nil
+}
Review Comment:
@zeroshade I found this pattern in the python library, but I have seen it
when I created a table using spark as well I think.
I thought it was quite neat, and wanted to dig into where it came from but
didn't get a chance.
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
zeroshade commented on code in PR #59:
URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1503479405
##
table/metadata.go:
##
@@ -399,3 +400,32 @@ func (m *MetadataV2) UnmarshalJSON(b []byte) error {
m.preValidate()
return m.validate()
}
+
+func NewMetadataV2(schema *iceberg.Schema, partitionSpec
iceberg.PartitionSpec, sortOrder SortOrder, location string, tableUUID
uuid.UUID, properties iceberg.Properties) (*MetadataV2, error) {
Review Comment:
Should we use a builder pattern like we do for manifests? Instead of a
single func with a ton of params?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
zeroshade commented on code in PR #59: URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1503478251 ## docs/cfn/AWS_TESTING.md: ## @@ -0,0 +1,74 @@ + + +# AWS integration testing + Review Comment: why `/docs/cfn/` ? What is `cfn`? -- 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-unsubscr...@iceberg.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
zeroshade commented on code in PR #59:
URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1503477206
##
catalog/catalog.go:
##
@@ -185,3 +197,33 @@ func TableNameFromIdent(ident table.Identifier) string {
func NamespaceFromIdent(ident table.Identifier) table.Identifier {
return ident[:len(ident)-1]
}
+
+func getMetadataPath(locationPath string, newVersion int) (string, error) {
+ if newVersion < 0 {
+ return "", fmt.Errorf("invalid table version: %d must be a
non-negative integer", newVersion)
+ }
+
+ metaDataPath, err := url.JoinPath(strings.TrimLeft(locationPath, "/"),
"metadata", fmt.Sprintf("%05d-%s.metadata.json", newVersion,
uuid.New().String()))
+ if err != nil {
+ return "", fmt.Errorf("failed to build metadata path: %w", err)
+ }
+
+ return metaDataPath, nil
+}
+
+func getLocationForTable(location, defaultLocation, database, tableName
string) (*url.URL, error) {
+ if location != "" {
+ return url.Parse(location)
+ }
+
+ if defaultLocation == "" {
+ return nil, fmt.Errorf("no default path is set, please specify
a location when creating a table")
+ }
+
+ u, err := url.Parse(defaultLocation)
+ if err != nil {
+ return nil, fmt.Errorf("failed to parse location URL: %w", err)
+ }
+
+ return u.JoinPath(fmt.Sprintf("%s.db", database), tableName), nil
+}
Review Comment:
is the `*.db` syntax path specific to glue? I don't think it's used in the
REST catalog at all. Should this go into `glue.go` instead of here?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
Re: [PR] feat: glue table creation with some docs on testing [iceberg-go]
zeroshade commented on code in PR #59:
URL: https://github.com/apache/iceberg-go/pull/59#discussion_r1503476723
##
catalog/catalog.go:
##
@@ -185,3 +197,33 @@ func TableNameFromIdent(ident table.Identifier) string {
func NamespaceFromIdent(ident table.Identifier) table.Identifier {
return ident[:len(ident)-1]
}
+
+func getMetadataPath(locationPath string, newVersion int) (string, error) {
+ if newVersion < 0 {
+ return "", fmt.Errorf("invalid table version: %d must be a
non-negative integer", newVersion)
+ }
+
+ metaDataPath, err := url.JoinPath(strings.TrimLeft(locationPath, "/"),
"metadata", fmt.Sprintf("%05d-%s.metadata.json", newVersion,
uuid.New().String()))
+ if err != nil {
+ return "", fmt.Errorf("failed to build metadata path: %w", err)
+ }
+
+ return metaDataPath, nil
+}
Review Comment:
are these specific to glue?
And wouldn't generating the uuid and version etc. be something that should
be in the table package rather than in here?
--
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-unsubscr...@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: issues-unsubscr...@iceberg.apache.org
For additional commands, e-mail: issues-h...@iceberg.apache.org
