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/08/10 15:30:25 UTC
[GitHub] [iceberg] Fokko opened a new pull request, #5490: Python: Add __str__ implementation to SortOrder and SortField
Fokko opened a new pull request, #5490:
URL: https://github.com/apache/iceberg/pull/5490
This way it shows nicely in the CLI
--
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 #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#issuecomment-1213237967
Looks great. Thanks, @Fokko!
--
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] Fokko commented on a diff in pull request #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#discussion_r943227047
##########
python/pyiceberg/table/sorting.py:
##########
@@ -36,11 +36,17 @@ class SortDirection(Enum):
ASC = "asc"
DESC = "desc"
+ def __str__(self):
+ return str(self.name)
+
class NullOrder(Enum):
NULLS_FIRST = "nulls-first"
NULLS_LAST = "nulls-last"
+ def __str__(self):
+ return str(self.name)
Review Comment:
Yes, I think we do want that. I'll make this equivalent to Java: https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/NullOrder.java
--
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 diff in pull request #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#discussion_r942662808
##########
python/pyiceberg/table/sorting.py:
##########
@@ -83,6 +89,9 @@ def set_null_order(cls, values: Dict[str, Any]) -> Dict[str, Any]:
direction: SortDirection = Field()
null_order: NullOrder = Field(alias="null-order")
+ def __str__(self):
+ return f"{self.transform}({self.source_id}) {self.direction} {self.null_order}"
Review Comment:
Missing `)`.
--
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 diff in pull request #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#discussion_r942662292
##########
python/pyiceberg/table/sorting.py:
##########
@@ -36,11 +36,17 @@ class SortDirection(Enum):
ASC = "asc"
DESC = "desc"
+ def __str__(self):
+ return str(self.name)
Review Comment:
`self.name` is already a string, so we don't need to call `str` on it.
--
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] Fokko commented on a diff in pull request #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
Fokko commented on code in PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#discussion_r943220642
##########
python/tests/table/test_sorting.py:
##########
@@ -68,3 +70,12 @@ def test_sorting_schema():
),
)
]
+
+
+def test_sorting_to_string(sort_order: SortOrder):
+ expected = """[
+ identity(19) ASC NULLS_FIRST
Review Comment:
I took inspiration from the Java implementation, and it looks like there is no comma: https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/SortOrder.java#L137-L150
--
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 #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #5490:
URL: https://github.com/apache/iceberg/pull/5490
--
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 diff in pull request #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#discussion_r942664982
##########
python/pyiceberg/table/sorting.py:
##########
@@ -36,11 +36,17 @@ class SortDirection(Enum):
ASC = "asc"
DESC = "desc"
+ def __str__(self):
+ return str(self.name)
+
class NullOrder(Enum):
NULLS_FIRST = "nulls-first"
NULLS_LAST = "nulls-last"
+ def __str__(self):
+ return str(self.name)
Review Comment:
Do we want this to be the SQL sort order? If so, we probably want to remove the `_` or `-` in the name or value. Otherwise we'll get output like `day(ts) DESC NULLS_LAST`.
##########
python/pyiceberg/table/sorting.py:
##########
@@ -83,6 +89,9 @@ def set_null_order(cls, values: Dict[str, Any]) -> Dict[str, Any]:
direction: SortDirection = Field()
null_order: NullOrder = Field(alias="null-order")
+ def __str__(self):
+ return f"{self.transform}({self.source_id}) {self.direction} {self.null_order}"
Review Comment:
Also, if `transform` is the identity transform, it should be omitted. Rather than `identity(category) ASC NULLS_FIRST` it should be `category ASC NULLS_FIRST`.
--
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 diff in pull request #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#discussion_r942662808
##########
python/pyiceberg/table/sorting.py:
##########
@@ -83,6 +89,9 @@ def set_null_order(cls, values: Dict[str, Any]) -> Dict[str, Any]:
direction: SortDirection = Field()
null_order: NullOrder = Field(alias="null-order")
+ def __str__(self):
+ return f"{self.transform}({self.source_id}) {self.direction} {self.null_order}"
Review Comment:
Missing `)`.
##########
python/pyiceberg/table/sorting.py:
##########
@@ -83,6 +89,9 @@ def set_null_order(cls, values: Dict[str, Any]) -> Dict[str, Any]:
direction: SortDirection = Field()
null_order: NullOrder = Field(alias="null-order")
+ def __str__(self):
+ return f"{self.transform}({self.source_id}) {self.direction} {self.null_order}"
Review Comment:
If `transform` is the identity transform, it should be omitted. Rather than `identity(category) ASC NULLS_FIRST` it should be `category ASC NULLS_FIRST`.
--
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 diff in pull request #5490: Python: Add __str__ implementation to SortOrder and SortField
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #5490:
URL: https://github.com/apache/iceberg/pull/5490#discussion_r942668475
##########
python/tests/table/test_sorting.py:
##########
@@ -68,3 +70,12 @@ def test_sorting_schema():
),
)
]
+
+
+def test_sorting_to_string(sort_order: SortOrder):
+ expected = """[
+ identity(19) ASC NULLS_FIRST
Review Comment:
Should there be a comma ending each line since this is in `[...]` as a list?
--
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