You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "haizhou-zhao (via GitHub)" <gi...@apache.org> on 2024/02/29 19:13:17 UTC
[PR] [ISSUE-9837] Correct additionalProperties for SnapshotSummary Model [iceberg]
haizhou-zhao opened a new pull request, #9838:
URL: https://github.com/apache/iceberg/pull/9838
Details described on https://github.com/apache/iceberg/issues/9837
--
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] [ISSUE-9837] Correct additionalProperties for SnapshotSummary Model [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#discussion_r1524549654
##########
open-api/rest-catalog-open-api.yaml:
##########
@@ -1972,8 +1972,8 @@ components:
operation:
type: string
enum: ["append", "replace", "overwrite", "delete"]
- additionalProperties:
- type: string
+ additionalProperties:
Review Comment:
this change LGTM and also conforms to https://swagger.io/docs/specification/data-models/dictionaries/
--
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] [ISSUE-9837] Correct additionalProperties for SnapshotSummary Model [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#issuecomment-1971810856
> Can you do make generate to have python code update as well? Otherwise LGTM
@dramaticlly Tested with OpenAPI's python generator (if that's what you mean), details also in the issue https://github.com/apache/iceberg/issues/9837. Though you'll need to scroll a bit down to see that.
--
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] OpenAPI: Fix additionalProperties for SnapshotSummary [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on code in PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#discussion_r1552476063
##########
open-api/rest-catalog-open-api.py:
##########
@@ -171,7 +171,6 @@ class SortOrder(BaseModel):
class Summary(BaseModel):
operation: Literal['append', 'replace', 'overwrite', 'delete']
- additionalProperties: Optional[str] = None
Review Comment:
So every generator is different. I don't think this is supported by the one we use right now. But I think the change in the yaml is correct 👍
--
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] OpenAPI: Fix additionalProperties for SnapshotSummary [iceberg]
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko merged PR #9838:
URL: https://github.com/apache/iceberg/pull/9838
--
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] [ISSUE-9837] Correct additionalProperties for SnapshotSummary Model [iceberg]
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#discussion_r1524549107
##########
open-api/rest-catalog-open-api.py:
##########
@@ -171,7 +171,6 @@ class SortOrder(BaseModel):
class Summary(BaseModel):
operation: Literal['append', 'replace', 'overwrite', 'delete']
- additionalProperties: Optional[str] = None
Review Comment:
I would have expected the summary to also have `Optional[Dict[str, str]] = None`
--
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] OpenAPI: Fix additionalProperties for SnapshotSummary [iceberg]
Posted by "haizhou-zhao (via GitHub)" <gi...@apache.org>.
haizhou-zhao commented on code in PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#discussion_r1538384050
##########
open-api/rest-catalog-open-api.py:
##########
@@ -171,7 +171,6 @@ class SortOrder(BaseModel):
class Summary(BaseModel):
operation: Literal['append', 'replace', 'overwrite', 'delete']
- additionalProperties: Optional[str] = None
Review Comment:
When I used the plain OpenAPI generator (tested on 6.6.0 & 7.0.0) to generate python code, this is what I got
```
...
class SnapshotSummary(BaseModel):
"""
SnapshotSummary
"""
operation: StrictStr = Field(...)
additional_properties: Dict[str, Any] = {}
__properties = ["operation"]
...
```
Which is closer to what you suggested above. I guess the code generation process here might leverage some different library.
--
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] [ISSUE-9837] Correct additionalProperties for SnapshotSummary Model [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#issuecomment-1971802670
Can you do make generate to have python code update as well? Otherwise LGTM
--
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] [ISSUE-9837] Correct additionalProperties for SnapshotSummary Model [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#issuecomment-1971825356
> > Can you do make generate to have python code update as well? Otherwise LGTM
>
>
>
> @dramaticlly Tested with OpenAPI's python generator (if that's what you mean), details also in the issue https://github.com/apache/iceberg/issues/9837. Though you'll need to scroll a bit down to see that.
Can you check if your workspace is empty after generate? I think it will update the python file and you need that as part of your commit. You can see pull request build also failed for the same reason
--
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] [ISSUE-9837] Correct additionalProperties for SnapshotSummary Model [iceberg]
Posted by "dramaticlly (via GitHub)" <gi...@apache.org>.
dramaticlly commented on PR #9838:
URL: https://github.com/apache/iceberg/pull/9838#issuecomment-1995194176
@nastra @Fokko can you help review the change?
--
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