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 2021/02/16 02:08:03 UTC
[GitHub] [iceberg] aokolnychyi opened a new pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
aokolnychyi opened a new pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239
This PR avoid using the schema from `PartitionSpec` in `SortOrderUtil` since unpartitioned specs have an empty schema.
----------------------------------------------------------------
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.
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] aokolnychyi closed pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi closed pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239
----------------------------------------------------------------
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.
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] aokolnychyi commented on pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#issuecomment-786278708
Thanks for reviewing, @RussellSpitzer @rdblue!
----------------------------------------------------------------
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.
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] aokolnychyi commented on a change in pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r581323560
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
+ public static SortOrder buildSortOrder(Schema schema, PartitionSpec spec, SortOrder sortOrder) {
Review comment:
Sounds good. Let me update it.
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
+ public static SortOrder buildSortOrder(Schema schema, PartitionSpec spec, SortOrder sortOrder) {
Review comment:
Fixed.
----------------------------------------------------------------
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.
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] aokolnychyi commented on a change in pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r576507316
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
+ public static SortOrder buildSortOrder(Schema schema, PartitionSpec spec, SortOrder sortOrder) {
if (sortOrder.isUnsorted() && spec.isUnpartitioned()) {
return SortOrder.unsorted();
}
- Schema schema = spec.schema();
Review comment:
It wasn't possible to call this method with `PartitionSpec.unpartitioned()` as the schema in the spec was empty.
----------------------------------------------------------------
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.
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] aokolnychyi commented on a change in pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r580739645
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
+ public static SortOrder buildSortOrder(Schema schema, PartitionSpec spec, SortOrder sortOrder) {
Review comment:
I think the original API you guys had internally had this syntax. In OSS, we have access to `Table` wherever this is used. Internally, we temporarily use this version but I have no issues making it private in OSS. Let me know if you use this version, @rdblue.
----------------------------------------------------------------
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.
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] aokolnychyi commented on a change in pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r576507630
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
Review comment:
I may need to keep the old signature for compatibility but deprecate it. It is a util class, though.
----------------------------------------------------------------
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.
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 #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r581302851
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
+ public static SortOrder buildSortOrder(Schema schema, PartitionSpec spec, SortOrder sortOrder) {
Review comment:
I don't think we use it. We should be able to pass the table, I think.
----------------------------------------------------------------
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.
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] aokolnychyi merged pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi merged pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239
----------------------------------------------------------------
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.
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 #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r580695680
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
Review comment:
How was this failing before? The unpartitioned spec isn't usually returned by a table once it has been written and loaded from disk, right?
----------------------------------------------------------------
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.
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 #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r580696483
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
+ public static SortOrder buildSortOrder(Schema schema, PartitionSpec spec, SortOrder sortOrder) {
Review comment:
Is this used anywhere? Maybe we should make it private instead of just changing 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.
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] aokolnychyi commented on a change in pull request #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
aokolnychyi commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r580738145
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
Review comment:
We have a few places where it is used before the table is created. That's where we hit it. Should not affect usual cases.
----------------------------------------------------------------
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.
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 #2239: Core: Support unpartitioned specs in SortOrderUtil
Posted by GitBox <gi...@apache.org>.
rdblue commented on a change in pull request #2239:
URL: https://github.com/apache/iceberg/pull/2239#discussion_r581302851
##########
File path: core/src/main/java/org/apache/iceberg/util/SortOrderUtil.java
##########
@@ -37,16 +37,14 @@ private SortOrderUtil() {
}
public static SortOrder buildSortOrder(Table table) {
- return buildSortOrder(table.spec(), table.sortOrder());
+ return buildSortOrder(table.schema(), table.spec(), table.sortOrder());
}
- public static SortOrder buildSortOrder(PartitionSpec spec, SortOrder sortOrder) {
+ public static SortOrder buildSortOrder(Schema schema, PartitionSpec spec, SortOrder sortOrder) {
Review comment:
I don't think we use it. We should be able to pass the table, I think. Let's make it private then.
----------------------------------------------------------------
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.
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