You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/01/26 22:57:02 UTC
[GitHub] [iceberg] cccs-eric opened a new pull request #3989: Python: MapType missing is_map_type() method
cccs-eric opened a new pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989
MapType is missing `is_map_type()` method, so when calling it, it returns `False`. That simply breaks the API. I've checked and the other types are fine. Map was the only one with such a problem.
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#discussion_r793973916
##########
File path: python_legacy/iceberg/api/types/types.py
##########
@@ -696,6 +696,12 @@ def key_id(self):
def value_id(self):
return self.value_field.field_id
+ def asMapType(self):
Review comment:
It looks like these are being converted to snake_case instead. Should this be converted as well?
--
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
[GitHub] [iceberg] rdblue commented on pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#issuecomment-1025958689
Thanks, @cccs-eric! I'm sorry that I had missed your comment explaining why you chose not to do it. I think it makes sense to go ahead and change it. It is unlikely that this is used like you noted.
--
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
[GitHub] [iceberg] cccs-eric commented on pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
cccs-eric commented on pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#issuecomment-1025658560
@rdblue I've changed the `asMapType` method to `as_map_type` as requested. I simply wanted to highlight in my previous [comment](https://github.com/apache/iceberg/pull/3989#discussion_r793977478) that this method was part of the `Type` interface. But I'm totally fine with that 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
[GitHub] [iceberg] cccs-eric edited a comment on pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
cccs-eric edited a comment on pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#issuecomment-1025658560
@rdblue I've changed the `asMapType` method to `as_map_type` as requested. I simply wanted to highlight in my previous [comment](https://github.com/apache/iceberg/pull/3989#discussion_r793977478) that this method was part of the `Type` interface. But I'm totally fine with that change.
As for the `NotImplementedError` being raised in the visitor, did you want to also get that fixed? I had a look and there is a code gap [here](https://github.com/apache/iceberg/blob/master/python_legacy/iceberg/api/types/type_util.py#L172), but I tried to remove catch clauses in the visitor to see if any test would break and everything passed. So we are obviously missing test coverage in that area, so I think a fix for this should go in another PR, or simply be moved to the new python refactoring.
Let me know if want anything else added to this PR.
--
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
[GitHub] [iceberg] cccs-eric commented on a change in pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
cccs-eric commented on a change in pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#discussion_r793997252
##########
File path: python_legacy/iceberg/api/types/type_util.py
##########
@@ -143,7 +143,30 @@ def visit(arg, visitor): # noqa: ignore=C901
return visitor.list(list_var, element_result)
elif type_var.type_id == TypeID.MAP:
- raise NotImplementedError()
+ # TODO: How could this be improved to avoid copy-pasting the same logic for key and value fields?
+ map_var = type_var.as_nested_type().asMapType()
+ visitor.field_ids.append(map_var.key_field.field_id)
+ visitor.field_names.append(map_var.key_field.name)
+ try:
+ key_result = visit(map_var.key_type(), visitor)
+ except NotImplementedError:
Review comment:
Except for abstract classes, this error is still thrown at a few spots in the code:
But for things related to Map, I could only find this:
- https://github.com/apache/iceberg/blob/master/python_legacy/iceberg/api/types/type_util.py#L172
--
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
[GitHub] [iceberg] rdblue commented on a change in pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#discussion_r793974177
##########
File path: python_legacy/iceberg/api/types/type_util.py
##########
@@ -143,7 +143,30 @@ def visit(arg, visitor): # noqa: ignore=C901
return visitor.list(list_var, element_result)
elif type_var.type_id == TypeID.MAP:
- raise NotImplementedError()
+ # TODO: How could this be improved to avoid copy-pasting the same logic for key and value fields?
+ map_var = type_var.as_nested_type().asMapType()
+ visitor.field_ids.append(map_var.key_field.field_id)
+ visitor.field_names.append(map_var.key_field.name)
+ try:
+ key_result = visit(map_var.key_type(), visitor)
+ except NotImplementedError:
Review comment:
Are there areas where this is still thrown?
--
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
[GitHub] [iceberg] cccs-eric commented on a change in pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
cccs-eric commented on a change in pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#discussion_r793977478
##########
File path: python_legacy/iceberg/api/types/types.py
##########
@@ -696,6 +696,12 @@ def key_id(self):
def value_id(self):
return self.value_field.field_id
+ def asMapType(self):
Review comment:
I fully agree with you, but it is part of the [Type interface](https://github.com/apache/iceberg/blob/master/python_legacy/iceberg/api/types/type.py#L66). I didn't want to change too much in fear of breaking other code depending on this API. I can make the change if you like, but not sure if it could create side effects. Probably nothing major since code already using that interface method are always getting `False`...!
--
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
[GitHub] [iceberg] rdblue merged pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
rdblue merged pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989
--
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
[GitHub] [iceberg] rdblue commented on pull request #3989: Python: MapType missing required methods and visit() not supporting MapType
Posted by GitBox <gi...@apache.org>.
rdblue commented on pull request #3989:
URL: https://github.com/apache/iceberg/pull/3989#issuecomment-1025261311
@cccs-eric, could you take a look at the minor fixes here? I think it's about ready but I'd like to make sure the method name 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