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/07/24 03:09:42 UTC
[GitHub] [iceberg] zhaomin1423 opened a new pull request, #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
zhaomin1423 opened a new pull request, #5346:
URL: https://github.com/apache/iceberg/pull/5346
The IcebergSource#propertyAsLong is redundant, it can be replaced by PropertyUtil#propertyAsLong.
--
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] szehon-ho commented on a diff in pull request #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #5346:
URL: https://github.com/apache/iceberg/pull/5346#discussion_r936947297
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/IcebergSource.java:
##########
@@ -122,8 +122,8 @@ private Spark3Util.CatalogAndIdentifier catalogAndIdentifier(CaseInsensitiveStri
setupDefaultSparkCatalogs(spark);
String path = options.get(SparkReadOptions.PATH);
- Long snapshotId = propertyAsLong(options, SparkReadOptions.SNAPSHOT_ID);
- Long asOfTimestamp = propertyAsLong(options, SparkReadOptions.AS_OF_TIMESTAMP);
+ Long snapshotId = PropertyUtil.propertyAsLong(options, SparkReadOptions.SNAPSHOT_ID, -1);
+ Long asOfTimestamp = PropertyUtil.propertyAsLong(options, SparkReadOptions.AS_OF_TIMESTAMP, -1);
Review Comment:
In this case you have to change the following code right to expect -1?
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/IcebergSource.java:
##########
@@ -122,8 +122,8 @@ private Spark3Util.CatalogAndIdentifier catalogAndIdentifier(CaseInsensitiveStri
setupDefaultSparkCatalogs(spark);
String path = options.get(SparkReadOptions.PATH);
- Long snapshotId = propertyAsLong(options, SparkReadOptions.SNAPSHOT_ID);
- Long asOfTimestamp = propertyAsLong(options, SparkReadOptions.AS_OF_TIMESTAMP);
+ Long snapshotId = PropertyUtil.propertyAsLong(options, SparkReadOptions.SNAPSHOT_ID, -1);
+ Long asOfTimestamp = PropertyUtil.propertyAsLong(options, SparkReadOptions.AS_OF_TIMESTAMP, -1);
Review Comment:
In this case, dont we have to change the following code to expect -1?
--
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] zhaomin1423 commented on pull request #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
Posted by GitBox <gi...@apache.org>.
zhaomin1423 commented on PR #5346:
URL: https://github.com/apache/iceberg/pull/5346#issuecomment-1213625542
> @zhaomin1423 yea its a good question, it seems it was done this way in the initial split of the Spark modules to different versions. I think most of the spark code is actually the same across versions :(. I guess the other option is to have just one spark module and have a shim layer to take care of different things, but not sure that's easy either.
Keeping a shim layer is difficult with the addition of versions，because a lot of reflect will be added, it is obscure when reading, and some question can't be found in compile stage. Adding a spark common module to put same code, it might be more easier.
--
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] zhaomin1423 commented on a diff in pull request #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
Posted by GitBox <gi...@apache.org>.
zhaomin1423 commented on code in PR #5346:
URL: https://github.com/apache/iceberg/pull/5346#discussion_r936205856
##########
core/src/main/java/org/apache/iceberg/util/PropertyUtil.java:
##########
@@ -57,7 +57,7 @@ public static int propertyAsInt(Map<String, String> properties,
}
public static long propertyAsLong(Map<String, String> properties,
- String property, long defaultValue) {
+ String property, Long defaultValue) {
Review Comment:
Thanks for your opinion，have updated.
--
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] zhaomin1423 commented on a diff in pull request #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
Posted by GitBox <gi...@apache.org>.
zhaomin1423 commented on code in PR #5346:
URL: https://github.com/apache/iceberg/pull/5346#discussion_r937330148
##########
spark/v3.3/spark/src/main/java/org/apache/iceberg/spark/source/IcebergSource.java:
##########
@@ -122,8 +122,8 @@ private Spark3Util.CatalogAndIdentifier catalogAndIdentifier(CaseInsensitiveStri
setupDefaultSparkCatalogs(spark);
String path = options.get(SparkReadOptions.PATH);
- Long snapshotId = propertyAsLong(options, SparkReadOptions.SNAPSHOT_ID);
- Long asOfTimestamp = propertyAsLong(options, SparkReadOptions.AS_OF_TIMESTAMP);
+ Long snapshotId = PropertyUtil.propertyAsLong(options, SparkReadOptions.SNAPSHOT_ID, -1);
+ Long asOfTimestamp = PropertyUtil.propertyAsLong(options, SparkReadOptions.AS_OF_TIMESTAMP, -1);
Review Comment:
Thanks for your help, have updated.
--
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] szehon-ho commented on pull request #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on PR #5346:
URL: https://github.com/apache/iceberg/pull/5346#issuecomment-1213339808
@zhaomin1423 yea its a good question, it seems it was done this way in the initial split of the Spark modules to different versions. I think most of the spark code is actually the same across versions :(. I guess the other option is to keep have just one spark module and have a shim layer to take care of different things, but not sure that's easy either.
--
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] szehon-ho commented on a diff in pull request #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
Posted by GitBox <gi...@apache.org>.
szehon-ho commented on code in PR #5346:
URL: https://github.com/apache/iceberg/pull/5346#discussion_r935891855
##########
core/src/main/java/org/apache/iceberg/util/PropertyUtil.java:
##########
@@ -57,7 +57,7 @@ public static int propertyAsInt(Map<String, String> properties,
}
public static long propertyAsLong(Map<String, String> properties,
- String property, long defaultValue) {
+ String property, Long defaultValue) {
Review Comment:
Not a huge fan of changing signature , even this minor way.
Would a better option be just to change the consuming code to expect a different non-null default value? (like -1)
--
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] zhaomin1423 commented on pull request #5346: Substitute the method of PropertyUtil#propertyAsLong for IcebergSource#propertyAsLong
Posted by GitBox <gi...@apache.org>.
zhaomin1423 commented on PR #5346:
URL: https://github.com/apache/iceberg/pull/5346#issuecomment-1210054609
> @zhaomin1423 actually do you mind doing the same change for other spark versions? I feel normally we do feature for specific spark version(s), but for something like this that's purely cosmetic, would prefer to keep the code synchronized so its easier to backport changes if necessary.
Sorry for late reply. I'm wondering, why not use a common module or abstract class for repeated logical.
--
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